From 3ccdf3ecb834f04733fddf1120b175d96f03d613 Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Tue, 16 Oct 2018 12:51:04 -0700 Subject: [PATCH 1/2] Fix off-by-one error introduced in connection pool stack. This got introduced when the connection pool queue was changed into a stack to reuse hot connections. --- Release/src/http/client/http_client_asio.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index 645b45466c..2b204afdad 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -353,14 +353,16 @@ class connection_pool_stack std::shared_ptr try_acquire() CPPREST_NOEXCEPT { const size_t oldConnectionsSize = m_connections.size(); - if (m_highWater > oldConnectionsSize) + if (oldConnectionsSize == 0) { - m_highWater = oldConnectionsSize; + m_staleBefore = 0; + return nullptr; } - if (oldConnectionsSize == 0) + const size_t newConnectionsSize = oldConnectionsSize - 1; + if (m_staleBefore > newConnectionsSize) { - return nullptr; + m_staleBefore = newConnectionsSize; } auto result = std::move(m_connections.back()); @@ -376,15 +378,16 @@ class connection_pool_stack bool free_stale_connections() CPPREST_NOEXCEPT { - m_connections.erase(m_connections.begin(), m_connections.begin() + m_highWater); + assert(m_staleBefore <= m_connections.size()); + m_connections.erase(m_connections.begin(), m_connections.begin() + m_staleBefore); const size_t connectionsSize = m_connections.size(); - m_highWater = connectionsSize; + m_staleBefore = connectionsSize; return (connectionsSize != 0); } private: - size_t m_highWater = 0; std::vector> m_connections; + size_t m_staleBefore = 0; }; /// Implements a connection pool with adaptive connection removal From 10921f10743638b3e991f2b4789193b9bdf11958 Mon Sep 17 00:00:00 2001 From: Billy Robert O'Neal III Date: Tue, 16 Oct 2018 13:33:17 -0700 Subject: [PATCH 2/2] Make connection_pool_stack testable and add tests. --- Release/src/CMakeLists.txt | 7 +- Release/src/http/client/http_client_asio.cpp | 48 +------------- .../src/http/common/connection_pool_helpers.h | 66 +++++++++++++++++++ .../functional/http/client/CMakeLists.txt | 5 +- .../http/client/connection_pool_tests.cpp | 45 +++++++++++++ 5 files changed, 120 insertions(+), 51 deletions(-) create mode 100644 Release/src/http/common/connection_pool_helpers.h create mode 100644 Release/tests/functional/http/client/connection_pool_tests.cpp diff --git a/Release/src/CMakeLists.txt b/Release/src/CMakeLists.txt index eceaad7437..cea7048c7d 100644 --- a/Release/src/CMakeLists.txt +++ b/Release/src/CMakeLists.txt @@ -16,12 +16,13 @@ set(SOURCES ${HEADERS_DETAILS} pch/stdafx.h http/client/http_client.cpp - http/client/http_client_msg.cpp http/client/http_client_impl.h - http/common/internal_http_helpers.h + http/client/http_client_msg.cpp + http/common/connection_pool_helpers.h + http/common/http_compression.cpp http/common/http_helpers.cpp http/common/http_msg.cpp - http/common/http_compression.cpp + http/common/internal_http_helpers.h http/listener/http_listener.cpp http/listener/http_listener_msg.cpp http/listener/http_server_api.cpp diff --git a/Release/src/http/client/http_client_asio.cpp b/Release/src/http/client/http_client_asio.cpp index 2b204afdad..ada8e9494d 100644 --- a/Release/src/http/client/http_client_asio.cpp +++ b/Release/src/http/client/http_client_asio.cpp @@ -17,6 +17,7 @@ #include #include "../common/internal_http_helpers.h" +#include "../common/connection_pool_helpers.h" #include "cpprest/asyncrt_utils.h" #if defined(__clang__) @@ -345,51 +346,6 @@ class asio_connection bool m_closed; }; -class connection_pool_stack -{ -public: - // attempts to acquire a connection from the deque; returns nullptr if no connection is - // available - std::shared_ptr try_acquire() CPPREST_NOEXCEPT - { - const size_t oldConnectionsSize = m_connections.size(); - if (oldConnectionsSize == 0) - { - m_staleBefore = 0; - return nullptr; - } - - const size_t newConnectionsSize = oldConnectionsSize - 1; - if (m_staleBefore > newConnectionsSize) - { - m_staleBefore = newConnectionsSize; - } - - auto result = std::move(m_connections.back()); - m_connections.pop_back(); - return result; - } - - // releases `released` back to the connection pool - void release(std::shared_ptr&& released) - { - m_connections.push_back(std::move(released)); - } - - bool free_stale_connections() CPPREST_NOEXCEPT - { - assert(m_staleBefore <= m_connections.size()); - m_connections.erase(m_connections.begin(), m_connections.begin() + m_staleBefore); - const size_t connectionsSize = m_connections.size(); - m_staleBefore = connectionsSize; - return (connectionsSize != 0); - } - -private: - std::vector> m_connections; - size_t m_staleBefore = 0; -}; - /// Implements a connection pool with adaptive connection removal /// /// Every 30 seconds, the lambda in `start_epoch_interval` fires, triggering the @@ -504,7 +460,7 @@ class asio_connection_pool final : public std::enable_shared_from_this m_connections; + std::map> m_connections; bool m_is_timer_running; boost::asio::deadline_timer m_pool_epoch_timer; }; diff --git a/Release/src/http/common/connection_pool_helpers.h b/Release/src/http/common/connection_pool_helpers.h new file mode 100644 index 0000000000..580b82af23 --- /dev/null +++ b/Release/src/http/common/connection_pool_helpers.h @@ -0,0 +1,66 @@ +#pragma once + +#include "cpprest/details/cpprest_compat.h" +#include +#include +#include + +namespace web +{ +namespace http +{ +namespace client +{ +namespace details +{ + +template +class connection_pool_stack +{ +public: + // attempts to acquire a connection from the deque; returns nullptr if no connection is + // available + std::shared_ptr try_acquire() CPPREST_NOEXCEPT + { + const size_t oldConnectionsSize = m_connections.size(); + if (oldConnectionsSize == 0) + { + m_staleBefore = 0; + return nullptr; + } + + auto result = std::move(m_connections.back()); + m_connections.pop_back(); + const size_t newConnectionsSize = m_connections.size(); + if (m_staleBefore > newConnectionsSize) + { + m_staleBefore = newConnectionsSize; + } + + return result; + } + + // releases `released` back to the connection pool + void release(std::shared_ptr&& released) + { + m_connections.push_back(std::move(released)); + } + + bool free_stale_connections() CPPREST_NOEXCEPT + { + assert(m_staleBefore <= m_connections.size()); + m_connections.erase(m_connections.begin(), m_connections.begin() + m_staleBefore); + const size_t connectionsSize = m_connections.size(); + m_staleBefore = connectionsSize; + return (connectionsSize != 0); + } + +private: + std::vector> m_connections; + size_t m_staleBefore = 0; +}; + +} // details +} // client +} // http +} // web diff --git a/Release/tests/functional/http/client/CMakeLists.txt b/Release/tests/functional/http/client/CMakeLists.txt index d92b477481..45f0d9af02 100644 --- a/Release/tests/functional/http/client/CMakeLists.txt +++ b/Release/tests/functional/http/client/CMakeLists.txt @@ -2,8 +2,11 @@ set(SOURCES authentication_tests.cpp building_request_tests.cpp client_construction.cpp + compression_tests.cpp + connection_pool_tests.cpp connections_and_errors.cpp header_tests.cpp + http_client_fuzz_tests.cpp http_client_tests.cpp http_methods_tests.cpp multiple_requests.cpp @@ -20,8 +23,6 @@ set(SOURCES response_stream_tests.cpp status_code_reason_phrase_tests.cpp to_string_tests.cpp - http_client_fuzz_tests.cpp - compression_tests.cpp ) add_casablanca_test(httpclient_test SOURCES) diff --git a/Release/tests/functional/http/client/connection_pool_tests.cpp b/Release/tests/functional/http/client/connection_pool_tests.cpp new file mode 100644 index 0000000000..037ed69d88 --- /dev/null +++ b/Release/tests/functional/http/client/connection_pool_tests.cpp @@ -0,0 +1,45 @@ +#include "stdafx.h" +#include +#include "../../../src/http/common/connection_pool_helpers.h" + +using namespace web::http::client::details; + +SUITE(connection_pooling) { + TEST(empty_returns_nullptr) { + connection_pool_stack connectionStack; + VERIFY_ARE_EQUAL(connectionStack.try_acquire(), std::shared_ptr{}); + } + + static int noisyCount = 0; + struct noisy { + noisy() = delete; + noisy(int) { ++noisyCount; } + noisy(const noisy&) = delete; + noisy(noisy&&) { ++noisyCount; } + noisy& operator=(const noisy&) = delete; + noisy& operator=(noisy&&) = delete; + ~noisy() { --noisyCount; } + }; + + TEST(cycled_connections_survive) { + connection_pool_stack connectionStack; + VERIFY_ARE_EQUAL(0, noisyCount); + connectionStack.release(std::make_shared(42)); + connectionStack.release(std::make_shared(42)); + connectionStack.release(std::make_shared(42)); + VERIFY_ARE_EQUAL(3, noisyCount); + VERIFY_IS_TRUE(connectionStack.free_stale_connections()); + auto tmp = connectionStack.try_acquire(); + VERIFY_ARE_NOT_EQUAL(tmp, std::shared_ptr{}); + connectionStack.release(std::move(tmp)); + VERIFY_ARE_EQUAL(tmp, std::shared_ptr{}); + tmp = connectionStack.try_acquire(); + VERIFY_ARE_NOT_EQUAL(tmp, std::shared_ptr{}); + connectionStack.release(std::move(tmp)); + VERIFY_IS_TRUE(connectionStack.free_stale_connections()); + VERIFY_ARE_EQUAL(1, noisyCount); + VERIFY_IS_FALSE(connectionStack.free_stale_connections()); + VERIFY_ARE_EQUAL(0, noisyCount); + VERIFY_IS_FALSE(connectionStack.free_stale_connections()); + } +};