If the user called `stop()` before the init message (the response to the /connect request) was received and the init message was not received within the `TransportConnectTimeout` we would have thrown an exception saying "transport timed out when trying to connect". This is unexpected since the user had already called `.stop()` on the connection. The issue was that in the thread that was terminating the `start()` request in case of connect timeout we did not check if the connection hadn't been stopped. After the change, if the connection had been stopped we just complete the `connect_request_tce` and let the continuation to take care of the rest (which results in throwing pplx::task_canceled exception since the `start()` was cancelled by invoking the `stop()` function).
Bonus: removing the test that tests that the transport is being stopped if the instance goes out of scope without first `disconnect()`ing first. This test is flakey and there is no good way of making it more reliable. The `websocket_transport` class is meant to only be used internally and we always `disconnect()` the transport when the connection is being stopped.
- the name of the cpprestsdk was incorrect so the dependency could not be resolved
- the name of the .targets file was not updated so the .targets file was not installed into a project the package was installed in
- there was a stale .targets file after the previous rename
Note: cpprestsdk moved to shipping multiple smaller packages for different platforms so renamed SignalR package to reflect this. We would most likely adopt this model as well.
When we stop a connection we just signal that the transport should be stopped but don't wait for it to actually stop. If then the same connection is immediately started the old transport can still be running. Because the callbacks are not reset when stopping the transport errors and messages from the old transport can still invoke them on the newly started connection. This is especially harmful in case of errors because they would cause reconnects. The fix is to ignore any message or error received from a transport started by a connection that has already been stopped.
Previously to prevent the user from deadlocking themselves in case the called `connection.stop()` from the `reconnecting` event we would first start the reconnecting process and then called the `reconnecting` event. This was racey since it was (very unlikely but still) possible that reconnect would finish quickly and would invoke `reconnected` callback before the `reconnecting` callback was invoked (happens once in a blue moon on a @BrennanConroy 's box). Another issue was that if the user called `connection.stop()` and `connection.start()` in the event the connection could be dropped so we would start a new reconnect attempt and as a result we would have 2 reconnecting attempts running in parallel but using the same disconnect cts and start event. The fix is to invoke the `reconnecting` event before starting the reconnecting process. We also copy the current disconnect cts which makes it possible to check if the connection was restarted in the `reconnecting` callback and if it was we are canceling this reconnect. This makes it impossible to have multiple concurrent reconnects.
We were signalling the `m_start_completed_event` after the connection reconnected but before we changed the connection state to `connected`. If there was an ongoing call to the `stop()` function the thread would get unblocked and could change the connection state to `disconnecting` before the thread doing the reconnect would have a chance to change the connection state to `connected`. This caused an assert because we assume that if reconnecting succeeded we should go from the `reconnecting` to the `connected` state. The fix is to signal the event after we changed the state. It is also correct because the connection has reconnected succesfully so the state it should be is the `connected` state.
`trace_log_writer` is the default log writer which (for windows) is using `OutputDebugString` to log the client activity. It was only public because it was used as the default parameter value for `log_writer` in the ctors of the `connection` and `hub_connection` classes. This class was not really meant to be used externally (and after one of the previous changes it could not even be used externally because the `write` method was not exported from the dll) so I am moving it from the public includes folder into the implementation folder.
- fixing paths to includes (how did it even compile before, especially on the CI?)
- `websockets` namespace was pulled out from the `experimental` namespace and renamed from `web_sockets`
- #ifdefing Windows specific stuff in stdafx.h
- Adding common 3rd party headers to stdafx.h to precompile them - makes build 25% faster
- Removing targetver.h - it was merged with stdafx.h
Reasons:
- secure websockets apparently did not work correctly in 2.2.0
- 2.4.0.1 has v140 compatible binaries we will need
Other changes:
- using headers installed with NuGet package and not with VS
- enforcing using the `std::min` function istead of the `min` macro defined only in a windows header file
moozzyk
Pawel Kadluczka
Pawel Kadluczka
Xiaohongt
BrennanConroy