ExceptionToTheRule - incorrect exception thrown if stop() called before init message recieved and /connect timeouts

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.

Author: moozzyk

src/signalrclient/connection_impl.cpp

@@ -225,10 +225,23 @@ namespace signalr
         auto transport = connection->m_transport_factory->create_transport(
             transport_type::websockets, connection->m_logger, connection->m_headers, process_response_callback, error_callback);
 
-        pplx::create_task([negotiation_response, connect_request_tce]()
+        auto& disconnect_cts = m_disconnect_cts;
+        pplx::create_task([negotiation_response, connect_request_tce, disconnect_cts, weak_connection]()
         {
             std::this_thread::sleep_for(std::chrono::milliseconds(negotiation_response.transport_connect_timeout));
-            connect_request_tce.set_exception(std::runtime_error("transport timed out when trying to connect"));
+
+            // if the disconnect_cts is cancelled it means that the connection has been stopped or went out of scope in
+            // which case we should not throw due to timeout. Instead we need to set the tce prevent the task that is
+            // using this tce from hanging indifinitely. (This will eventually result in throwing the pplx::task_canceled
+            // exception to the user since this is what we do in the start() function if disconnect_cts is tripped).
+            if (disconnect_cts.get_token().is_canceled())
+            {
+                connect_request_tce.set();
+            }
+            else
+            {
+                connect_request_tce.set_exception(std::runtime_error("transport timed out when trying to connect"));
+            }
         });
 
         return connection->send_connect_request(transport, negotiation_response.connection_token, connect_request_tce)

test/signalrclienttests/connection_impl_tests.cpp

@@ -878,6 +878,52 @@ TEST(connection_impl_stop, stop_cancels_ongoing_start_request)
     ASSERT_EQ(_XPLATSTR("[state change] connecting -> disconnected\n"), remove_date_from_log_entry(log_entries[4]));
 }
 
