From aa4065bd24aff94d790b31c4e11115359d8666bf Mon Sep 17 00:00:00 2001 From: Billy O'Neal Date: Wed, 1 Aug 2018 02:01:01 -0700 Subject: [PATCH] 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 | 67 +++++-------------- Release/src/http/client/http_client_asio.cpp | 2 - Release/src/http/client/http_client_impl.h | 15 ++--- .../src/http/client/http_client_winhttp.cpp | 31 ++++++++- Release/src/http/client/http_client_winrt.cpp | 6 -- 5 files changed, 50 insertions(+), 71 deletions(-) diff --git a/Release/src/http/client/http_client.cpp b/Release/src/http/client/http_client.cpp index fdaca8f151..1e4e5036c2 100644 --- a/Release/src/http/client/http_client.cpp +++ b/Release/src/http/client/http_client.cpp @@ -167,7 +167,7 @@ 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) +void _http_client_communicator::async_send_request_impl(const std::shared_ptr &request) { auto self = std::static_pointer_cast<_http_client_communicator>(this->shared_from_this()); // Schedule a task to start sending. @@ -175,7 +175,7 @@ void _http_client_communicator::open_and_send_request_async(const std::shared_pt { 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) -{ - // 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_client::request(http_request request, const pplx: } -}}} \ No newline at end of file +}}} diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index 600bf26605..f9b9ac1f73 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/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_ctx) override; - unsigned long open() override { return 0; } - void release_connection(std::shared_ptr& conn) { m_pool->release(conn); diff --git a/Release/src/http/client/http_client_impl.h b/Release/src/http/client/http_client_impl.h index 4e47687aa4..e4508ebcf1 100644 --- a/Release/src/http/client/http_client_impl.h +++ b/Release/src/http/client/http_client_impl.h @@ -126,30 +126,23 @@ 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) = 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 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); - void open_and_send_request(const std::shared_ptr &request); + void async_send_request_impl(const std::shared_ptr &request); // Queue used to guarantee ordering of requests, when applicable. std::queue> m_requests_queue; - int m_scheduled; + bool m_outstanding; }; /// @@ -157,4 +150,4 @@ class _http_client_communicator : public http_pipeline_stage /// std::shared_ptr<_http_client_communicator> create_platform_final_pipeline_stage(uri&& base_uri, http_client_config&& client_config); -}}}} \ No newline at end of file +}}}} diff --git a/Release/src/http/client/http_client_winhttp.cpp b/Release/src/http/client/http_client_winhttp.cpp index 8f9b2b3146..c81dd6b317 100644 --- a/Release/src/http/client/http_client_winhttp.cpp +++ b/Release/src/http/client/http_client_winhttp.cpp @@ -14,6 +14,8 @@ ****/ #include "stdafx.h" +#include + #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) { + // 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_context = std::static_pointer_cast(request); std::weak_ptr weak_winhttp_context = winhttp_context; @@ -1531,6 +1557,8 @@ class winhttp_client : public _http_client_communicator } } + std::atomic 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( } }}}} - diff --git a/Release/src/http/client/http_client_winrt.cpp b/Release/src/http/client/http_client_winrt.cpp index 4ada65a75b..e8ee50a018 100644 --- a/Release/src/http/client/http_client_winrt.cpp +++ b/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) {