Delete open() from _http_client_communicator (#819)

* Delete open() from _http_client_communicator and move its functionality into WinHTTP, as that is the only backend using that thing.

* Some CR comments from Robert.

Release/src/http/client/http_client.cpp

@@ -167,15 +167,15 @@ request_context::request_context(const std::shared_ptr<_http_client_communicator
     responseImpl->_prepare_to_receive_data();
 }
 
-void _http_client_communicator::open_and_send_request_async(const std::shared_ptr<request_context> &request)
+void _http_client_communicator::async_send_request_impl(const std::shared_ptr<request_context> &request)
 {
     auto self = std::static_pointer_cast<_http_client_communicator>(this->shared_from_this());
     // Schedule a task to start sending.
     pplx::create_task([self, request]
     {
         try
         {
-            self->open_and_send_request(request);
+            self->send_request(request);
         }
         catch (...)
         {
@@ -188,20 +188,21 @@ void _http_client_communicator::async_send_request(const std::shared_ptr<request
 {
     if (m_client_config.guarantee_order())
     {
-        pplx::extensibility::scoped_critical_section_t l(m_open_lock);
+        pplx::extensibility::scoped_critical_section_t l(m_client_lock);
 
-        if (++m_scheduled == 1)
+        if (m_outstanding)
         {
-            open_and_send_request_async(request);
+            m_requests_queue.push(request);
         }
         else
         {
-            m_requests_queue.push(request);
+            async_send_request_impl(request);
+            m_outstanding = true;
         }
     }
     else
     {
-        open_and_send_request_async(request);
+        async_send_request_impl(request);
     }
 }
 
@@ -210,16 +211,18 @@ void _http_client_communicator::finish_request()
     // If guarantee order is specified we don't need to do anything.
     if (m_client_config.guarantee_order())
     {
-        pplx::extensibility::scoped_critical_section_t l(m_open_lock);
-
-        --m_scheduled;
+        pplx::extensibility::scoped_critical_section_t l(m_client_lock);
 
-        if (!m_requests_queue.empty())
+        if (m_requests_queue.empty())
+        {
+            m_outstanding = false;
+        }
+        else
         {
             auto request = m_requests_queue.front();
             m_requests_queue.pop();
 
-            open_and_send_request_async(request);
+            async_send_request_impl(request);
         }
     }
 }
@@ -235,46 +238,10 @@ const uri & _http_client_communicator::base_uri() const
 }
 
 _http_client_communicator::_http_client_communicator(http::uri&& address, http_client_config&& client_config)
-    : m_uri(std::move(address)), m_client_config(std::move(client_config)), m_opened(false), m_scheduled(0)
+    : m_uri(std::move(address)), m_client_config(std::move(client_config)), m_outstanding(false)
 {
 }
 
-// Wraps opening the client around sending a request.
-void _http_client_communicator::open_and_send_request(const std::shared_ptr<request_context> &request)
-{
-    // First see if client needs to be opened.
-    unsigned long error = 0;
-
-    if (!m_opened)
-    {
-        pplx::extensibility::scoped_critical_section_t l(m_open_lock);
-
-        // Check again with the lock held
-        if (!m_opened)
-        {
-            error = open();
-
-            if (error == 0)
-            {
-                m_opened = true;
-            }
-        }
-    }
-
-    if (error != 0)
-    {
-        // Failed to open
-        request->report_error(error, _XPLATSTR("Open failed"));
-
-        // DO NOT TOUCH the this pointer after completing the request
-        // This object could be freed along with the request as it could
-        // be the last reference to this object
-        return;
-    }
-
-    send_request(request);
-}
-
 inline void request_context::finish()
 {
     // If cancellation is enabled and registration was performed, unregister.
@@ -437,4 +404,4 @@ pplx::task<http_response> http_client::request(http_request request, const pplx:
 }
 
 
-}}}
+}}}

Release/src/http/client/http_client_asio.cpp

@@ -400,8 +400,6 @@ class asio_client final : public _http_client_communicator
 
     void send_request(const std::shared_ptr<request_context> &request_ctx) override;
 