+TEST(connection_impl_stop, ongoing_start_request_cancelled_if_connection_stopped_before_init_message_received)
+{
+    auto web_request_factory = std::make_unique<test_web_request_factory>([](const web::uri& url)
+    {
+        auto response_body =
+            url.path() == _XPLATSTR("/negotiate")
+            ? _XPLATSTR("{\"Url\":\"/signalr\", \"ConnectionToken\" : \"A==\", \"ConnectionId\" : \"f7707523-307d-4cba-9abf-3eef701241e8\", ")
+            _XPLATSTR("\"DisconnectTimeout\" : 0.5, \"ConnectionTimeout\" : 110.0, \"TryWebSockets\" : true, ")
+            _XPLATSTR("\"ProtocolVersion\" : \"1.4\", \"TransportConnectTimeout\" : 0.1, \"LongPollDelay\" : 0.0}")
+            : _XPLATSTR("");
+
+        return std::unique_ptr<web_request>(new web_request_stub((unsigned short)200, _XPLATSTR("OK"), response_body));
+    });
+
+    auto websocket_client = create_test_websocket_client(/*receive function*/ []()
+    {
+        return pplx::task_from_result<std::string>("{}");
+    });
+
+    auto writer = std::shared_ptr<log_writer>{std::make_shared<memory_log_writer>()};
+    auto connection = connection_impl::create(create_uri(), _XPLATSTR(""), trace_level::all, writer,
+        std::move(web_request_factory), std::make_unique<test_transport_factory>(websocket_client));
+
+    auto start_task = connection->start();
+    connection->stop().get();
+
+    start_task.then([](pplx::task<void> t)
+    {
+        try
+        {
+            t.get();
+            ASSERT_TRUE(false); // exception expected but not thrown
+        }
+        catch (const pplx::task_canceled &)
+        { }
+    }).get();
+
+    auto log_entries = std::dynamic_pointer_cast<memory_log_writer>(writer)->get_log_entries();
+    ASSERT_EQ(5, log_entries.size());
+    ASSERT_EQ(_XPLATSTR("[state change] disconnected -> connecting\n"), remove_date_from_log_entry(log_entries[0]));
+    ASSERT_EQ(_XPLATSTR("[info        ] stopping connection\n"), remove_date_from_log_entry(log_entries[1]));
+    ASSERT_EQ(_XPLATSTR("[info        ] acquired lock in shutdown()\n"), remove_date_from_log_entry(log_entries[2]));
+    ASSERT_EQ(_XPLATSTR("[info        ] starting the connection has been cancelled.\n"), remove_date_from_log_entry(log_entries[3]));
+    ASSERT_EQ(_XPLATSTR("[state change] connecting -> disconnected\n"), remove_date_from_log_entry(log_entries[4]));
+}
+
 TEST(connection_impl_stop, stop_ignores_exceptions_from_abort_requests)
 {
     auto writer = std::shared_ptr<log_writer>{std::make_shared<memory_log_writer>()};
@@ -1190,25 +1236,6 @@ TEST(connection_impl_reconnect, reconnect_works_if_connection_dropped_during_aft
 {
     auto connection_dropped_event = std::make_shared<pplx::event>();
 
-    auto web_request_factory = std::make_unique<test_web_request_factory>([&connection_dropped_event](const web::uri& url)
-    {
-        if (url.path() == _XPLATSTR("/start"))
-        {
-            connection_dropped_event->wait();
-        }
-
-        auto response_body =
-            url.path() == _XPLATSTR("/negotiate")
-            ? _XPLATSTR("{\"Url\":\"/signalr\", \"ConnectionToken\" : \"A==\", \"ConnectionId\" : \"f7707523-307d-4cba-9abf-3eef701241e8\", ")
-            _XPLATSTR("\"DisconnectTimeout\" : 0.5, \"ConnectionTimeout\" : 110.0, \"TryWebSockets\" : true, ")
-            _XPLATSTR("\"ProtocolVersion\" : \"1.4\", \"TransportConnectTimeout\" : 5.0, \"LongPollDelay\" : 0.0}")
-            : url.path() == _XPLATSTR("/start")
-                ? _XPLATSTR("{\"Response\":\"started\" }")
-                : _XPLATSTR("");
-
-        return std::unique_ptr<web_request>(new web_request_stub((unsigned short)200, _XPLATSTR("OK"), response_body));
-    });
-
     int call_number = -1;
     auto websocket_client = create_test_websocket_client(
         /* receive function */ [call_number, connection_dropped_event]() mutable

test/signalrclienttests/websocket_transport_tests.cpp

@@ -128,30 +128,6 @@ TEST(websocket_transport_connect, can_connect_after_disconnecting)
     // shouldn't throw or crash
 }
 
-TEST(websocket_transport_connect, transport_destroyed_even_if_disconnect_not_called)
-{
-    auto client = std::make_shared<test_websocket_client>();
-    {
-        auto ws_transport = websocket_transport::create([&](){ return client; }, logger(std::make_shared<trace_log_writer>(), trace_level::none),
-            [](const utility::string_t&){}, [](const std::exception&){});
-
-        ws_transport->connect(_XPLATSTR("ws://fakeuri.org")).get();
-    }
-
-    // The receive loop may still run even if the transport goes out of scope above. Since the receive loop may hold
-    // on to the websocket_transport instance by using a shared_ptr the actual instance may not be released even though
-    // the ws_transport variable goes out of scope. Therefore we need to wait for the receive loop to finish but we
-    // don't have any indicator when it happens since it is running on a separate and unsychnronized thread.
-    for (int wait_time_ms = 10; wait_time_ms < 5000 && client.use_count() > 1; wait_time_ms <<= 1)
-    {
-        pplx::wait(wait_time_ms);
-    }
-
-    // the idea is that if the transport is not destroyed it will hold a reference
-    // to the client and therefore the `use_count()` will be greater than 1
-    ASSERT_EQ(1, client.use_count());
-}
-
 TEST(websocket_transport_send, send_creates_and_sends_websocket_messages)
 {
     bool send_called = false;