PurePollution - changing the way we inject factories

Before we would create factories in the `connection` class and pass references to them when creating the `connection_impl` instance. In the long run this won't work for at least couple of reasons:
- transports will have to have a reference to the `connection_impl` to be able to let connection know about events (e.g. message received, disconnected, error etc). As a result to create a transport a (smart) pointer to the `connection_impl` will have to be passed to the `transport_factory.create()` method. However this would require making the `connection_impl type` (and all types it uses) public because the factories are members of a public (i.e. the `connection`) type. An alternative is to use uniqe pointers which will be set to default factories in the `connection_impl` itself. Tests will still be able to inject their own implementation. An additional advantage of using pointers is that now we can hide a few types that were only exposed because we had references in the connection class and since the references are gone the types don't have to exposed anymore.
- because transports will have to have a back pointer to a `connection_impl` instance it will be possible that the `connection_impl` instance will outlive the `connection` object itself (think about tasks that captured a pointer to `connection_impl` and that running after the `connection` object goes out of scope). In this scenario the transport/`connection_impl` may want to use factories while they were already destroyed because they were members of an object that has already been destroyed. This would result in undefined behavior and most likely a crash. Moving factories to the `connection_impl` fixes this issue (note: currently the `connection` type holds a raw pointer to a `connection_impl` instance which is `delete`d in the `connection`'s dtor; this will have to be changed (most likely by changing the raw pointer to a smart pointer) to prevent from trying to use an already disposed pointer in transports - same as above)
As a result of this change we are 'polluting' the code base with smart pointers but the public surface is cleaner than before.

include/signalrclient/connection.h

@@ -6,8 +6,6 @@
 #include <cpprest\http_client.h>
 #include "_exports.h"
 #include "transport_type.h"
-#include "web_request_factory.h"
-#include "transport_factory.h"
 #include "connection_state.h"
 
 namespace signalr
@@ -30,9 +28,6 @@ namespace signalr
         SIGNALRCLIENT_API connection_state get_connection_state() const;
 
     private:
-        web_request_factory m_web_request_factory;
-        transport_factory m_transport_factory;
-
         connection_impl *m_pImpl;
     };
 }

src/signalrclient/Build/VS2013/signalrclient.vcxproj

@@ -93,13 +93,8 @@
     <ClInclude Include="..\..\..\..\include\signalrclient\log_writer.h" />
     <ClInclude Include="..\..\..\..\include\signalrclient\trace_level.h" />
     <ClInclude Include="..\..\..\..\include\signalrclient\trace_log_writer.h" />
-    <ClInclude Include="..\..\..\..\include\signalrclient\transport.h" />
-    <ClInclude Include="..\..\..\..\include\signalrclient\transport_factory.h" />
     <ClInclude Include="..\..\..\..\include\signalrclient\transport_type.h" />
     <ClInclude Include="..\..\..\..\include\signalrclient\web_exception.h" />
-    <ClInclude Include="..\..\..\..\include\signalrclient\web_request.h" />
-    <ClInclude Include="..\..\..\..\include\signalrclient\web_request_factory.h" />
-    <ClInclude Include="..\..\..\..\include\signalrclient\web_response.h" />
     <ClInclude Include="..\..\..\..\include\signalrclient\_exports.h" />
     <ClInclude Include="..\..\connection_impl.h" />
     <ClInclude Include="..\..\constants.h" />
@@ -110,9 +105,14 @@
     <ClInclude Include="..\..\request_sender.h" />
     <ClInclude Include="..\..\stdafx.h" />
     <ClInclude Include="..\..\targetver.h" />
+    <ClInclude Include="..\..\transport.h" />
+    <ClInclude Include="..\..\transport_factory.h" />
     <ClInclude Include="..\..\url_builder.h" />
     <ClInclude Include="..\..\websocket_client.h" />
     <ClInclude Include="..\..\websocket_transport.h" />
+    <ClInclude Include="..\..\web_request.h" />
+    <ClInclude Include="..\..\web_request_factory.h" />
+    <ClInclude Include="..\..\web_response.h" />
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="..\..\connection.cpp" />

src/signalrclient/Build/VS2013/signalrclient.vcxproj.filters

@@ -57,21 +57,6 @@
     <ClInclude Include="..\..\connection_impl.h">
       <Filter>Header Files</Filter>
     </ClInclude>