-    unsigned long open() override { return 0; }
-
     void release_connection(std::shared_ptr<asio_connection>& conn)
     {
         m_pool->release(conn);

Release/src/http/client/http_client_impl.h

@@ -126,35 +126,28 @@ class _http_client_communicator : public http_pipeline_stage
 protected:
     _http_client_communicator(http::uri&& address, http_client_config&& client_config);
 
-    // Method to open client.
-    virtual unsigned long open() = 0;
-
     // HTTP client implementations must implement send_request.
     virtual void send_request(_In_ const std::shared_ptr<request_context> &request) = 0;
 
     // URI to connect to.
     const http::uri m_uri;
 
+    pplx::extensibility::critical_section_t m_client_lock;
 private:
 
     http_client_config m_client_config;
 
-    std::atomic<bool> m_opened;
-
-    pplx::extensibility::critical_section_t m_open_lock;
-
     // Wraps opening the client around sending a request.
-    void open_and_send_request_async(const std::shared_ptr<request_context> &request);
-    void open_and_send_request(const std::shared_ptr<request_context> &request);
+    void async_send_request_impl(const std::shared_ptr<request_context> &request);
 
     // Queue used to guarantee ordering of requests, when applicable.
     std::queue<std::shared_ptr<request_context>> m_requests_queue;
-    int m_scheduled;
+    bool m_outstanding;
 };
 
 /// <summary>
 /// Factory function implemented by the separate platforms to construct their subclasses of _http_client_communicator
 /// </summary>
 std::shared_ptr<_http_client_communicator> create_platform_final_pipeline_stage(uri&& base_uri, http_client_config&& client_config);
 
-}}}}
+}}}}

Release/src/http/client/http_client_winhttp.cpp

@@ -14,6 +14,8 @@
 ****/
 #include "stdafx.h"
 
+#include <atomic>
+
 #include "cpprest/http_headers.h"
 #include "http_client_impl.h"
 
@@ -350,6 +352,7 @@ class winhttp_client : public _http_client_communicator
     winhttp_client(http::uri address, http_client_config client_config)
         : _http_client_communicator(std::move(address), std::move(client_config))
         , m_secure(m_uri.scheme() == _XPLATSTR("https"))
+        , m_opened(false)
         , m_hSession(nullptr)
         , m_hConnection(nullptr) { }
 
@@ -402,8 +405,19 @@ class winhttp_client : public _http_client_communicator
     }
 
     // Open session and connection with the server.
-    virtual unsigned long open() override
+    unsigned long open()
     {
+        if (m_opened)
+        {
+            return 0;
+        }
+
+        pplx::extensibility::scoped_critical_section_t l(m_client_lock);
+        if (m_opened)
+        {
+            return 0;
+        }
+
         // This object have lifetime greater than proxy_name and proxy_bypass
         // which may point to its elements.
         ie_proxy_config proxyIE;
@@ -574,12 +588,24 @@ class winhttp_client : public _http_client_communicator
             return report_failure(_XPLATSTR("Error opening connection"));
         }
 
+        m_opened = true;
         return S_OK;
     }
 
     // Start sending request.
     void send_request(_In_ const std::shared_ptr<request_context> &request)
     {
+        // First see if we need to be opened.
+        unsigned long error = open();
+        if (error != 0)
+        {
+            // DO NOT TOUCH the this pointer after completing the request
+            // This object could be freed along with the request as it could
+            // be the last reference to this object
+            request->report_error(error, _XPLATSTR("Open failed"));
+            return;
+        }
+
         http_request &msg = request->m_request;
         std::shared_ptr<winhttp_request_context> winhttp_context = std::static_pointer_cast<winhttp_request_context>(request);
         std::weak_ptr<winhttp_request_context> weak_winhttp_context = winhttp_context;
@@ -1531,6 +1557,8 @@ class winhttp_client : public _http_client_communicator
     }
     }
 
+    std::atomic<bool> m_opened;
+
     // WinHTTP session and connection
     HINTERNET m_hSession;
     HINTERNET m_hConnection;
@@ -1549,4 +1577,3 @@ std::shared_ptr<_http_client_communicator> create_platform_final_pipeline_stage(
 }
 
 }}}}
-

Release/src/http/client/http_client_winrt.cpp

@@ -378,12 +378,6 @@ class winrt_client : public _http_client_communicator
 
 protected:
 
-    // Method to open client.
-    unsigned long open()
-    {
-        return 0;
-    }
-
     // Start sending request.
     void send_request(_In_ const std::shared_ptr<request_context> &request)
     {