From fe82d73bef7319bbb5b6d4343b13835e45fde5e4 Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Mon, 30 Jul 2018 14:30:24 -0700 Subject: [PATCH 1/2] Delete open() from _http_client_communicator and move its functionality into WinHTTP, as that is the only backend using that thing. --- Release/src/http/client/http_client.cpp | 58 ++++--------------- Release/src/http/client/http_client_asio.cpp | 2 - Release/src/http/client/http_client_impl.h | 14 +---- .../src/http/client/http_client_winhttp.cpp | 28 ++++++++- Release/src/http/client/http_client_winrt.cpp | 6 -- 5 files changed, 40 insertions(+), 68 deletions(-) diff --git a/Release/src/http/client/http_client.cpp b/Release/src/http/client/http_client.cpp index 164bec3f48..be7c038ab8 100644 --- a/Release/src/http/client/http_client.cpp +++ b/Release/src/http/client/http_client.cpp @@ -129,7 +129,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::send_request_async_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. @@ -137,7 +137,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 (...) { @@ -150,11 +150,11 @@ void _http_client_communicator::async_send_request(const std::shared_ptr &request) + : m_uri(std::move(address)), m_client_config(std::move(client_config)) { - // 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() @@ -399,4 +361,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 4ba3e0851b..39e049c1da 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 b292323ee0..2363b8f813 100644 --- a/Release/src/http/client/http_client_impl.h +++ b/Release/src/http/client/http_client_impl.h @@ -117,30 +117,22 @@ 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 send_request_async_impl(const std::shared_ptr &request); // Queue used to guarantee ordering of requests, when applicable. std::queue> m_requests_queue; - int m_scheduled; }; /// @@ -148,4 +140,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 2417859956..16434a3750 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" @@ -403,7 +405,7 @@ class winhttp_client : public _http_client_communicator } // Open session and connection with the server. - virtual unsigned long open() override + unsigned long open() { // This object have lifetime greater than proxy_name and proxy_bypass // which may point to its elements. @@ -581,6 +583,28 @@ class winhttp_client : public _http_client_communicator // Start sending request. void send_request(_In_ const std::shared_ptr &request) { + // First see if we need to be opened. + if (!m_opened) + { + pplx::extensibility::scoped_critical_section_t l(m_client_lock); + + // Check again with the lock held + if (!m_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; + } + + m_opened = true; + } + } + http_request &msg = request->m_request; std::shared_ptr winhttp_context = std::static_pointer_cast(request); std::weak_ptr weak_winhttp_context = winhttp_context; @@ -1546,6 +1570,8 @@ class winhttp_client : public _http_client_communicator } } + std::atomic m_opened; + // WinHTTP session and connection HINTERNET m_hSession; HINTERNET m_hConnection; 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) { From 943dfad5474bc6b2a1cdd49c5033cadd6f4737ad Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Mon, 30 Jul 2018 14:59:17 -0700 Subject: [PATCH 2/2] Some CR comments from Robert. --- Release/src/http/client/http_client.cpp | 21 ++++++---- Release/src/http/client/http_client_impl.h | 3 +- .../src/http/client/http_client_winhttp.cpp | 39 ++++++++++--------- 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/Release/src/http/client/http_client.cpp b/Release/src/http/client/http_client.cpp index be7c038ab8..7b7be26d4c 100644 --- a/Release/src/http/client/http_client.cpp +++ b/Release/src/http/client/http_client.cpp @@ -129,7 +129,7 @@ request_context::request_context(const std::shared_ptr<_http_client_communicator responseImpl->_prepare_to_receive_data(); } -void _http_client_communicator::send_request_async_impl(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. @@ -152,18 +152,19 @@ void _http_client_communicator::async_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; + bool m_outstanding; }; /// diff --git a/Release/src/http/client/http_client_winhttp.cpp b/Release/src/http/client/http_client_winhttp.cpp index 16434a3750..0ff2b5cf29 100644 --- a/Release/src/http/client/http_client_winhttp.cpp +++ b/Release/src/http/client/http_client_winhttp.cpp @@ -353,6 +353,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) { } @@ -407,6 +408,17 @@ class winhttp_client : public _http_client_communicator // Open session and connection with the server. 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; @@ -577,6 +589,7 @@ class winhttp_client : public _http_client_communicator return report_failure(_XPLATSTR("Error opening connection")); } + m_opened = true; return S_OK; } @@ -584,25 +597,14 @@ class winhttp_client : public _http_client_communicator void send_request(_In_ const std::shared_ptr &request) { // First see if we need to be opened. - if (!m_opened) + unsigned long error = open(); + if (error != 0) { - pplx::extensibility::scoped_critical_section_t l(m_client_lock); - - // Check again with the lock held - if (!m_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; - } - - m_opened = true; - } + // 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; @@ -1590,4 +1592,3 @@ std::shared_ptr<_http_client_communicator> create_platform_final_pipeline_stage( } }}}} -