-    <ClInclude Include="..\..\..\..\include\signalrclient\transport.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
-    <ClInclude Include="..\..\..\..\include\signalrclient\transport_factory.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
-    <ClInclude Include="..\..\..\..\include\signalrclient\web_request.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
-    <ClInclude Include="..\..\..\..\include\signalrclient\web_request_factory.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
-    <ClInclude Include="..\..\..\..\include\signalrclient\web_response.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
     <ClInclude Include="..\..\..\..\include\signalrclient\connection_state.h">
       <Filter>Header Files</Filter>
     </ClInclude>
@@ -90,6 +75,21 @@
     <ClInclude Include="..\..\..\..\include\signalrclient\trace_log_writer.h">
       <Filter>Header Files</Filter>
     </ClInclude>
+    <ClInclude Include="..\..\transport_factory.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="..\..\web_request_factory.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="..\..\transport.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="..\..\web_response.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="..\..\web_request.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="..\..\stdafx.cpp">

src/signalrclient/connection.cpp

@@ -4,15 +4,13 @@
 #include "stdafx.h"
 #include "signalrclient\connection.h"
 #include "signalrclient\transport_type.h"
-#include "signalrclient\web_request_factory.h"
-#include "signalrclient\transport_factory.h"
 #include "connection_impl.h"
 
 namespace signalr
 {
     connection::connection(const utility::string_t& url, const utility::string_t& querystring)
     {
-        m_pImpl = new connection_impl(url, querystring, m_web_request_factory, m_transport_factory);
+        m_pImpl = new connection_impl(url, querystring);
     }
 
     connection::~connection()

src/signalrclient/connection_impl.cpp

@@ -8,10 +8,14 @@
 
 namespace signalr
 {
+    connection_impl::connection_impl(const utility::string_t& url, const utility::string_t& querystring)
+        : connection_impl(url, querystring, std::make_unique<web_request_factory>(), std::make_unique<transport_factory>())
+    { }
+
     connection_impl::connection_impl(const utility::string_t& url, const utility::string_t& querystring,
-        web_request_factory& web_request_factory, transport_factory& transport_factory)
-        : m_base_uri(url), m_querystring(querystring), m_web_request_factory(web_request_factory),
-        m_transport_factory(transport_factory), m_connection_state(connection_state::disconnected)
+        std::unique_ptr<web_request_factory> web_request_factory, std::unique_ptr<transport_factory> transport_factory)
+        : m_base_uri(url), m_querystring(querystring), m_web_request_factory(std::move(web_request_factory)), 
+        m_transport_factory(std::move(transport_factory)), m_connection_state(std::move(connection_state::disconnected))
     { }
 
     pplx::task<void> connection_impl::start()

src/signalrclient/connection_impl.h

@@ -5,19 +5,21 @@
 
 #include <atomic>
 #include <cpprest\http_client.h>
-#include "signalrclient\web_request_factory.h"
-#include "signalrclient\transport_factory.h"
+#include "signalrclient\trace_level.h"
 #include "signalrclient\connection_state.h"
+#include "web_request_factory.h"
+#include "transport_factory.h"
+#include "logger.h"
 
 namespace signalr
 {
     class connection_impl
     {
     public:
-        // taking references to be able to inject factories for test purposes. The caller must 
-        // make sure that actual instances outlive connection_impl
+        connection_impl(const utility::string_t& url, const utility::string_t& querystring);
+
         connection_impl(const utility::string_t& url, const utility::string_t& querystring,
-            web_request_factory& web_request_factory, transport_factory& transport_factory);
+            std::unique_ptr<web_request_factory> web_request_factory, std::unique_ptr<transport_factory> transport_factory);
 
         connection_impl(const connection_impl&) = delete;
 
@@ -32,8 +34,8 @@ namespace signalr
         utility::string_t m_querystring;
         std::atomic<connection_state> m_connection_state;
 
-        web_request_factory &m_web_request_factory;
-        transport_factory& m_transport_factory;
+        std::unique_ptr<web_request_factory> m_web_request_factory;
+        std::unique_ptr<transport_factory> m_transport_factory;
 
         bool change_state(connection_state old_state, connection_state new_state);
     };

src/signalrclient/http_sender.cpp

@@ -4,7 +4,7 @@
 #include "stdafx.h"
 #include <cpprest\http_client.h>
 #include "signalrclient\web_exception.h"
-#include "signalrclient\web_request.h"
+#include "web_request.h"
 #include "constants.h"
 
 namespace signalr

src/signalrclient/http_sender.h

@@ -6,7 +6,7 @@
 #include <ppl.h>
 #include <cpprest\basic_types.h>
 
-#include "signalrclient\web_request.h"
+#include "web_request.h"
 
 namespace pplx = concurrency;
 

src/signalrclient/request_sender.h

@@ -4,7 +4,7 @@
 #pragma once
 
 #include <cpprest\base_uri.h>
-#include "signalrclient\web_request_factory.h"
+#include "web_request_factory.h"
 #include "negotiation_response.h"
 
 namespace signalr

src/signalrclient/transport_factory.cpp

@@ -2,7 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 #include "stdafx.h"
-#include "signalrclient\transport_factory.h"
+#include "transport_factory.h"
 #include "websocket_transport.h"
 
 namespace signalr

src/signalrclient/web_request.cpp

@@ -3,7 +3,7 @@
 
 #include "stdafx.h"
 #include <cpprest\http_client.h>
-#include "signalrclient\web_request.h"
+#include "web_request.h"
 
 namespace signalr
 {

src/signalrclient/web_request_factory.cpp

@@ -2,7 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 #include "stdafx.h"
-#include "signalrclient\web_request_factory.h"
+#include "web_request_factory.h"
 
 namespace signalr
 {

src/signalrclient/websocket_transport.cpp

@@ -111,7 +111,6 @@ namespace signalr
 
                     return;
                 }
-
                 // TODO: log, report error, close websocket (when appropriate)
                 catch (const web_sockets::client::websocket_exception&)
                 {

src/signalrclient/websocket_transport.h

@@ -5,7 +5,8 @@
 
 #include <cpprest\ws_client.h>
 #include "url_builder.h"
-#include "signalrclient\transport.h"
+#include "transport.h"
+#include "logger.h"
 #include "default_websocket_client.h"
 
 namespace signalr

test/signalrclienttests/connection_impl_tests.cpp

@@ -3,28 +3,23 @@
 
 #include "stdafx.h"
 #include "connection_impl.h"
-#include "signalrclient\web_request_factory.h"
-#include "signalrclient\transport_factory.h"
+#include "signalrclient\trace_level.h"
+#include "signalrclient\trace_log_writer.h"
+#include "web_request_factory.h"
+#include "transport_factory.h"
+#include "memory_log_writer.h"
 
 using namespace signalr;
 
 TEST(connection_impl_connection_state, initial_connection_state_is_disconnected)
 {
-    web_request_factory request_factory;
-    transport_factory transport_factory;
-
-    connection_impl connection{ _XPLATSTR("url"), _XPLATSTR(""), request_factory, transport_factory };
-
+    connection_impl connection{ _XPLATSTR("url"), _XPLATSTR("") };
     ASSERT_EQ(connection_state::disconnected, connection.get_connection_state());
 }
 
 TEST(connection_impl_start, cannot_start_non_disconnected_exception)
 {
-    web_request_factory request_factory;
-    transport_factory transport_factory;
-
-    connection_impl connection{ _XPLATSTR("url"), _XPLATSTR(""), request_factory, transport_factory };
-
+    connection_impl connection{ _XPLATSTR("url"), _XPLATSTR("") };
     connection.start().wait();
 
     try
@@ -42,12 +37,8 @@ TEST(connection_impl_start, cannot_start_non_disconnected_exception)
 
 TEST(connection_impl_start, connection_state_is_connecting_when_connection_is_being_started)
 {
-    web_request_factory request_factory;
-    transport_factory transport_factory;
-
-    connection_impl connection{ _XPLATSTR("url"), _XPLATSTR(""), request_factory, transport_factory };
+    connection_impl connection{ _XPLATSTR("url"), _XPLATSTR(""), };
 
     connection.start().wait();
     ASSERT_EQ(connection.get_connection_state(), connection_state::connecting);
 }
-

test/signalrclienttests/test_web_request_factory.h

@@ -3,7 +3,7 @@
 
 #pragma once
 
-#include "signalrclient\web_request_factory.h"
+#include "web_request_factory.h"
 #include "web_request_stub.h"
 
 using namespace signalr;

test/signalrclienttests/web_request_stub.h

@@ -3,8 +3,8 @@
 
 #pragma once
 
-#include "signalrclient\web_request.h"
-#include "signalrclient\web_response.h"
+#include "web_request.h"
+#include "web_response.h"
 
 using namespace signalr;
 

test/signalrclienttests/web_request_tests.cpp

@@ -3,7 +3,7 @@
 
 #include "stdafx.h"
 #include <cpprest\http_listener.h>
-#include "signalrclient\web_request.h"
+#include "web_request.h"
 
 using namespace web;
 using namespace signalr;