Do not reuse connection with diff port (#2122)

* Do not reuse connection with diff port

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)
 

sdk/core/azure-core/src/http/curl/curl.cpp

@@ -1157,7 +1157,9 @@ std::unique_ptr<CurlNetworkConnection> 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<CurlNetworkConnection> 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(

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

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(

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