Fix stop to properly wait (#73)

Author: BrennanConroy

include/signalrclient/cancellation_token.h

@@ -5,6 +5,7 @@
 #pragma once
 
 #include <memory>
+#include <functional>
 
 namespace signalr
 {

include/signalrclient/connection.h

@@ -53,11 +53,14 @@ namespace signalr
         cancellation_token_source(const cancellation_token_source&) = delete;
         cancellation_token_source& operator=(const cancellation_token_source&) = delete;
 
-        void cancel()
+        // returns false if already canceled, otherwise true if this cancels
+        bool cancel()
         {
+            bool signal = false;
             std::vector<std::function<void()>> callbacks;
             {
                 std::lock_guard<std::mutex> lock(m_lock);
+                signal = !m_signaled;
                 m_signaled = true;
                 m_condition.notify_all();
                 callbacks = std::move(m_callbacks);
@@ -84,6 +87,8 @@ namespace signalr
                     throw errors;
                 }
             }
+
+            return signal;
         }
 
         void reset()

src/signalrclient/cancellation_token_source.h

@@ -3,19 +3,33 @@
 // See the LICENSE file in the project root for more information.
 
 #include "stdafx.h"
-#include "signalrclient/connection.h"
+#include "connection.h"
 #include "signalrclient/transport_type.h"
 #include "connection_impl.h"
+#include "completion_event.h"
 
 namespace signalr
 {
-    connection::connection(const std::string& url, trace_level trace_level, std::shared_ptr<log_writer> log_writer)
-        : m_pImpl(connection_impl::create(url, trace_level, std::move(log_writer)))
+    connection::connection(const std::string& url, trace_level trace_level, std::shared_ptr<log_writer> log_writer,
+        std::function<std::shared_ptr<http_client>(const signalr_client_config&)> http_client_factory,
+        std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory, bool skip_negotiation)
+        : m_pImpl(connection_impl::create(url, trace_level, std::move(log_writer), http_client_factory, websocket_factory, skip_negotiation))
     {}
 
     // Do NOT remove this destructor. Letting the compiler generate and inline the default dtor may lead to
     // undefinded behavior since we are using an incomplete type. More details here:  http://herbsutter.com/gotw/_100/
-    connection::~connection() = default;
+    connection::~connection()
+    {
+        if (m_pImpl)
+        {
+            completion_event completion;
+            m_pImpl->stop([completion](std::exception_ptr) mutable
+                {
+                    completion.set();
+                }, nullptr);
+            completion.get();
+        }
+    }
 
     void connection::start(std::function<void(std::exception_ptr)> callback) noexcept
     {

src/signalrclient/connection.cpp

@@ -0,0 +1,57 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#pragma once
+
+#include <memory>
+#include <functional>
+#include "signalrclient/connection_state.h"
+#include "signalrclient/trace_level.h"
+#include "signalrclient/log_writer.h"
+#include "signalrclient/signalr_client_config.h"
+#include "signalrclient/transfer_format.h"
+#include "signalrclient/http_client.h"
+#include "signalrclient/websocket_client.h"
+
+namespace signalr
+{
+    class connection_impl;
+
+    class connection
+    {
+    public:
+        typedef std::function<void(std::string&&)> message_received_handler;
+
+        explicit connection(const std::string& url, trace_level trace_level = trace_level::info, std::shared_ptr<log_writer> log_writer = nullptr,
+            std::function<std::shared_ptr<http_client>(const signalr_client_config&)> http_client_factory = nullptr,
+            std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory = nullptr, bool skip_negotiation = false);
+
+        ~connection();
+
+        connection(const connection&) = delete;
+
+        connection& operator=(const connection&) = delete;
+
+        void start(std::function<void(std::exception_ptr)> callback) noexcept;
+
+        void send(const std::string& data, transfer_format transfer_format, std::function<void(std::exception_ptr)> callback) noexcept;
+
+        void set_message_received(const message_received_handler& message_received_callback);
+        void set_disconnected(const std::function<void(std::exception_ptr)>& disconnected_callback);
+
+        void set_client_config(const signalr_client_config& config);
+
+        void stop(std::function<void(std::exception_ptr)> callback, std::exception_ptr exception) noexcept;
+
+        connection_state get_connection_state() const noexcept;
+        std::string get_connection_id() const;
+
+    private:
+        // The recommended smart pointer to use when doing pImpl is the `std::unique_ptr`. However
+        // we are capturing the m_pImpl instance in the lambdas used by tasks which can outlive
+        // the connection instance. Using `std::shared_ptr` guarantees that we won't be using
+        // a deleted object if the task is run after the `connection` instance goes away.
+        std::shared_ptr<connection_impl> m_pImpl;
+    };
+}

src/signalrclient/connection.h

@@ -20,11 +20,6 @@
 
 namespace signalr
 {
-    std::shared_ptr<connection_impl> connection_impl::create(const std::string& url, trace_level trace_level, const std::shared_ptr<log_writer>& log_writer)
-    {
-        return connection_impl::create(url, trace_level, log_writer, nullptr, nullptr, false);
-    }
-
     std::shared_ptr<connection_impl> connection_impl::create(const std::string& url, trace_level trace_level, const std::shared_ptr<log_writer>& log_writer,
         std::function<std::shared_ptr<http_client>(const signalr_client_config&)> http_client_factory, std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory, const bool skip_negotiation)
     {

src/signalrclient/connection_impl.cpp

@@ -27,8 +27,6 @@ namespace signalr
     class connection_impl : public std::enable_shared_from_this<connection_impl>
     {
     public:
-        static std::shared_ptr<connection_impl> create(const std::string& url, trace_level trace_level, const std::shared_ptr<log_writer>& log_writer);
-
         static std::shared_ptr<connection_impl> create(const std::string& url, trace_level trace_level, const std::shared_ptr<log_writer>& log_writer,
             std::function<std::shared_ptr<http_client>(const signalr_client_config&)> http_client_factory, std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory, bool skip_negotiation = false);
 

src/signalrclient/connection_impl.h

@@ -31,7 +31,18 @@ namespace signalr
 
     // Do NOT remove this destructor. Letting the compiler generate and inline the default dtor may lead to
     // undefined behavior since we are using an incomplete type. More details here:  http://herbsutter.com/gotw/_100/
-    hub_connection::~hub_connection() = default;
+    hub_connection::~hub_connection()
+    {
+        if (m_pImpl)
+        {
+            completion_event completion;
+            m_pImpl->stop([completion](std::exception_ptr) mutable
+                {
+                    completion.set();
+                });
+            completion.get();
+        }
+    }
 
     void hub_connection::start(std::function<void(std::exception_ptr)> callback) noexcept
     {

src/signalrclient/hub_connection.cpp

@@ -38,11 +38,11 @@ namespace signalr
     hub_connection_impl::hub_connection_impl(const std::string& url, std::unique_ptr<hub_protocol>&& hub_protocol, trace_level trace_level,
         const std::shared_ptr<log_writer>& log_writer, std::function<std::shared_ptr<http_client>(const signalr_client_config&)> http_client_factory,
         std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory, const bool skip_negotiation)
-        : m_connection(connection_impl::create(url, trace_level, log_writer,
-            http_client_factory, websocket_factory, skip_negotiation)), m_logger(log_writer, trace_level),
+        : m_connection(connection_impl::create(url, trace_level, log_writer, http_client_factory, websocket_factory, skip_negotiation))
+            , m_logger(log_writer, trace_level),
         m_callback_manager("connection went out of scope before invocation result was received"),
         m_handshakeReceived(false), m_disconnected([](std::exception_ptr) noexcept {}), m_protocol(std::move(hub_protocol))
-    {}
+    { }
 
     void hub_connection_impl::initialize()
     {

src/signalrclient/hub_connection_impl.cpp

@@ -5,12 +5,15 @@
 #pragma once
 
 #include <unordered_map>
-#include "connection_impl.h"
 #include "callback_manager.h"
 #include "case_insensitive_comparison_utils.h"
 #include "completion_event.h"
 #include "signalrclient/signalr_value.h"
 #include "hub_protocol.h"
+#include "connection.h"
+#include "logger.h"
+#include "cancellation_token_source.h"
+#include "connection_impl.h"
 
 namespace signalr
 {