From c2c218ffe194070069cd0203be1fbcc0094339c3 Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 15 Aug 2024 14:35:46 -0700 Subject: [PATCH 01/26] fhfgh --- .../inc/azure/core/http/curl_transport.hpp | 35 +++++++++++++++++++ .../test/ut/curl_session_test_test.cpp | 7 ++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index d1780a5e12..cd524e34c2 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -26,6 +26,32 @@ namespace Azure { namespace Core { namespace Http { * */ constexpr std::chrono::milliseconds DefaultConnectionTimeout = std::chrono::minutes(5); + + /** + * @brief The configuration options for the keep-alive feature. + * + * @remark The keep-alive feature allows the SDK to reuse the same connection to the service for + * multiple requests. + */ + struct KeepAliveOptions + { + /** + * @brief The time in seconds that the host will allow an idle connection to remain open + * before it is closed. + * + * @remark A connection is idle if no data is sent or received by a host. A host + * may keep an idle connection open for longer than timeout seconds, but the host should + * attempt to retain a connection for at least timeout seconds. + * + */ + std::chrono::seconds ConnectionTimeout; + + /** + * @brief The maximum number of requests that a host will allow over a single connection. + * + */ + std::size_t MaxRequests = (size_t)0; + }; } // namespace _detail /** @@ -151,6 +177,15 @@ namespace Azure { namespace Core { namespace Http { */ bool HttpKeepAlive = true; + /** + * @brief Options specified in the keep-alive request header + * + * @remark The keep-alive feature allows the SDK to reuse the same connection to the service for + * multiple requests. This field is populated if the Keep-Alive header is present in the + * request. + */ + Azure::Nullable<_detail::KeepAliveOptions> KeepAliveOptions; + /** * @brief This option determines whether libcurl verifies the authenticity of the peer's * certificate. diff --git a/sdk/core/azure-core/test/ut/curl_session_test_test.cpp b/sdk/core/azure-core/test/ut/curl_session_test_test.cpp index def4d7a030..7460272f27 100644 --- a/sdk/core/azure-core/test/ut/curl_session_test_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_session_test_test.cpp @@ -38,11 +38,14 @@ namespace Azure { namespace Core { namespace Test { // Simulate a request to be sent Azure::Core::Url url("http://microsoft.com"); Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); - + request.SetHeader("Keep-alive", "timeout=5, max=10"); // Move the curlMock to build a session and then send the request // The session will get the response we mock before, so it will pass for this GET Azure::Core::Http::CurlTransportOptions transportOptions; - transportOptions.HttpKeepAlive = true; + // transportOptions.HttpKeepAlive = true; + transportOptions.KeepAliveOptions + = Azure::Core::Http::_detail::KeepAliveOptions{std::chrono::seconds(5), 10}; + auto session = std::make_unique( request, std::move(uniqueCurlMock), transportOptions); From ceab41fd0b2a681185a4203166d8ad091cdeaaac Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 11:10:13 -0700 Subject: [PATCH 02/26] first stab at the code --- .../inc/azure/core/http/curl_transport.hpp | 9 +-- sdk/core/azure-core/src/http/curl/curl.cpp | 72 +++++++++++++++++-- .../src/http/curl/curl_connection_private.hpp | 34 +++++++++ 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index cd524e34c2..1b0b513998 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -13,6 +13,7 @@ #include "azure/core/nullable.hpp" #include +#include #include #include @@ -33,8 +34,8 @@ namespace Azure { namespace Core { namespace Http { * @remark The keep-alive feature allows the SDK to reuse the same connection to the service for * multiple requests. */ - struct KeepAliveOptions - { + class KeepAliveOptions { + public: /** * @brief The time in seconds that the host will allow an idle connection to remain open * before it is closed. @@ -44,7 +45,7 @@ namespace Azure { namespace Core { namespace Http { * attempt to retain a connection for at least timeout seconds. * */ - std::chrono::seconds ConnectionTimeout; + std::chrono::seconds ConnectionTimeout = std::chrono::seconds(0); /** * @brief The maximum number of requests that a host will allow over a single connection. @@ -184,7 +185,7 @@ namespace Azure { namespace Core { namespace Http { * multiple requests. This field is populated if the Keep-Alive header is present in the * request. */ - Azure::Nullable<_detail::KeepAliveOptions> KeepAliveOptions; + Azure::Nullable KeepAliveOptions; /** * @brief This option determines whether libcurl verifies the authenticity of the peer's diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 176e612472..d227c5e729 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -2220,10 +2220,17 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo { g_curlConnectionPool.ConnectionPoolIndex.erase(hostPoolIndex); } - - Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Re-using connection from the pool."); - // return connection ref - return connection; + if (!connection->IsExpired()) + { + connection->IncreaseUsageCount(); + Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Re-using connection from the pool."); + // return connection ref + return connection; + } + else + { + Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Connection expired. Discarding."); + } } } } @@ -2235,6 +2242,45 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo return std::make_unique(request, options, hostDisplayName, connectionKey); } +Azure::Core::Http::_detail::KeepAliveOptions CurlConnection::ParseKeepAliveHeader( + std::string const& keepAlive) +{ + Azure::Core::Http::_detail::KeepAliveOptions keepAliveOptions; + // Parse the Keep-Alive header to determine if the connection should be kept alive. + // The Keep-Alive header is in the format of: + // Keep-Alive: timeout=5, max=1000 + // The timeout is the number of seconds the connection is allowed to be idle before it is closed. + // The max is the maximum number of requests that can be made on the connection before it is + // closed. + // If the header is not present, the connection will be kept alive for the lifetime of the + // application. + std::string const timeoutKey = "timeout="; + std::string const maxKey = "max="; + auto timeoutPos = keepAlive.find(timeoutKey); + auto maxPos = keepAlive.find(maxKey); + if (timeoutPos != std::string::npos) + { + auto timeoutEnd = keepAlive.find(',', timeoutPos); + if (timeoutEnd == std::string::npos) + { + timeoutEnd = keepAlive.size(); + } + keepAliveOptions.ConnectionTimeout = std::chrono::seconds(std::stoi(keepAlive.substr( + timeoutPos + timeoutKey.size(), timeoutEnd - timeoutPos - timeoutKey.size()))); + } + if (maxPos != std::string::npos) + { + auto maxEnd = keepAlive.find(',', maxPos); + if (maxEnd == std::string::npos) + { + maxEnd = keepAlive.size(); + } + keepAliveOptions.MaxRequests + = std::stoi(keepAlive.substr(maxPos + maxKey.size(), maxEnd - maxPos - maxKey.size())); + } + return keepAliveOptions; +} + // Move the connection back to the connection pool. Push it to the front so it becomes the // first connection to be picked next time some one ask for a connection to the pool (LIFO) void CurlConnectionPool::MoveConnectionBackToPool( @@ -2246,9 +2292,9 @@ void CurlConnectionPool::MoveConnectionBackToPool( return; // The server has asked us to not re-use this connection. } - if (connection->IsShutdown()) + if (connection->IsShutdown() || connection->IsExpired()) { - // Can't re-used a shut down connection + // Can't re-used a shut down connection or an expired connection return; } @@ -2309,8 +2355,22 @@ CurlConnection::CurlConnection( _detail::DefaultFailedToGetNewConnectionTemplate + hostDisplayName + ". " + std::string("curl_easy_init returned Null")); } + CURLcode result; + if (request.GetHeader("connection").HasValue() && request.GetHeader("keep-alive").HasValue()) + { + auto connectionHeader = Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeader("connection").Value()); + auto keepAliveHeader = Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeader("keep-alive").Value()); + + if (connectionHeader == "keep-alive") + { + m_keepAliveOptions = ParseKeepAliveHeader(keepAliveHeader); + } + } + if (options.EnableCurlTracing) { if (!SetLibcurlOption( diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 7c360d3fbc..7ebcb06971 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -134,6 +134,20 @@ namespace Azure { namespace Core { * @return `true` is the connection was shut it down; otherwise, `false`. */ bool IsShutdown() const { return m_isShutDown; } + + size_t m_usedCount = 0; + /** + * @brief Increase the usage count. + * + */ + void IncreaseUsageCount() { m_usedCount++; }; + + /** + * @brief Get the connection usage count. + * + * @return The usage count + */ + const size_t GetUsageCount() { return m_usedCount; } }; /** @@ -145,6 +159,7 @@ namespace Azure { namespace Core { Azure::Core::_internal::UniqueHandle m_handle; curl_socket_t m_curlSocket; std::chrono::steady_clock::time_point m_lastUseTime; + std::chrono::steady_clock::time_point m_firstUseTime = std::chrono::steady_clock::now(); std::string m_connectionKey; // CRL validation is disabled by default to be consistent with WinHTTP behavior bool m_enableCrlValidation{false}; @@ -161,6 +176,8 @@ namespace Azure { namespace Core { static int CurlSslCtxCallback(CURL* curl, void* sslctx, void* parm); int SslCtxCallback(CURL* curl, void* sslctx); int VerifyCertificateError(int ok, X509_STORE_CTX* storeContext); + Azure::Core::Http::_detail::KeepAliveOptions ParseKeepAliveHeader(std::string const& keepAlive); + Azure::Nullable m_keepAliveOptions; public: /** @@ -201,6 +218,23 @@ namespace Azure { namespace Core { */ bool IsExpired() override { + // if we have keep alive options and we haven reached the max requests declare expired + if (m_keepAliveOptions.HasValue() && m_keepAliveOptions.Value().MaxRequests > 0 + && m_keepAliveOptions.Value().MaxRequests > m_usedCount) + { + return true; + } + + // if we have keep alive options and we have a connection timeout and the connection time + // frame has passed declare expired + if (m_keepAliveOptions.HasValue() + && m_keepAliveOptions.Value().ConnectionTimeout > std::chrono::seconds(0) + && m_firstUseTime + m_keepAliveOptions.Value().ConnectionTimeout + < std::chrono::steady_clock::now()) + { + return true; + } + auto connectionOnWaitingTimeMs = std::chrono::duration_cast( std::chrono::steady_clock::now() - this->m_lastUseTime); return connectionOnWaitingTimeMs.count() >= _detail::DefaultConnectionExpiredMilliseconds; From db82fc7a1cf5692db3b6d7223ef3b0c799421edb Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 11:12:34 -0700 Subject: [PATCH 03/26] dfsdfsd --- sdk/core/azure-core/test/ut/curl_session_test_test.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/sdk/core/azure-core/test/ut/curl_session_test_test.cpp b/sdk/core/azure-core/test/ut/curl_session_test_test.cpp index 7460272f27..5ae4f4178d 100644 --- a/sdk/core/azure-core/test/ut/curl_session_test_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_session_test_test.cpp @@ -38,14 +38,11 @@ namespace Azure { namespace Core { namespace Test { // Simulate a request to be sent Azure::Core::Url url("http://microsoft.com"); Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); - request.SetHeader("Keep-alive", "timeout=5, max=10"); // Move the curlMock to build a session and then send the request // The session will get the response we mock before, so it will pass for this GET Azure::Core::Http::CurlTransportOptions transportOptions; - // transportOptions.HttpKeepAlive = true; - transportOptions.KeepAliveOptions - = Azure::Core::Http::_detail::KeepAliveOptions{std::chrono::seconds(5), 10}; - + transportOptions.HttpKeepAlive = true; + auto session = std::make_unique( request, std::move(uniqueCurlMock), transportOptions); From fdf3046394afa79e62217d195c07f414a84b759d Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 11:13:12 -0700 Subject: [PATCH 04/26] erwrwe --- sdk/core/azure-core/test/ut/curl_session_test_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/test/ut/curl_session_test_test.cpp b/sdk/core/azure-core/test/ut/curl_session_test_test.cpp index 5ae4f4178d..def4d7a030 100644 --- a/sdk/core/azure-core/test/ut/curl_session_test_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_session_test_test.cpp @@ -38,11 +38,11 @@ namespace Azure { namespace Core { namespace Test { // Simulate a request to be sent Azure::Core::Url url("http://microsoft.com"); Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); + // Move the curlMock to build a session and then send the request // The session will get the response we mock before, so it will pass for this GET Azure::Core::Http::CurlTransportOptions transportOptions; transportOptions.HttpKeepAlive = true; - auto session = std::make_unique( request, std::move(uniqueCurlMock), transportOptions); From c6f1a61c2c76f32576147bdc169ce18e903f8eb1 Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 11:23:26 -0700 Subject: [PATCH 05/26] ooooooof --- sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index 1b0b513998..f30d49c697 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -51,7 +51,7 @@ namespace Azure { namespace Core { namespace Http { * @brief The maximum number of requests that a host will allow over a single connection. * */ - std::size_t MaxRequests = (size_t)0; + std::size_t MaxRequests = size_t(0); }; } // namespace _detail From 68fd7b9cd68f3d1c4cc6ff378a681e527b56ece7 Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 11:32:55 -0700 Subject: [PATCH 06/26] clang ans bb --- sdk/core/azure-core/src/http/curl/curl.cpp | 5 +++-- .../azure-core/src/http/curl/curl_connection_private.hpp | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index d227c5e729..9b1d9cefe1 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -2220,6 +2220,7 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo { g_curlConnectionPool.ConnectionPoolIndex.erase(hostPoolIndex); } + // if the connection is expired do not return it, let the code flow and return a new one. if (!connection->IsExpired()) { connection->IncreaseUsageCount(); @@ -2230,7 +2231,7 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo else { Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Connection expired. Discarding."); - } + } } } } @@ -2355,7 +2356,7 @@ CurlConnection::CurlConnection( _detail::DefaultFailedToGetNewConnectionTemplate + hostDisplayName + ". " + std::string("curl_easy_init returned Null")); } - + CURLcode result; if (request.GetHeader("connection").HasValue() && request.GetHeader("keep-alive").HasValue()) diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 7ebcb06971..d5a6a60fe4 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -147,7 +147,7 @@ namespace Azure { namespace Core { * * @return The usage count */ - const size_t GetUsageCount() { return m_usedCount; } + size_t GetUsageCount() const { return m_usedCount; } }; /** @@ -176,7 +176,8 @@ namespace Azure { namespace Core { static int CurlSslCtxCallback(CURL* curl, void* sslctx, void* parm); int SslCtxCallback(CURL* curl, void* sslctx); int VerifyCertificateError(int ok, X509_STORE_CTX* storeContext); - Azure::Core::Http::_detail::KeepAliveOptions ParseKeepAliveHeader(std::string const& keepAlive); + Azure::Core::Http::_detail::KeepAliveOptions ParseKeepAliveHeader( + std::string const& keepAlive); Azure::Nullable m_keepAliveOptions; public: From 873e6f4bdec91e634c8dcd9c7794b96270d55b6f Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 13:24:43 -0700 Subject: [PATCH 07/26] parse header tests --- sdk/core/azure-core/src/http/curl/curl.cpp | 52 ++++-- .../src/http/curl/curl_connection_private.hpp | 8 +- sdk/core/azure-core/test/ut/CMakeLists.txt | 2 +- .../test/ut/curl_connection_tests.cpp | 175 ++++++++++++++++++ 4 files changed, 218 insertions(+), 19 deletions(-) create mode 100644 sdk/core/azure-core/test/ut/curl_connection_tests.cpp diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 9b1d9cefe1..a77739d7e9 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -2246,7 +2246,6 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo Azure::Core::Http::_detail::KeepAliveOptions CurlConnection::ParseKeepAliveHeader( std::string const& keepAlive) { - Azure::Core::Http::_detail::KeepAliveOptions keepAliveOptions; // Parse the Keep-Alive header to determine if the connection should be kept alive. // The Keep-Alive header is in the format of: // Keep-Alive: timeout=5, max=1000 @@ -2255,29 +2254,43 @@ Azure::Core::Http::_detail::KeepAliveOptions CurlConnection::ParseKeepAliveHeade // closed. // If the header is not present, the connection will be kept alive for the lifetime of the // application. + Azure::Core::Http::_detail::KeepAliveOptions keepAliveOptions; std::string const timeoutKey = "timeout="; std::string const maxKey = "max="; auto timeoutPos = keepAlive.find(timeoutKey); auto maxPos = keepAlive.find(maxKey); - if (timeoutPos != std::string::npos) + try { - auto timeoutEnd = keepAlive.find(',', timeoutPos); - if (timeoutEnd == std::string::npos) + + if (timeoutPos != std::string::npos) { - timeoutEnd = keepAlive.size(); + auto timeoutEnd = keepAlive.find(',', timeoutPos); + if (timeoutEnd == std::string::npos) + { + timeoutEnd = keepAlive.size(); + } + + keepAliveOptions.ConnectionTimeout = std::chrono::seconds(std::stoi(keepAlive.substr( + timeoutPos + timeoutKey.size(), timeoutEnd - timeoutPos - timeoutKey.size()))); } - keepAliveOptions.ConnectionTimeout = std::chrono::seconds(std::stoi(keepAlive.substr( - timeoutPos + timeoutKey.size(), timeoutEnd - timeoutPos - timeoutKey.size()))); - } - if (maxPos != std::string::npos) - { - auto maxEnd = keepAlive.find(',', maxPos); - if (maxEnd == std::string::npos) + + if (maxPos != std::string::npos) { - maxEnd = keepAlive.size(); + auto maxEnd = keepAlive.find(',', maxPos); + if (maxEnd == std::string::npos) + { + maxEnd = keepAlive.size(); + } + keepAliveOptions.MaxRequests + = std::stoi(keepAlive.substr(maxPos + maxKey.size(), maxEnd - maxPos - maxKey.size())); } - keepAliveOptions.MaxRequests - = std::stoi(keepAlive.substr(maxPos + maxKey.size(), maxEnd - maxPos - maxKey.size())); + } + catch (std::invalid_argument const&) + { + Log::Write( + Logger::Level::Error, + "Failed to parse max value / timeout from Keep-Alive header: " + keepAlive); + return Azure::Core::Http::_detail::KeepAliveOptions(); } return keepAliveOptions; } @@ -2368,7 +2381,14 @@ CurlConnection::CurlConnection( if (connectionHeader == "keep-alive") { - m_keepAliveOptions = ParseKeepAliveHeader(keepAliveHeader); + auto keepAliveValue = ParseKeepAliveHeader(keepAliveHeader); + // if we have issues parsing this header , or the data in the header is invalid no point in + // setting it up + if (keepAliveValue.ConnectionTimeout != std::chrono::seconds(0) + || keepAliveValue.MaxRequests > 0) + { + m_keepAliveOptions = keepAliveValue; + } } } diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index d5a6a60fe4..3a125042d2 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -29,7 +29,9 @@ /// From openssl/x509.h. Avoids needing to include openssl headers typedef struct x509_store_ctx_st X509_STORE_CTX; - +namespace Azure { namespace Core { namespace Test { + class CurlConnectionTest_ParseKeepAliveHeader_Test; +}}} // namespace Azure::Core::Test namespace Azure { namespace Core { namespace _detail { /** @@ -155,6 +157,8 @@ namespace Azure { namespace Core { * */ class CurlConnection final : public CurlNetworkConnection { + friend class Azure::Core::Test::CurlConnectionTest_ParseKeepAliveHeader_Test; + private: Azure::Core::_internal::UniqueHandle m_handle; curl_socket_t m_curlSocket; @@ -176,9 +180,9 @@ namespace Azure { namespace Core { static int CurlSslCtxCallback(CURL* curl, void* sslctx, void* parm); int SslCtxCallback(CURL* curl, void* sslctx); int VerifyCertificateError(int ok, X509_STORE_CTX* storeContext); + Azure::Nullable m_keepAliveOptions; Azure::Core::Http::_detail::KeepAliveOptions ParseKeepAliveHeader( std::string const& keepAlive); - Azure::Nullable m_keepAliveOptions; public: /** diff --git a/sdk/core/azure-core/test/ut/CMakeLists.txt b/sdk/core/azure-core/test/ut/CMakeLists.txt index 9dbcfec817..7c9885ab29 100644 --- a/sdk/core/azure-core/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core/test/ut/CMakeLists.txt @@ -30,7 +30,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED True) if(BUILD_TRANSPORT_CURL) SET(CURL_OPTIONS_TESTS curl_options_test.cpp) SET(CURL_SESSION_TESTS curl_session_test_test.cpp curl_session_test.hpp) - SET(CURL_CONNECTION_POOL_TESTS curl_connection_pool_test.cpp) + SET(CURL_CONNECTION_POOL_TESTS curl_connection_pool_test.cpp curl_connection_tests.cpp) endif() if(RUN_LONG_UNIT_TESTS) diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp new file mode 100644 index 0000000000..5f01329512 --- /dev/null +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -0,0 +1,175 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** + * @file + * @brief The base class for testing a curl session. + * + * @remark The curl connection mock is defined here. + * + */ +#include "transport_adapter_base_test.hpp" + +#include +#include +#include + +#if defined(BUILD_CURL_HTTP_TRANSPORT_ADAPTER) +#include "azure/core/http/curl_transport.hpp" + +#ifdef _MSC_VER +#pragma warning(push) +#elif defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) // !_MSC_VER +#pragma GCC diagnostic push +#elif defined(__clang__) // !_MSC_VER !__clang__ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wgnu-zero-variadic-macro-arguments" +#endif // _MSC_VER + +#include + +#include + +#include +#include + +namespace Azure { namespace Core { namespace Test { + + class CurlConnectionTest : public ::testing::Test {}; + + TEST(CurlConnectionTest, ParseKeepAliveHeader) + { + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); + + Azure::Core::Http::CurlConnection curlConnection( + req, Azure::Core::Http::CurlTransportOptions(), "hostName", "propKey"); + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=10"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("max=10, timeout=5"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5,max=10"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("max=10,timeout=5"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("max=10"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader(""); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=10, extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=10, extra=1,"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=10, extra=1, "); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader(" max=10, extra=1,"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(10)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader(", , extra=1, "); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=, extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("max=, extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("max= , extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=, max=10, extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=, extra=1,"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max= , extra=1, "); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max=10, extra=1"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max= , extra=1,"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max= , extra=1, "); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5 max= , extra=1,"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max= 10 extra=1, "); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + } +}}} // namespace Azure::Core::Test + +#endif \ No newline at end of file From 5090b1330ea37666e5a423350f2317f4b179bcee Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 14:13:02 -0700 Subject: [PATCH 08/26] is expired tests --- .../src/http/curl/curl_connection_private.hpp | 21 +++++++-- .../test/ut/curl_connection_tests.cpp | 44 +++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 3a125042d2..62499be759 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -137,19 +137,18 @@ namespace Azure { namespace Core { */ bool IsShutdown() const { return m_isShutDown; } - size_t m_usedCount = 0; /** * @brief Increase the usage count. * */ - void IncreaseUsageCount() { m_usedCount++; }; + virtual void IncreaseUsageCount(){}; /** * @brief Get the connection usage count. * * @return The usage count */ - size_t GetUsageCount() const { return m_usedCount; } + virtual size_t GetUsageCount() const { return 0; }; }; /** @@ -169,6 +168,7 @@ namespace Azure { namespace Core { bool m_enableCrlValidation{false}; // Allow the connection to proceed if retrieving the CRL failed. bool m_allowFailedCrlRetrieval{true}; + size_t m_usedCount = size_t(0); static int CurlLoggingCallback( CURL* handle, @@ -225,7 +225,7 @@ namespace Azure { namespace Core { { // if we have keep alive options and we haven reached the max requests declare expired if (m_keepAliveOptions.HasValue() && m_keepAliveOptions.Value().MaxRequests > 0 - && m_keepAliveOptions.Value().MaxRequests > m_usedCount) + && m_keepAliveOptions.Value().MaxRequests <= m_usedCount) { return true; } @@ -270,6 +270,19 @@ namespace Azure { namespace Core { */ CURLcode SendBuffer(uint8_t const* buffer, size_t bufferSize, Context const& context) override; + + /** + * @brief Increase the usage count. + * + */ + void IncreaseUsageCount() override { m_usedCount++; }; + + /** + * @brief Get the connection usage count. + * + * @return The usage count + */ + size_t GetUsageCount() const override { return m_usedCount; } }; } // namespace Http }} // namespace Azure::Core diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp index 5f01329512..662159de21 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -28,7 +28,9 @@ #include +#include #include +#include #include #include @@ -170,6 +172,48 @@ namespace Azure { namespace Core { namespace Test { EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } } + + TEST(CurlConnectionTest, IsExpiredNot) + { + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); + req.SetHeader("Connection", "keep-alive"); + req.SetHeader("Keep-Alive", "timeout=120, max=2"); + Azure::Core::Http::CurlConnection curlConnection( + req, Azure::Core::Http::CurlTransportOptions(), "hostName", "propKey"); + curlConnection.UpdateLastUsageTime(); + EXPECT_TRUE(!curlConnection.IsExpired()); + } + + TEST(CurlConnectionTest, IsExpiredMaxUsage) + { + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); + req.SetHeader("Connection", "keep-alive"); + req.SetHeader("Keep-Alive", "timeout=120, max=2"); + Azure::Core::Http::CurlConnection curlConnection( + req, Azure::Core::Http::CurlTransportOptions(), "hostName", "propKey"); + curlConnection.IncreaseUsageCount(); + curlConnection.IncreaseUsageCount(); // usage == max + curlConnection.UpdateLastUsageTime(); + EXPECT_TRUE(curlConnection.IsExpired()); + curlConnection.IncreaseUsageCount(); + curlConnection.IncreaseUsageCount(); // usage > max + EXPECT_TRUE(curlConnection.IsExpired()); + } + + TEST(CurlConnectionTest, IsExpiredTimeout) + { + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); + req.SetHeader("Connection", "keep-alive"); + req.SetHeader("Keep-Alive", "timeout=1, max=2"); + Azure::Core::Http::CurlConnection curlConnection( + req, Azure::Core::Http::CurlTransportOptions(), "hostName", "propKey"); + std::this_thread::sleep_for(std::chrono::milliseconds(1100)); + curlConnection.UpdateLastUsageTime(); + EXPECT_TRUE(curlConnection.IsExpired()); + } }}} // namespace Azure::Core::Test #endif \ No newline at end of file From 4e90914649b4ca7d16a8dcf9048c2d8c55f3045f Mon Sep 17 00:00:00 2001 From: George Arama Date: Tue, 20 Aug 2024 14:21:51 -0700 Subject: [PATCH 09/26] two more tests --- sdk/core/azure-core/test/ut/curl_connection_tests.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp index 662159de21..b5db03aa6e 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -171,6 +171,16 @@ namespace Azure { namespace Core { namespace Test { EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=x, max=10"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } + { + auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=n"); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); + } } TEST(CurlConnectionTest, IsExpiredNot) From 1030454832c4dcb654760af84dc633fc00589ecc Mon Sep 17 00:00:00 2001 From: George Arama Date: Wed, 21 Aug 2024 11:00:10 -0700 Subject: [PATCH 10/26] break the expire logic --- sdk/core/azure-core/src/http/curl/curl.cpp | 4 ++-- .../src/http/curl/curl_connection_private.hpp | 21 ++++++++++++++++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index a77739d7e9..d9bc2b7d90 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -2221,7 +2221,7 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo g_curlConnectionPool.ConnectionPoolIndex.erase(hostPoolIndex); } // if the connection is expired do not return it, let the code flow and return a new one. - if (!connection->IsExpired()) + if (!connection->IsKeepAliveExpired()) { connection->IncreaseUsageCount(); Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Re-using connection from the pool."); @@ -2306,7 +2306,7 @@ void CurlConnectionPool::MoveConnectionBackToPool( return; // The server has asked us to not re-use this connection. } - if (connection->IsShutdown() || connection->IsExpired()) + if (connection->IsShutdown() || connection->IsKeepAliveExpired()) { // Can't re-used a shut down connection or an expired connection return; diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 62499be759..7730895099 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -107,6 +107,12 @@ namespace Azure { namespace Core { */ virtual bool IsExpired() = 0; + /** + * @brief Checks whether this CURL connection is expired due to keep-alive settings. + * + */ + virtual bool IsKeepAliveExpired() { return false; }; + /** * @brief This function is used when working with streams to pull more data from the wire. * Function will try to keep pulling data from socket until the buffer is all written or until @@ -222,6 +228,17 @@ namespace Azure { namespace Core { * @return `true` if this connection is considered expired; otherwise, `false`. */ bool IsExpired() override + { + auto connectionOnWaitingTimeMs = std::chrono::duration_cast( + std::chrono::steady_clock::now() - this->m_lastUseTime); + return connectionOnWaitingTimeMs.count() >= _detail::DefaultConnectionExpiredMilliseconds || IsKeepAliveExpired(); + } + + /** + * @brief Checks whether this CURL connection is expired due to keep-alive settings. + * @return `true` if this connection is considered expired; otherwise, `false`. + */ + bool IsKeepAliveExpired() override { // if we have keep alive options and we haven reached the max requests declare expired if (m_keepAliveOptions.HasValue() && m_keepAliveOptions.Value().MaxRequests > 0 @@ -240,9 +257,7 @@ namespace Azure { namespace Core { return true; } - auto connectionOnWaitingTimeMs = std::chrono::duration_cast( - std::chrono::steady_clock::now() - this->m_lastUseTime); - return connectionOnWaitingTimeMs.count() >= _detail::DefaultConnectionExpiredMilliseconds; + return false; } /** From 3ccd4c58a5a07aaea0c339046fa92a55b29217bd Mon Sep 17 00:00:00 2001 From: George Arama Date: Wed, 21 Aug 2024 14:00:56 -0700 Subject: [PATCH 11/26] clangs11 --- sdk/core/azure-core/src/http/curl/curl_connection_private.hpp | 3 ++- sdk/core/azure-core/test/ut/curl_connection_tests.cpp | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 7730895099..64ec0b4986 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -231,7 +231,8 @@ namespace Azure { namespace Core { { auto connectionOnWaitingTimeMs = std::chrono::duration_cast( std::chrono::steady_clock::now() - this->m_lastUseTime); - return connectionOnWaitingTimeMs.count() >= _detail::DefaultConnectionExpiredMilliseconds || IsKeepAliveExpired(); + return connectionOnWaitingTimeMs.count() >= _detail::DefaultConnectionExpiredMilliseconds + || IsKeepAliveExpired(); } /** diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp index b5db03aa6e..5e86ad9dcc 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -37,7 +37,8 @@ namespace Azure { namespace Core { namespace Test { - class CurlConnectionTest : public ::testing::Test {}; + class CurlConnectionTest : public ::testing::Test { + }; TEST(CurlConnectionTest, ParseKeepAliveHeader) { From bbfbcd7a505c28f622500700963ae9085924e8c2 Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 11:21:30 -0700 Subject: [PATCH 12/26] connection thingy test --- .../src/http/curl/curl_connection_private.hpp | 2 +- .../test/ut/curl_connection_pool_test.cpp | 1169 +++++++++-------- 2 files changed, 651 insertions(+), 520 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 64ec0b4986..7f1b2ab6b9 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -174,7 +174,7 @@ namespace Azure { namespace Core { bool m_enableCrlValidation{false}; // Allow the connection to proceed if retrieving the CRL failed. bool m_allowFailedCrlRetrieval{true}; - size_t m_usedCount = size_t(0); + size_t m_usedCount = size_t(1); static int CurlLoggingCallback( CURL* handle, diff --git a/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp b/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp index 61ecc10046..b51b85be47 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp @@ -39,636 +39,767 @@ inline std::string CreateConnectionKey( namespace Azure { namespace Core { namespace Test { -/*********************** Unique Tests for Libcurl ********************************/ + /*********************** Unique Tests for Libcurl ********************************/ #if defined(BUILD_CURL_HTTP_TRANSPORT_ADAPTER) - TEST(CurlConnectionPool, connectionPoolTest) + TEST(CurlConnectionPool, connectionPoolTest) + { { - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); - // Make sure there are nothing in the pool - EXPECT_EQ(CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.size(), 0); - } + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + // Make sure there are nothing in the pool + EXPECT_EQ(CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.size(), 0); + } - // Use the same request for all connections. - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); - std::string const expectedConnectionKey(CreateConnectionKey( - AzureSdkHttpbinServer::Schema(), - AzureSdkHttpbinServer::Host(), - ",0,0,0,0,0,1,1,0,0,0,0")); + // Use the same request for all connections. + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), AzureSdkHttpbinServer::Host(), ",0,0,0,0,0,1,1,0,0,0,0")); - { - // Creating a new connection with default options - Azure::Core::Http::CurlTransportOptions options; - auto connection - = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + { + // Creating a new connection with default options + Azure::Core::Http::CurlTransportOptions options; + auto connection + = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); - EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); - auto session - = std::make_unique(req, std::move(connection), options); - // Simulate connection was used already - session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; - session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; - session->m_httpKeepAlive = true; - } - // Check that after the connection is gone, it is moved back to the pool - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .size(), - 1); - auto connectionFromPool - = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.begin() - ->second.begin() - ->get(); - EXPECT_EQ(connectionFromPool->GetConnectionKey(), expectedConnectionKey); - } + auto session + = std::make_unique(req, std::move(connection), options); + // Simulate connection was used already + session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; + session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; + session->m_httpKeepAlive = true; + } + // Check that after the connection is gone, it is moved back to the pool + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 1); + auto connectionFromPool + = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .begin() + ->second.begin() + ->get(); + EXPECT_EQ(connectionFromPool->GetConnectionKey(), expectedConnectionKey); + } - // Test that asking a connection with same config will re-use the same connection - { - // Creating a new connection with default options - Azure::Core::Http::CurlTransportOptions options; - auto connection - = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + // Test that asking a connection with same config will re-use the same connection + { + // Creating a new connection with default options + Azure::Core::Http::CurlTransportOptions options; + auto connection + = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); - // There was just one connection in the pool, it should be empty now - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .size(), - 0); - // And the connection key for the connection we got is the expected - EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // There was just one connection in the pool, it should be empty now + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 0); + // And the connection key for the connection we got is the expected + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + + auto session + = std::make_unique(req, std::move(connection), options); + // Simulate connection was used already + session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; + session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; + session->m_httpKeepAlive = true; + } + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // Check that after the connection is gone, it is moved back to the pool + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 1); + auto values = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ConnectionPoolIndex.begin(); + EXPECT_EQ(values->second.size(), 1); + EXPECT_EQ(values->second.begin()->get()->GetConnectionKey(), expectedConnectionKey); + } - auto session - = std::make_unique(req, std::move(connection), options); - // Simulate connection was used already - session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; - session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; - session->m_httpKeepAlive = true; - } - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // Check that after the connection is gone, it is moved back to the pool - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .size(), - 1); - auto values = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.begin(); - EXPECT_EQ(values->second.size(), 1); - EXPECT_EQ(values->second.begin()->get()->GetConnectionKey(), expectedConnectionKey); - } + // Now test that using a different connection config won't re-use the same connection + std::string const secondExpectedKey = AzureSdkHttpbinServer::Schema() + "://" + + AzureSdkHttpbinServer::Host() + ",0,0,0,0,0,1,0,0,0,0,200000"; + { + // Creating a new connection with options + Azure::Core::Http::CurlTransportOptions options; + options.SslVerifyPeer = false; + options.ConnectionTimeout = std::chrono::seconds(200); + auto connection + = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + EXPECT_EQ(connection->GetConnectionKey(), secondExpectedKey); + // One connection still in the pool after getting a new connection and with first expected + // key + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 1); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .begin() + ->second.begin() + ->get() + ->GetConnectionKey(), + expectedConnectionKey); + + auto session + = std::make_unique(req, std::move(connection), options); + // Simulate connection was used already + session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; + session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; + session->m_httpKeepAlive = true; + } - // Now test that using a different connection config won't re-use the same connection - std::string const secondExpectedKey = AzureSdkHttpbinServer::Schema() + "://" - + AzureSdkHttpbinServer::Host() + ",0,0,0,0,0,1,0,0,0,0,200000"; + // Now there should be 2 index wit one connection each + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 2); + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + for (auto& val : + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex) { - // Creating a new connection with options - Azure::Core::Http::CurlTransportOptions options; - options.SslVerifyPeer = false; - options.ConnectionTimeout = std::chrono::seconds(200); - auto connection - = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); - EXPECT_EQ(connection->GetConnectionKey(), secondExpectedKey); - // One connection still in the pool after getting a new connection and with first expected - // key - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .size(), - 1); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .begin() - ->second.begin() - ->get() - ->GetConnectionKey(), - expectedConnectionKey); - - auto session - = std::make_unique(req, std::move(connection), options); - // Simulate connection was used already - session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; - session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; - session->m_httpKeepAlive = true; + EXPECT_EQ(val.second.size(), 1); } + // The connection pool should have the two connections we added earlier. + EXPECT_NE( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .end(), + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .find(expectedConnectionKey)); + EXPECT_NE( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .end(), + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .find(secondExpectedKey)); + } - // Now there should be 2 index wit one connection each + { + Azure::Core::Http::CurlSession::ResponseBufferParser responseParser; + EXPECT_EQ(responseParser.ExtractResponse(), nullptr); + + const uint8_t responseBuf[] = "HTTP/1.1 200 OK\r\n\r\n"; + static_cast(responseParser.Parse(responseBuf, sizeof(responseBuf) - 1)); + EXPECT_NE(responseParser.ExtractResponse(), nullptr); + EXPECT_EQ(responseParser.ExtractResponse(), nullptr); + } + + // Test re-using same custom config + { + // Creating a new connection with default options + Azure::Core::Http::CurlTransportOptions options; + auto connection + = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // One connection still in the pool after getting a new connection and with first expected + // key EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), - 2); + 1); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .begin() + ->second.begin() + ->get() + ->GetConnectionKey(), + secondExpectedKey); + + auto session + = std::make_unique(req, std::move(connection), options); + // Simulate connection was used already + session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; + session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; + session->m_httpKeepAlive = true; + } + // Now there should be 2 index wit one connection each + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 2); + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + for (auto& val : + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex) { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - for (auto& val : Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex) - { - EXPECT_EQ(val.second.size(), 1); - } - // The connection pool should have the two connections we added earlier. - EXPECT_NE( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .end(), - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .find(expectedConnectionKey)); - EXPECT_NE( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .end(), - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .find(secondExpectedKey)); + EXPECT_EQ(val.second.size(), 1); } + // The connection pool should have the two connections we added earlier. + EXPECT_NE( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .end(), + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .find(expectedConnectionKey)); + EXPECT_NE( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .end(), + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .find(secondExpectedKey)); + } + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // clean the pool + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + } - { - Azure::Core::Http::CurlSession::ResponseBufferParser responseParser; - EXPECT_EQ(responseParser.ExtractResponse(), nullptr); +#ifdef RUN_LONG_UNIT_TESTS + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // clean the pool + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 0); + } - const uint8_t responseBuf[] = "HTTP/1.1 200 OK\r\n\r\n"; - static_cast(responseParser.Parse(responseBuf, sizeof(responseBuf) - 1)); - EXPECT_NE(responseParser.ExtractResponse(), nullptr); - EXPECT_EQ(responseParser.ExtractResponse(), nullptr); + // Test pool clean routine. + std::cout << "Running Connection Pool Cleaner Test. This test can take up to 2 minutes to " + "complete." + << std::endl + << "Add compiler option -DRUN_LONG_UNIT_TESTS=OFF when building if you want to " + "skip this test." + << std::endl; + { + // Make sure the clean pool thread is started by adding 5 connections to the pool + std::vector> connections; + for (int count = 0; count < 5; count++) + { + connections.emplace_back( + CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, {})); } - - // Test re-using same custom config + for (int count = 0; count < 5; count++) { - // Creating a new connection with default options - Azure::Core::Http::CurlTransportOptions options; - auto connection - = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); - EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); - // One connection still in the pool after getting a new connection and with first expected - // key - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .size(), - 1); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .begin() - ->second.begin() - ->get() - ->GetConnectionKey(), - secondExpectedKey); - - auto session - = std::make_unique(req, std::move(connection), options); - // Simulate connection was used already - session->m_lastStatusCode = Azure::Core::Http::HttpStatusCode::Ok; - session->m_sessionState = Azure::Core::Http::CurlSession::SessionState::STREAMING; - session->m_httpKeepAlive = true; + CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connections[count]), true); } - // Now there should be 2 index wit one connection each + } + + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), - 2); - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - for (auto& val : Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex) - { - EXPECT_EQ(val.second.size(), 1); - } - // The connection pool should have the two connections we added earlier. - EXPECT_NE( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .end(), - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .find(expectedConnectionKey)); - EXPECT_NE( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .end(), - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .find(secondExpectedKey)); - } + 1); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ConnectionPoolIndex[expectedConnectionKey] + .size(), + 5); + } + + // Wait for 60 secs (default time to expire a connection) + std::this_thread::sleep_for(60ms); + + { + // Now check the pool until the clean thread until finishes removing the connections or + // fail after 5 minutes (indicates a problem with the clean routine) + + auto timeOut = Context{std::chrono::system_clock::now() + 5min}; + bool poolIsEmpty = false; + while (!poolIsEmpty && !timeOut.IsCancelled()) { + std::this_thread::sleep_for(10ms); + // If test wakes while clean pool is running, it will wait until lock is released by + // the clean pool thread. std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // clean the pool - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + poolIsEmpty = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ConnectionPoolIndex.size() + == 0; } + EXPECT_TRUE(poolIsEmpty); + } + +#endif + // Test max connections in pool. Try to add 2k connections to the pool. + // Using fake connections to avoid opening real HTTP connections :) + // { + // using ::testing::_; + // using ::testing::Return; + // using ::testing::ReturnRef; + + // { + // std::lock_guard lock( + // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // // clean the pool + // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + // } + + // std::string hostKey("key"); + // for (uint64_t count = 0; count < 2000; count++) + // { + // MockCurlNetworkConnection* curlMock = new MockCurlNetworkConnection(); + // EXPECT_CALL(*curlMock, GetConnectionKey()).WillRepeatedly(ReturnRef(hostKey)); + // EXPECT_CALL(*curlMock, UpdateLastUsageTime()).WillRepeatedly(Return()); + // EXPECT_CALL(*curlMock, IsExpired()).WillRepeatedly(Return(false)); + // EXPECT_CALL(*curlMock, ReadFromSocket(_, _, _)).WillRepeatedly(Return(count)); + // EXPECT_CALL(*curlMock, DestructObj()); + + // CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + // std::unique_ptr(curlMock), + // true); + // } + // // No need to take look here because connections are mocked to never be expired. + // EXPECT_EQ( + // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + // .size(), + // 1); + // EXPECT_EQ( + // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + // .ConnectionPoolIndex[hostKey] + // .size(), + // Azure::Core::Http::_detail::MaxConnectionsPerIndex); + // // Test the first and last connection. Each connection should remove the last and + // oldest auto connectionIt = + // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + // .ConnectionPoolIndex[hostKey] + // .begin(); + // EXPECT_EQ( + // connectionIt->get()->ReadFromSocket(nullptr, 0, Context{}), + // 2000 - 1); // starting from zero + // connectionIt = --(Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + // .ConnectionPoolIndex[hostKey] + // .end()); + // EXPECT_EQ( + // connectionIt->get()->ReadFromSocket(nullptr, 0, Context{}), + // 2000 - 1024); + + // // Check the pool will take other host-key + // { + // std::string otherKey("otherHostKey"); + // MockCurlNetworkConnection* curlMock = new MockCurlNetworkConnection(); + // EXPECT_CALL(*curlMock, GetConnectionKey()).WillRepeatedly(ReturnRef(otherKey)); + // EXPECT_CALL(*curlMock, UpdateLastUsageTime()).WillRepeatedly(Return()); + // EXPECT_CALL(*curlMock, IsExpired()).WillRepeatedly(Return(false)); + // EXPECT_CALL(*curlMock, DestructObj()); + + // CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + // std::unique_ptr(curlMock), + // true); + + // EXPECT_EQ( + // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + // .ConnectionPoolIndex.size(), + // 2); + // EXPECT_EQ( + // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + // .ConnectionPoolIndex[otherKey] + // .size(), + // 1); + // // No changes to the full pool + // EXPECT_EQ( + // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + // .ConnectionPoolIndex[hostKey] + // .size(), + // Azure::Core::Http::_detail::MaxConnectionsPerIndex); + // } + // { + // std::lock_guard lock( + // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // // clean the pool + // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + // } + // } + } + + TEST(CurlConnectionPool, uniquePort) + { + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .clear(); + // Make sure there is nothing in the pool + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 0); + } + + { + // Request with no port + std::string const authority(AzureSdkHttpbinServer::Get()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ",0,0,0,0,0,1,1,0,0,0,0")); + + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); -#ifdef RUN_LONG_UNIT_TESTS { std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // clean the pool - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), 0); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); } + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); + } - // Test pool clean routine. - std::cout << "Running Connection Pool Cleaner Test. This test can take up to 2 minutes to " - "complete." - << std::endl - << "Add compiler option -DRUN_LONG_UNIT_TESTS=OFF when building if you want to " - "skip this test." - << std::endl; - { - // Make sure the clean pool thread is started by adding 5 connections to the pool - std::vector> connections; - for (int count = 0; count < 5; count++) - { - connections.emplace_back( - CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, {})); - } - for (int count = 0; count < 5; count++) - { - CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( - std::move(connections[count]), true); - } - } + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // Test connection was moved to the pool + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 1); + } + + { + // Request with port + std::string const authority(AzureSdkHttpbinServer::GetWithPort()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ":443,0,0,0,0,0,1,1,0,0,0,0")); + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); + + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); { std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // Check connection in pool is not re-used because the port is different EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), 1); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex[expectedConnectionKey] - .size(), - 5); } + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); + } + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // Check 2 connections in the pool + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 2); + } - // Wait for 60 secs (default time to expire a connection) - std::this_thread::sleep_for(60ms); + // Re-use connections + { + // Request with no port + std::string const authority(AzureSdkHttpbinServer::Get()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ",0,0,0,0,0,1,1,0,0,0,0")); - { - // Now check the pool until the clean thread until finishes removing the connections or - // fail after 5 minutes (indicates a problem with the clean routine) - - auto timeOut = Context{std::chrono::system_clock::now() + 5min}; - bool poolIsEmpty = false; - while (!poolIsEmpty && !timeOut.IsCancelled()) - { - std::this_thread::sleep_for(10ms); - // If test wakes while clean pool is running, it will wait until lock is released by - // the clean pool thread. - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - poolIsEmpty = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.size() - == 0; - } - EXPECT_TRUE(poolIsEmpty); - } + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); -#endif - // Test max connections in pool. Try to add 2k connections to the pool. - // Using fake connections to avoid opening real HTTP connections :) - // { - // using ::testing::_; - // using ::testing::Return; - // using ::testing::ReturnRef; - - // { - // std::lock_guard lock( - // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // // clean the pool - // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); - // } - - // std::string hostKey("key"); - // for (uint64_t count = 0; count < 2000; count++) - // { - // MockCurlNetworkConnection* curlMock = new MockCurlNetworkConnection(); - // EXPECT_CALL(*curlMock, GetConnectionKey()).WillRepeatedly(ReturnRef(hostKey)); - // EXPECT_CALL(*curlMock, UpdateLastUsageTime()).WillRepeatedly(Return()); - // EXPECT_CALL(*curlMock, IsExpired()).WillRepeatedly(Return(false)); - // EXPECT_CALL(*curlMock, ReadFromSocket(_, _, _)).WillRepeatedly(Return(count)); - // EXPECT_CALL(*curlMock, DestructObj()); - - // CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( - // std::unique_ptr(curlMock), - // true); - // } - // // No need to take look here because connections are mocked to never be expired. - // EXPECT_EQ( - // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - // .size(), - // 1); - // EXPECT_EQ( - // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - // .ConnectionPoolIndex[hostKey] - // .size(), - // Azure::Core::Http::_detail::MaxConnectionsPerIndex); - // // Test the first and last connection. Each connection should remove the last and - // oldest auto connectionIt = - // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - // .ConnectionPoolIndex[hostKey] - // .begin(); - // EXPECT_EQ( - // connectionIt->get()->ReadFromSocket(nullptr, 0, Context{}), - // 2000 - 1); // starting from zero - // connectionIt = --(Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - // .ConnectionPoolIndex[hostKey] - // .end()); - // EXPECT_EQ( - // connectionIt->get()->ReadFromSocket(nullptr, 0, Context{}), - // 2000 - 1024); - - // // Check the pool will take other host-key - // { - // std::string otherKey("otherHostKey"); - // MockCurlNetworkConnection* curlMock = new MockCurlNetworkConnection(); - // EXPECT_CALL(*curlMock, GetConnectionKey()).WillRepeatedly(ReturnRef(otherKey)); - // EXPECT_CALL(*curlMock, UpdateLastUsageTime()).WillRepeatedly(Return()); - // EXPECT_CALL(*curlMock, IsExpired()).WillRepeatedly(Return(false)); - // EXPECT_CALL(*curlMock, DestructObj()); - - // CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( - // std::unique_ptr(curlMock), - // true); - - // EXPECT_EQ( - // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - // .ConnectionPoolIndex.size(), - // 2); - // EXPECT_EQ( - // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - // .ConnectionPoolIndex[otherKey] - // .size(), - // 1); - // // No changes to the full pool - // EXPECT_EQ( - // Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - // .ConnectionPoolIndex[hostKey] - // .size(), - // Azure::Core::Http::_detail::MaxConnectionsPerIndex); - // } - // { - // std::lock_guard lock( - // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // // clean the pool - // CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); - // } - // } - } - - TEST(CurlConnectionPool, uniquePort) - { { std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .clear(); - // Make sure there is nothing in the pool EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), - 0); + 1); } + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); + } - { - // Request with no port - std::string const authority(AzureSdkHttpbinServer::Get()); - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); - std::string const expectedConnectionKey(CreateConnectionKey( - AzureSdkHttpbinServer::Schema(), - AzureSdkHttpbinServer::Host(), - ",0,0,0,0,0,1,1,0,0,0,0")); - - // Creating a new connection with default options - auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ExtractOrCreateCurlConnection(req, {}); - - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.size(), - 0); - EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); - } - // move connection back to the pool - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .MoveConnectionBackToPool(std::move(connection), true); - } + { + // Make sure there is nothing in the pool + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 2); + } + { + // Request with port + std::string const authority(AzureSdkHttpbinServer::GetWithPort()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ":443,0,0,0,0,0,1,1,0,0,0,0")); + + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); { std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // Test connection was moved to the pool + // Check connection in pool is not re-used because the port is different EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), 1); } + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); + } + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 2); + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .clear(); + } + } - { - // Request with port - std::string const authority(AzureSdkHttpbinServer::GetWithPort()); - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); - std::string const expectedConnectionKey(CreateConnectionKey( - AzureSdkHttpbinServer::Schema(), - AzureSdkHttpbinServer::Host(), - ":443,0,0,0,0,0,1,1,0,0,0,0")); - - // Creating a new connection with default options - auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ExtractOrCreateCurlConnection(req, {}); + TEST(CurlConnectionPool, connectionPoolKeepAlive) + { + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .clear(); + // Make sure there is nothing in the pool + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 0); + } + + { + // Request with no port + std::string const authority(AzureSdkHttpbinServer::Get()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + req.SetHeader("Connection", "keep-alive"); + req.SetHeader("Keep-Alive", "timeout=50, max=2"); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ",0,0,0,0,0,1,1,0,0,0,0")); + + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); - EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // Check connection in pool is not re-used because the port is different - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.size(), - 1); - } - // move connection back to the pool - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .MoveConnectionBackToPool(std::move(connection), true); - } { std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // Check 2 connections in the pool EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), - 2); - } - - // Re-use connections - { - // Request with no port - std::string const authority(AzureSdkHttpbinServer::Get()); - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); - std::string const expectedConnectionKey(CreateConnectionKey( - AzureSdkHttpbinServer::Schema(), - AzureSdkHttpbinServer::Host(), - ",0,0,0,0,0,1,1,0,0,0,0")); - - // Creating a new connection with default options - auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ExtractOrCreateCurlConnection(req, {}); - - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.size(), - 1); - } + 0); EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); - // move connection back to the pool - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .MoveConnectionBackToPool(std::move(connection), true); } + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); + } + + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + // Test connection was moved to the pool + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 1); + } + + // Re-use connections + { + // Request with no port + std::string const authority(AzureSdkHttpbinServer::Get()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ",0,0,0,0,0,1,1,0,0,0,0")); + + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); { - // Make sure there is nothing in the pool std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), - 2); + 0); } - { - // Request with port - std::string const authority(AzureSdkHttpbinServer::GetWithPort()); - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); - std::string const expectedConnectionKey(CreateConnectionKey( - AzureSdkHttpbinServer::Schema(), - AzureSdkHttpbinServer::Host(), - ":443,0,0,0,0,0,1,1,0,0,0,0")); - - // Creating a new connection with default options - auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ExtractOrCreateCurlConnection(req, {}); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); + } + + { + // Make sure there is nothing in the pool since we used the connection twice it is now expired + // thus it should not be moved back into the pool + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 0); + } + { + // Request with no port + std::string const authority(AzureSdkHttpbinServer::Get()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey(CreateConnectionKey( + AzureSdkHttpbinServer::Schema(), + AzureSdkHttpbinServer::Host(), + ",0,0,0,0,0,1,1,0,0,0,0")); + + // Creating a new connection with default options + auto connection = Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool + .ExtractOrCreateCurlConnection(req, {}); - EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - // Check connection in pool is not re-used because the port is different - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .ConnectionPoolIndex.size(), - 1); - } - // move connection back to the pool - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool - .MoveConnectionBackToPool(std::move(connection), true); - } { std::lock_guard lock( CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); EXPECT_EQ( Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex .size(), - 2); - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .clear(); + 0); } + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool( + std::move(connection), true); } - TEST(CurlConnectionPool, resiliencyOnConnectionClosed) { - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 1); + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .clear(); + } + } - Azure::Core::Http::CurlTransportOptions options; - auto connection - = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); - // Simulate connection lost (like server disconnection). - connection->Shutdown(); + TEST(CurlConnectionPool, resiliencyOnConnectionClosed) + { + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Get())); - { - // Check that CURLE_SEND_ERROR is produced when trying to use the connection. - auto session - = std::make_unique(req, std::move(connection), options); - auto r = session->Perform(Azure::Core::Context{}); - EXPECT_EQ(CURLE_SEND_ERROR, r); - } - } + Azure::Core::Http::CurlTransportOptions options; + auto connection + = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + // Simulate connection lost (like server disconnection). + connection->Shutdown(); - TEST(CurlConnectionPool, forceConnectionClosed) { - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Status(101))); + // Check that CURLE_SEND_ERROR is produced when trying to use the connection. + auto session + = std::make_unique(req, std::move(connection), options); + auto r = session->Perform(Azure::Core::Context{}); + EXPECT_EQ(CURLE_SEND_ERROR, r); + } + } - Azure::Core::Http::CurlTransportOptions options; - auto connection - = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + TEST(CurlConnectionPool, forceConnectionClosed) + { + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Status(101))); - { - // Check we can use the connection to retrieve headers in the response, but the connection - // is still flagged as shutdown. - auto session - = std::make_unique(req, std::move(connection), options); - - auto r = session->Perform(Azure::Core::Context{}); - EXPECT_EQ(CURLE_OK, r); - auto response = session->ExtractResponse(); - EXPECT_EQ(response->GetStatusCode(), Azure::Core::Http::HttpStatusCode::SwitchingProtocols); - EXPECT_EQ("close", response->GetHeaders().find("Connection")->second); - } + Azure::Core::Http::CurlTransportOptions options; + auto connection + = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); + + { + // Check we can use the connection to retrieve headers in the response, but the connection + // is still flagged as shutdown. + auto session + = std::make_unique(req, std::move(connection), options); + + auto r = session->Perform(Azure::Core::Context{}); + EXPECT_EQ(CURLE_OK, r); + auto response = session->ExtractResponse(); + EXPECT_EQ(response->GetStatusCode(), Azure::Core::Http::HttpStatusCode::SwitchingProtocols); + EXPECT_EQ("close", response->GetHeaders().find("Connection")->second); } + } - TEST(CurlConnectionPool, connectionClose) + TEST(CurlConnectionPool, connectionClose) + { + /// When getting the header connection: close from an HTTP response, the connection should not + /// be moved back to the pool. { - /// When getting the header connection: close from an HTTP response, the connection should not - /// be moved back to the pool. - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); - // Make sure there are nothing in the pool - EXPECT_EQ(CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.size(), 0); - } + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.clear(); + // Make sure there are nothing in the pool + EXPECT_EQ(CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex.size(), 0); + } - // Use the same request for all connections. - Azure::Core::Http::Request req( - Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Headers())); - // Server will return this header back - req.SetHeader("connection", "close"); + // Use the same request for all connections. + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(AzureSdkHttpbinServer::Headers())); + // Server will return this header back + req.SetHeader("connection", "close"); - { - // Create a pipeline to send the request and dispose after it. - Azure::Core::Http::_internal::HttpPipeline pipeline({}, "test", "test", {}, {}); - auto response = pipeline.Send(req, Azure::Core::Context{}); - EXPECT_PRED2( - [](Azure::Core::Http::HttpStatusCode a, Azure::Core::Http::HttpStatusCode b) { - return a == b; - }, - response->GetStatusCode(), - Azure::Core::Http::HttpStatusCode::Ok); - } + { + // Create a pipeline to send the request and dispose after it. + Azure::Core::Http::_internal::HttpPipeline pipeline({}, "test", "test", {}, {}); + auto response = pipeline.Send(req, Azure::Core::Context{}); + EXPECT_PRED2( + [](Azure::Core::Http::HttpStatusCode a, Azure::Core::Http::HttpStatusCode b) { + return a == b; + }, + response->GetStatusCode(), + Azure::Core::Http::HttpStatusCode::Ok); + } - // Check that after the connection is gone, it is moved back to the pool - { - std::lock_guard lock( - CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); - EXPECT_EQ( - Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex - .size(), - 0); - } + // Check that after the connection is gone, it is moved back to the pool + { + std::lock_guard lock( + CurlConnectionPool::g_curlConnectionPool.ConnectionPoolMutex); + EXPECT_EQ( + Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool.ConnectionPoolIndex + .size(), + 0); } + } #endif }}} // namespace Azure::Core::Test From c674c45e53fa64928e3423ba2f32e61d06fa8750 Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 11:21:38 -0700 Subject: [PATCH 13/26] erwe --- sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp b/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp index b51b85be47..29442c1355 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp @@ -39,7 +39,7 @@ inline std::string CreateConnectionKey( namespace Azure { namespace Core { namespace Test { - /*********************** Unique Tests for Libcurl ********************************/ + /*********************** Unique Tests for Libcurl ********************************/ #if defined(BUILD_CURL_HTTP_TRANSPORT_ADAPTER) TEST(CurlConnectionPool, connectionPoolTest) From f8861dda00b83b041ba383b0fc97075f5b82fdbe Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 14:49:49 -0700 Subject: [PATCH 14/26] check for the headers in the response. --- sdk/core/azure-core/src/http/curl/curl.cpp | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index d9bc2b7d90..af86addcfb 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -345,6 +345,9 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const { // Create CurlSession to perform request Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Creating a new session."); + // extract the relevant headers for keep-alive for processing when the response arrives. + auto requestConnectionHeader = request.GetHeaders().find("Connection"); + auto requestKeepAliveHeader = request.GetHeaders().find("Keep-Alive"); auto session = std::make_unique( request, @@ -403,6 +406,32 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); + { + auto responseConnectionHeader = response->GetHeaders().find("Connection"); + auto responseKeepAliveHeader = response->GetHeaders().find("Keep-Alive"); + // if the server supports keep alive the headers should be present in the reponse. If they are + // they shoud be the same as the request headers. + if (responseConnectionHeader != response->GetHeaders().end() + && requestConnectionHeader != request.GetHeaders().end() + // just in case the server sends the connection header in a different case + && Azure::Core::_internal::StringExtensions::ToLower(responseConnectionHeader->second) + == Azure::Core::_internal::StringExtensions::ToLower(requestConnectionHeader->second) + && responseKeepAliveHeader != response->GetHeaders().end() + && requestKeepAliveHeader != request.GetHeaders().end() + // just in case the server sends the keep-alive header in a different case + && Azure::Core::_internal::StringExtensions::ToLower(responseKeepAliveHeader->second) + == Azure::Core::_internal::StringExtensions::ToLower(requestKeepAliveHeader->second)) + { + Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); + } + else + { + // cleanup keep-alive header in the request since they don't match up the response from the + // server. + request.RemoveHeader("Keep-Alive"); + m_options.KeepAliveOptions.Reset(); + } + } return response; } From 3862dd74d61d85e5cbd7bca10cb3543a21c41f53 Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 15:35:23 -0700 Subject: [PATCH 15/26] rtert --- sdk/core/azure-core/src/http/curl/curl.cpp | 48 +++++++++++----------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index af86addcfb..81fada60f2 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -406,32 +406,32 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); + + auto responseConnectionHeader = response->GetHeaders().find("Connection"); + auto responseKeepAliveHeader = response->GetHeaders().find("Keep-Alive"); + // if the server supports keep alive the headers should be present in the reponse. If they are + // they shoud be the same as the request headers. + if (responseConnectionHeader != response->GetHeaders().end() + && requestConnectionHeader != request.GetHeaders().end() + // just in case the server sends the connection header in a different case + && Azure::Core::_internal::StringExtensions::ToLower(responseConnectionHeader->second) + == Azure::Core::_internal::StringExtensions::ToLower(requestConnectionHeader->second) + && responseKeepAliveHeader != response->GetHeaders().end() + && requestKeepAliveHeader != request.GetHeaders().end() + // just in case the server sends the keep-alive header in a different case + && Azure::Core::_internal::StringExtensions::ToLower(responseKeepAliveHeader->second) + == Azure::Core::_internal::StringExtensions::ToLower(requestKeepAliveHeader->second)) + { + Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); + } + else { - auto responseConnectionHeader = response->GetHeaders().find("Connection"); - auto responseKeepAliveHeader = response->GetHeaders().find("Keep-Alive"); - // if the server supports keep alive the headers should be present in the reponse. If they are - // they shoud be the same as the request headers. - if (responseConnectionHeader != response->GetHeaders().end() - && requestConnectionHeader != request.GetHeaders().end() - // just in case the server sends the connection header in a different case - && Azure::Core::_internal::StringExtensions::ToLower(responseConnectionHeader->second) - == Azure::Core::_internal::StringExtensions::ToLower(requestConnectionHeader->second) - && responseKeepAliveHeader != response->GetHeaders().end() - && requestKeepAliveHeader != request.GetHeaders().end() - // just in case the server sends the keep-alive header in a different case - && Azure::Core::_internal::StringExtensions::ToLower(responseKeepAliveHeader->second) - == Azure::Core::_internal::StringExtensions::ToLower(requestKeepAliveHeader->second)) - { - Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); - } - else - { - // cleanup keep-alive header in the request since they don't match up the response from the - // server. - request.RemoveHeader("Keep-Alive"); - m_options.KeepAliveOptions.Reset(); - } + // cleanup keep-alive header in the request since they don't match up the response from the + // server. + request.RemoveHeader("Keep-Alive"); + m_options.KeepAliveOptions.Reset(); } + return response; } From b6a1cef91bc967223e06a58c0cd8d6e019e03658 Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 16:02:49 -0700 Subject: [PATCH 16/26] gdfgd --- sdk/core/azure-core/src/http/curl/curl.cpp | 26 ++++++++++------------ 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 81fada60f2..329d8a93c5 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -345,9 +345,6 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const { // Create CurlSession to perform request Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Creating a new session."); - // extract the relevant headers for keep-alive for processing when the response arrives. - auto requestConnectionHeader = request.GetHeaders().find("Connection"); - auto requestKeepAliveHeader = request.GetHeaders().find("Keep-Alive"); auto session = std::make_unique( request, @@ -406,21 +403,22 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); - - auto responseConnectionHeader = response->GetHeaders().find("Connection"); - auto responseKeepAliveHeader = response->GetHeaders().find("Keep-Alive"); // if the server supports keep alive the headers should be present in the reponse. If they are // they shoud be the same as the request headers. - if (responseConnectionHeader != response->GetHeaders().end() - && requestConnectionHeader != request.GetHeaders().end() + if (response->GetHeaders().find("Connection") != response->GetHeaders().end() + && request.GetHeaders().find("Connection") != request.GetHeaders().end() // just in case the server sends the connection header in a different case - && Azure::Core::_internal::StringExtensions::ToLower(responseConnectionHeader->second) - == Azure::Core::_internal::StringExtensions::ToLower(requestConnectionHeader->second) - && responseKeepAliveHeader != response->GetHeaders().end() - && requestKeepAliveHeader != request.GetHeaders().end() + && Azure::Core::_internal::StringExtensions::ToLower( + response->GetHeaders().find("Connection")->second) + == Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeaders().find("Connection")->second) + && response->GetHeaders().find("Keep-Alive") != response->GetHeaders().end() + && request.GetHeaders().find("Keep-Alive") != request.GetHeaders().end() // just in case the server sends the keep-alive header in a different case - && Azure::Core::_internal::StringExtensions::ToLower(responseKeepAliveHeader->second) - == Azure::Core::_internal::StringExtensions::ToLower(requestKeepAliveHeader->second)) + && Azure::Core::_internal::StringExtensions::ToLower( + response->GetHeaders().find("Keep-Alive")->second) + == Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeaders().find("Keep-Alive")->second)) { Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); } From d46fc36760bc035e1c3b63cbadf78964ac80955b Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 16:03:01 -0700 Subject: [PATCH 17/26] rew --- sdk/core/azure-core/src/http/curl/curl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 329d8a93c5..a229565d2e 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -404,7 +404,7 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); // if the server supports keep alive the headers should be present in the reponse. If they are - // they shoud be the same as the request headers. + // they should be the same as the request headers. if (response->GetHeaders().find("Connection") != response->GetHeaders().end() && request.GetHeaders().find("Connection") != request.GetHeaders().end() // just in case the server sends the connection header in a different case From 56a4dc0135c70dc1f588b736a972d414c5b929c4 Mon Sep 17 00:00:00 2001 From: George Arama Date: Thu, 22 Aug 2024 16:22:01 -0700 Subject: [PATCH 18/26] spelling --- sdk/core/azure-core/src/http/curl/curl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index a229565d2e..e824db9a09 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -403,7 +403,7 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); - // if the server supports keep alive the headers should be present in the reponse. If they are + // if the server supports keep alive the headers should be present in the response. If they are // they should be the same as the request headers. if (response->GetHeaders().find("Connection") != response->GetHeaders().end() && request.GetHeaders().find("Connection") != request.GetHeaders().end() From cb5f4a11117a142a1b813014eb9e8f6e66096ccb Mon Sep 17 00:00:00 2001 From: George Arama Date: Fri, 23 Aug 2024 11:20:15 -0700 Subject: [PATCH 19/26] temp disable response reading --- sdk/core/azure-core/src/http/curl/curl.cpp | 48 +++++++++++----------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index e824db9a09..62f2844ce5 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -403,33 +403,35 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); - // if the server supports keep alive the headers should be present in the response. If they are + /* // if the server supports keep alive the headers should be present in the response. If they + // are // they should be the same as the request headers. if (response->GetHeaders().find("Connection") != response->GetHeaders().end() && request.GetHeaders().find("Connection") != request.GetHeaders().end() - // just in case the server sends the connection header in a different case - && Azure::Core::_internal::StringExtensions::ToLower( - response->GetHeaders().find("Connection")->second) - == Azure::Core::_internal::StringExtensions::ToLower( - request.GetHeaders().find("Connection")->second) && response->GetHeaders().find("Keep-Alive") != response->GetHeaders().end() - && request.GetHeaders().find("Keep-Alive") != request.GetHeaders().end() - // just in case the server sends the keep-alive header in a different case - && Azure::Core::_internal::StringExtensions::ToLower( - response->GetHeaders().find("Keep-Alive")->second) - == Azure::Core::_internal::StringExtensions::ToLower( - request.GetHeaders().find("Keep-Alive")->second)) - { - Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); - } - else - { - // cleanup keep-alive header in the request since they don't match up the response from the - // server. - request.RemoveHeader("Keep-Alive"); - m_options.KeepAliveOptions.Reset(); - } - + && request.GetHeaders().find("Keep-Alive") != request.GetHeaders().end()) + { + // just in case the server sends the headers in a different case + if (Azure::Core::_internal::StringExtensions::ToLower( + response->GetHeaders().find("Connection")->second) + == Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeaders().find("Connection")->second) + // just in case the server sends the keep-alive header in a different case + && Azure::Core::_internal::StringExtensions::ToLower( + response->GetHeaders().find("Keep-Alive")->second) + == Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeaders().find("Keep-Alive")->second)) + { + Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); + } + else + { + // cleanup keep-alive header in the request since they don't match up the response from the + // server. + request.RemoveHeader("Keep-Alive"); + m_options.KeepAliveOptions.Reset(); + } + }*/ return response; } From 255e56db4346d97cd196d1bc01226cb40d6c18bf Mon Sep 17 00:00:00 2001 From: George Arama Date: Mon, 26 Aug 2024 10:56:03 -0700 Subject: [PATCH 20/26] fix if statement --- sdk/core/azure-core/src/http/curl/curl.cpp | 50 +++++++++++----------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 62f2844ce5..1588341163 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -403,35 +403,33 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); - /* // if the server supports keep alive the headers should be present in the response. If they - // are + // if the server supports keep alive the headers should be present in the response. If they are // they should be the same as the request headers. if (response->GetHeaders().find("Connection") != response->GetHeaders().end() - && request.GetHeaders().find("Connection") != request.GetHeaders().end() + && request.GetHeader("Connection").HasValue() && response->GetHeaders().find("Keep-Alive") != response->GetHeaders().end() - && request.GetHeaders().find("Keep-Alive") != request.GetHeaders().end()) - { - // just in case the server sends the headers in a different case - if (Azure::Core::_internal::StringExtensions::ToLower( - response->GetHeaders().find("Connection")->second) - == Azure::Core::_internal::StringExtensions::ToLower( - request.GetHeaders().find("Connection")->second) - // just in case the server sends the keep-alive header in a different case - && Azure::Core::_internal::StringExtensions::ToLower( - response->GetHeaders().find("Keep-Alive")->second) - == Azure::Core::_internal::StringExtensions::ToLower( - request.GetHeaders().find("Keep-Alive")->second)) - { - Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); - } - else - { - // cleanup keep-alive header in the request since they don't match up the response from the - // server. - request.RemoveHeader("Keep-Alive"); - m_options.KeepAliveOptions.Reset(); - } - }*/ + && request.GetHeader("Keep-Alive").HasValue() + // just in case the server sends the headers in a different case + && Azure::Core::_internal::StringExtensions::ToLower( + response->GetHeaders().find("Connection")->second) + == Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeader("Connection").Value()) + // just in case the server sends the keep-alive header in a different case + && Azure::Core::_internal::StringExtensions::ToLower( + response->GetHeaders().find("Keep-Alive")->second) + == Azure::Core::_internal::StringExtensions::ToLower( + request.GetHeader("Keep-Alive").Value())) + { + Log::Write(Logger::Level::Verbose, LogMsgPrefix + "Response has same keep-alive settings"); + } + else + { + // cleanup keep-alive header in the request since they don't match up the response from + // the server. + request.RemoveHeader("Keep-Alive"); + m_options.KeepAliveOptions.Reset(); + } + return response; } From bd71ca0d01e30e9d0f7496c385c5bcd398e7248d Mon Sep 17 00:00:00 2001 From: George Arama Date: Mon, 26 Aug 2024 12:29:45 -0700 Subject: [PATCH 21/26] validate keep alive --- .../inc/azure/core/http/curl_transport.hpp | 1 + sdk/core/azure-core/src/http/curl/curl.cpp | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index f30d49c697..f11760db8e 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -245,6 +245,7 @@ namespace Azure { namespace Core { namespace Http { */ virtual void OnUpgradedConnection(std::unique_ptr&&){}; + void ValidateKeepAliveHeaders(Request& request, std::unique_ptr &response); public: /** * @brief Construct a new CurlTransport object. diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 1588341163..6c8484bfa0 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -403,6 +403,16 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const auto response = session->ExtractResponse(); // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); + // check the consistency of the keep alive headers between the request and the response + ValidateKeepAliveHeaders(request, response); + + return response; +} + +void CurlTransport::ValidateKeepAliveHeaders( + Request& request, + std::unique_ptr &response) +{ // if the server supports keep alive the headers should be present in the response. If they are // they should be the same as the request headers. if (response->GetHeaders().find("Connection") != response->GetHeaders().end() @@ -429,8 +439,6 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const request.RemoveHeader("Keep-Alive"); m_options.KeepAliveOptions.Reset(); } - - return response; } CURLcode CurlSession::Perform(Context const& context) From 42fb62676c485d5dd1c5b8ec9dd51fbf193ec11d Mon Sep 17 00:00:00 2001 From: George Arama Date: Mon, 26 Aug 2024 12:43:32 -0700 Subject: [PATCH 22/26] verify tests --- .../inc/azure/core/http/curl_transport.hpp | 8 ++++++- .../azure-core/test/ut/curl_options_test.cpp | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index f11760db8e..082ff28f9b 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -16,6 +16,9 @@ #include #include #include +namespace Azure { namespace Core { namespace Test { + class CurlTransport_VerifyKeepAliveHeaders_Test; +}}} // namespace Azure::Core::Test namespace Azure { namespace Core { namespace Http { class CurlNetworkConnection; @@ -236,6 +239,8 @@ namespace Azure { namespace Core { namespace Http { * @brief Concrete implementation of an HTTP Transport that uses libcurl. */ class CurlTransport : public HttpTransport { + friend class Azure::Core::Test::CurlTransport_VerifyKeepAliveHeaders_Test; + private: CurlTransportOptions m_options; @@ -245,7 +250,8 @@ namespace Azure { namespace Core { namespace Http { */ virtual void OnUpgradedConnection(std::unique_ptr&&){}; - void ValidateKeepAliveHeaders(Request& request, std::unique_ptr &response); + void ValidateKeepAliveHeaders(Request& request, std::unique_ptr& response); + public: /** * @brief Construct a new CurlTransport object. diff --git a/sdk/core/azure-core/test/ut/curl_options_test.cpp b/sdk/core/azure-core/test/ut/curl_options_test.cpp index 72f10fb73a..e6487094fd 100644 --- a/sdk/core/azure-core/test/ut/curl_options_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_options_test.cpp @@ -365,4 +365,28 @@ namespace Azure { namespace Core { namespace Test { 0); } + TEST(CurlTransport, VerifyKeepAliveHeaders) + { + Azure::Core::Http::CurlTransport transport; + Azure::Core::Url url(AzureSdkHttpbinServer::Get()); + Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); + request.SetHeader("Connection", "keep-alive"); + request.SetHeader("Keep-Alive", "timeout=5, max=1000"); + std::unique_ptr response + = std::make_unique( + 1, 0, Azure::Core::Http::HttpStatusCode(200), "OK"); + response->SetHeader("Connection", "keep-alive"); + response->SetHeader("Keep-Alive", "timeout=5, max=1000"); + + transport.ValidateKeepAliveHeaders(request, response); + + EXPECT_EQ(request.GetHeader("Connection").Value(), "keep-alive"); + EXPECT_EQ(request.GetHeader("Keep-Alive").Value(), "timeout=5, max=1000"); + + response->SetHeader("Connection", "close"); + transport.ValidateKeepAliveHeaders(request, response); + + EXPECT_EQ(request.GetHeader("Connection").Value(), "keep-alive"); + EXPECT_EQ(request.GetHeader("Keep-Alive").HasValue(), false); + } }}} // namespace Azure::Core::Test From 1dfde82cff341c90506c34b945cfb9105ba6e755 Mon Sep 17 00:00:00 2001 From: George Arama Date: Mon, 26 Aug 2024 12:50:08 -0700 Subject: [PATCH 23/26] clang --- sdk/core/azure-core/src/http/curl/curl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 6c8484bfa0..b0a1d65634 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -411,7 +411,7 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const void CurlTransport::ValidateKeepAliveHeaders( Request& request, - std::unique_ptr &response) + std::unique_ptr& response) { // if the server supports keep alive the headers should be present in the response. If they are // they should be the same as the request headers. From ccf469d4ab7e419ceb5709d962c561a0fcebbe94 Mon Sep 17 00:00:00 2001 From: gearama <50641385+gearama@users.noreply.github.com> Date: Fri, 30 Aug 2024 13:49:46 -0700 Subject: [PATCH 24/26] Update sdk/core/azure-core/test/ut/curl_connection_tests.cpp Co-authored-by: Rick Winter --- sdk/core/azure-core/test/ut/curl_connection_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp index 5e86ad9dcc..aacf6f946d 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -227,4 +227,4 @@ namespace Azure { namespace Core { namespace Test { } }}} // namespace Azure::Core::Test -#endif \ No newline at end of file +#endif From 8deb4f27a42a48a008b57a886c4174c40a94d750 Mon Sep 17 00:00:00 2001 From: George Arama Date: Wed, 4 Sep 2024 12:15:08 -0700 Subject: [PATCH 25/26] PR comments --- .../inc/azure/core/http/curl_transport.hpp | 8 ++-- sdk/core/azure-core/src/http/curl/curl.cpp | 8 +++- .../src/http/curl/curl_connection_private.hpp | 5 +- .../test/ut/curl_connection_tests.cpp | 48 ++++++++----------- 4 files changed, 35 insertions(+), 34 deletions(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index 082ff28f9b..872acd58b9 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -16,10 +16,11 @@ #include #include #include +#if defined(_azure_TESTING_BUILD) namespace Azure { namespace Core { namespace Test { class CurlTransport_VerifyKeepAliveHeaders_Test; }}} // namespace Azure::Core::Test - +#endif namespace Azure { namespace Core { namespace Http { class CurlNetworkConnection; @@ -54,7 +55,7 @@ namespace Azure { namespace Core { namespace Http { * @brief The maximum number of requests that a host will allow over a single connection. * */ - std::size_t MaxRequests = size_t(0); + std::size_t MaxRequests = size_t(1); }; } // namespace _detail @@ -239,8 +240,9 @@ namespace Azure { namespace Core { namespace Http { * @brief Concrete implementation of an HTTP Transport that uses libcurl. */ class CurlTransport : public HttpTransport { +#if defined(_azure_TESTING_BUILD) friend class Azure::Core::Test::CurlTransport_VerifyKeepAliveHeaders_Test; - +#endif private: CurlTransportOptions m_options; diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index b0a1d65634..8e78e0121c 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -404,11 +404,15 @@ std::unique_ptr CurlTransport::Send(Request& request, Context const // Move the ownership of the CurlSession (bodyStream) to the response response->SetBodyStream(std::move(session)); // check the consistency of the keep alive headers between the request and the response + // as suggested by RFC 7230 section 6.3 (https://tools.ietf.org/html/rfc7230#section-6.3) ValidateKeepAliveHeaders(request, response); return response; } +// Check if the server supports keep alive and if the headers are consistent between the request and +// the response. If they are not consistent, the keep alive header in the request is removed.and the +// calculated options are reset in order to prevent the wrongful caching of the keep alive settings. void CurlTransport::ValidateKeepAliveHeaders( Request& request, std::unique_ptr& response) @@ -419,7 +423,9 @@ void CurlTransport::ValidateKeepAliveHeaders( && request.GetHeader("Connection").HasValue() && response->GetHeaders().find("Keep-Alive") != response->GetHeaders().end() && request.GetHeader("Keep-Alive").HasValue() - // just in case the server sends the headers in a different case + // just in case the server sends the headers in a different case, the case sensitivity of the + // map is guaranteed for keys not values. So we need to ensure we compare the values in a case + // insensitive way. && Azure::Core::_internal::StringExtensions::ToLower( response->GetHeaders().find("Connection")->second) == Azure::Core::_internal::StringExtensions::ToLower( diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp index 7f1b2ab6b9..669b4872f9 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_private.hpp @@ -29,9 +29,11 @@ /// From openssl/x509.h. Avoids needing to include openssl headers typedef struct x509_store_ctx_st X509_STORE_CTX; +#if defined(_azure_TESTING_BUILD) namespace Azure { namespace Core { namespace Test { class CurlConnectionTest_ParseKeepAliveHeader_Test; }}} // namespace Azure::Core::Test +#endif namespace Azure { namespace Core { namespace _detail { /** @@ -162,8 +164,9 @@ namespace Azure { namespace Core { * */ class CurlConnection final : public CurlNetworkConnection { +#if defined(_azure_TESTING_BUILD) friend class Azure::Core::Test::CurlConnectionTest_ParseKeepAliveHeader_Test; - +#endif private: Azure::Core::_internal::UniqueHandle m_handle; curl_socket_t m_curlSocket; diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp index aacf6f946d..b968e81690 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -17,15 +17,6 @@ #if defined(BUILD_CURL_HTTP_TRANSPORT_ADAPTER) #include "azure/core/http/curl_transport.hpp" -#ifdef _MSC_VER -#pragma warning(push) -#elif defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) // !_MSC_VER -#pragma GCC diagnostic push -#elif defined(__clang__) // !_MSC_VER !__clang__ -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wgnu-zero-variadic-macro-arguments" -#endif // _MSC_VER - #include #include @@ -37,8 +28,7 @@ namespace Azure { namespace Core { namespace Test { - class CurlConnectionTest : public ::testing::Test { - }; + class CurlConnectionTest : public ::testing::Test {}; TEST(CurlConnectionTest, ParseKeepAliveHeader) { @@ -69,7 +59,7 @@ namespace Azure { namespace Core { namespace Test { } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); } { @@ -79,7 +69,7 @@ namespace Azure { namespace Core { namespace Test { } { auto parsedHeader = curlConnection.ParseKeepAliveHeader(""); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { @@ -99,7 +89,7 @@ namespace Azure { namespace Core { namespace Test { } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(5)); } { @@ -109,77 +99,77 @@ namespace Azure { namespace Core { namespace Test { } { auto parsedHeader = curlConnection.ParseKeepAliveHeader(", , extra=1, "); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=, extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("max=, extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("max= , extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=, max=10, extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=, extra=1,"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max= , extra=1, "); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max=10, extra=1"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max= , extra=1,"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max= , extra=1, "); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5 max= , extra=1,"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout= , max= 10 extra=1, "); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=x, max=10"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } { auto parsedHeader = curlConnection.ParseKeepAliveHeader("timeout=5, max=n"); - EXPECT_EQ(parsedHeader.MaxRequests, size_t(0)); + EXPECT_EQ(parsedHeader.MaxRequests, size_t(1)); EXPECT_EQ(parsedHeader.ConnectionTimeout, std::chrono::seconds(0)); } } From bfc8deead2a23665b91cdff61a38777de7f45287 Mon Sep 17 00:00:00 2001 From: George Arama Date: Wed, 4 Sep 2024 13:03:32 -0700 Subject: [PATCH 26/26] clang --- sdk/core/azure-core/test/ut/curl_connection_tests.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp index b968e81690..a40c43969d 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_tests.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_tests.cpp @@ -28,7 +28,8 @@ namespace Azure { namespace Core { namespace Test { - class CurlConnectionTest : public ::testing::Test {}; + class CurlConnectionTest : public ::testing::Test { + }; TEST(CurlConnectionTest, ParseKeepAliveHeader) {