From 668d343dde03a115944d097d94e201f7d8a0dedc Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Fri, 16 Apr 2021 16:11:11 -0700 Subject: [PATCH] Do not reuse connection with diff port (#2122) * Do not reuse connection with diff port --- sdk/core/azure-core/CHANGELOG.md | 3 + sdk/core/azure-core/src/http/curl/curl.cpp | 5 +- .../curl/curl_connection_pool_private.hpp | 2 + .../test/ut/curl_connection_pool.cpp | 101 +++++++++++++++++- .../test/ut/transport_adapter_base.hpp | 16 ++- 5 files changed, 118 insertions(+), 9 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index cc6f3325f..ef31a52ad 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -2,6 +2,9 @@ ## 1.0.0-beta.9 (Unreleased) +### Bug Fixes + +- Do not re-use a libcurl connection to same host but different port. ## 1.0.0-beta.8 (2021-04-07) diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 213737872..d1239f475 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -1157,7 +1157,9 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo CurlTransportOptions const& options, bool resetPool) { - std::string const& host = request.GetUrl().GetHost(); + uint16_t port = request.GetUrl().GetPort(); + std::string const& host = request.GetUrl().GetScheme() + request.GetUrl().GetHost() + + (port != 0 ? std::to_string(port) : ""); std::string const connectionKey = GetConnectionKey(host, options); { @@ -1219,7 +1221,6 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo + std::string(curl_easy_strerror(result))); } - uint16_t port = request.GetUrl().GetPort(); if (port != 0 && !SetLibcurlOption(newHandle, CURLOPT_PORT, port, &result)) { throw Azure::Core::Http::TransportException( diff --git a/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp b/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp index b18adbe3a..3bc9f22e9 100644 --- a/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp @@ -25,6 +25,7 @@ // Define the class name that reads from ConnectionPool private members namespace Azure { namespace Core { namespace Test { class CurlConnectionPool_connectionPoolTest_Test; + class CurlConnectionPool_uniquePort_Test; }}} // namespace Azure::Core::Test #endif @@ -44,6 +45,7 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { #if defined(TESTING_BUILD) // Give access to private to this tests class friend class Azure::Core::Test::CurlConnectionPool_connectionPoolTest_Test; + friend class Azure::Core::Test::CurlConnectionPool_uniquePort_Test; #endif private: // The cttor and dttor of this member makes sure of calling the libcurl global init and cleanup diff --git a/sdk/core/azure-core/test/ut/curl_connection_pool.cpp b/sdk/core/azure-core/test/ut/curl_connection_pool.cpp index 0cf15ebf5..7af20cb45 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_pool.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_pool.cpp @@ -37,7 +37,8 @@ namespace Azure { namespace Core { namespace Test { // 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 = AzureSdkHttpbinServer::Host() + "0011"; + std::string const expectedConnectionKey + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "0011"; { // Creating a new connection with default options @@ -88,7 +89,8 @@ namespace Azure { namespace Core { namespace Test { 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::Host() + "0010"; + std::string const secondExpectedKey + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "0010"; { // Creating a new connection with options Azure::Core::Http::CurlTransportOptions options; @@ -181,6 +183,101 @@ namespace Azure { namespace Core { namespace Test { #endif } + TEST(CurlConnectionPool, uniquePort) + { + Azure::Core::Http::_detail::CurlConnectionPool::ClearIndex(); + // Make sure there is nothing in the pool + EXPECT_EQ(Azure::Core::Http::_detail::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 + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "0011"; + + // Creating a new connection with default options + auto connection + = Azure::Core::Http::_detail::CurlConnectionPool::ExtractOrCreateCurlConnection( + req, {}); + + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 0); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::MoveConnectionBackToPool( + std::move(connection), Azure::Core::Http::HttpStatusCode::Ok); + } + // Test connection was moved to the pool + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 1); + + { + // Request with port + std::string const authority(AzureSdkHttpbinServer::WithPort()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "4430011"; + + // Creating a new connection with default options + auto connection + = Azure::Core::Http::_detail::CurlConnectionPool::ExtractOrCreateCurlConnection( + req, {}); + + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // Check connection in pool is not re-used because the port is different + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 1); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::MoveConnectionBackToPool( + std::move(connection), Azure::Core::Http::HttpStatusCode::Ok); + } + // Check 2 connections in the pool + EXPECT_EQ(Azure::Core::Http::_detail::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 + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "0011"; + + // Creating a new connection with default options + auto connection + = Azure::Core::Http::_detail::CurlConnectionPool::ExtractOrCreateCurlConnection( + req, {}); + + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 1); + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::MoveConnectionBackToPool( + std::move(connection), Azure::Core::Http::HttpStatusCode::Ok); + } + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 2); + { + // Request with port + std::string const authority(AzureSdkHttpbinServer::WithPort()); + Azure::Core::Http::Request req( + Azure::Core::Http::HttpMethod::Get, Azure::Core::Url(authority)); + std::string const expectedConnectionKey + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "4430011"; + + // Creating a new connection with default options + auto connection + = Azure::Core::Http::_detail::CurlConnectionPool::ExtractOrCreateCurlConnection( + req, {}); + + EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey); + // Check connection in pool is not re-used because the port is different + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 1); + // move connection back to the pool + Azure::Core::Http::_detail::CurlConnectionPool::MoveConnectionBackToPool( + std::move(connection), Azure::Core::Http::HttpStatusCode::Ok); + } + EXPECT_EQ(Azure::Core::Http::_detail::CurlConnectionPool::ConnectionPoolIndex.size(), 2); + Azure::Core::Http::_detail::CurlConnectionPool::ClearIndex(); + } + TEST(CurlConnectionPool, resiliencyOnConnectionClosed) { Azure::Core::Http::Request req( diff --git a/sdk/core/azure-core/test/ut/transport_adapter_base.hpp b/sdk/core/azure-core/test/ut/transport_adapter_base.hpp index 21d672b14..a0410d599 100644 --- a/sdk/core/azure-core/test/ut/transport_adapter_base.hpp +++ b/sdk/core/azure-core/test/ut/transport_adapter_base.hpp @@ -20,7 +20,7 @@ namespace Azure { namespace Core { namespace Test { namespace _detail { - constexpr static const char AzureSdkHttpbinServerSchema[] = "https://"; + constexpr static const char AzureSdkHttpbinServerSchema[] = "https"; constexpr static const char AzureSdkHttpbinServer[] = "azuresdkforcpp.azurewebsites.net"; } // namespace _detail @@ -28,25 +28,31 @@ namespace Azure { namespace Core { namespace Test { { inline static std::string Get() { - return std::string(_detail::AzureSdkHttpbinServerSchema) + return std::string(_detail::AzureSdkHttpbinServerSchema) + "://" + std::string(_detail::AzureSdkHttpbinServer) + "/get"; } + inline static std::string WithPort() + { + return std::string(_detail::AzureSdkHttpbinServerSchema) + "://" + + std::string(_detail::AzureSdkHttpbinServer) + ":443/get"; + } inline static std::string Put() { - return std::string(_detail::AzureSdkHttpbinServerSchema) + return std::string(_detail::AzureSdkHttpbinServerSchema) + "://" + std::string(_detail::AzureSdkHttpbinServer) + "/put"; } inline static std::string Delete() { - return std::string(_detail::AzureSdkHttpbinServerSchema) + return std::string(_detail::AzureSdkHttpbinServerSchema) + "://" + std::string(_detail::AzureSdkHttpbinServer) + "/delete"; } inline static std::string Patch() { - return std::string(_detail::AzureSdkHttpbinServerSchema) + return std::string(_detail::AzureSdkHttpbinServerSchema) + "://" + std::string(_detail::AzureSdkHttpbinServer) + "/patch"; } inline static std::string Host() { return std::string(_detail::AzureSdkHttpbinServer); } + inline static std::string Schema() { return std::string(_detail::AzureSdkHttpbinServerSchema); } }; struct TransportAdaptersTestParameter