Fixed libcurl connection pool to use `Connection` response header values (#5473)

* Fixed libcurl connection pool to use `Connection` response header values

---------

Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>

sdk/core/azure-core/CHANGELOG.md

@@ -1,14 +1,10 @@
 # Release History
 
-## 1.12.0-beta.1 (Unreleased)
-
-### Features Added
-
-### Breaking Changes
+## 1.12.0-beta.1 (2024-04-10)
 
 ### Bugs Fixed
 
-### Other Changes
+- [[#5450]](https://github.com/Azure/azure-sdk-for-cpp/issues/5450) Fixed libcurl connection pool to use `Connection` response header values.
 
 ## 1.11.3 (2024-04-09)
 

sdk/core/azure-core/inc/azure/core/http/raw_response.hpp

@@ -138,6 +138,18 @@ namespace Azure { namespace Core { namespace Http {
     // adding getters for version and stream body. Clang will complain on macOS if we have unused
     // fields in a class
 
+    /**
+     * @brief Get HTTP protocol major version.
+     *
+     */
+    int32_t GetMajorVersion() const { return m_majorVersion; }
+
+    /**
+     * @brief Get HTTP protocol minor version.
+     *
+     */
+    int32_t GetMinorVersion() const { return m_minorVersion; }
+
     /**
      * @brief Get HTTP status code of the HTTP response.
      *

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

@@ -874,6 +874,100 @@ CURLcode CurlSession::ReadStatusLineAndHeadersFromRawResponse(
   this->m_innerBufferSize = bufferSize;
   this->m_lastStatusCode = this->m_response->GetStatusCode();
 
+  // The logic below comes from the expectation that Azure services, particularly Storage, may not
+  // conform to HTTP standards when it comes to handling 100-continue requests, and not send
+  // "Connection: close" when they should. We do not know for sure if this is true, but this logic
+  // did exist for libcurl transport in earlier C++ SDK versions.
+  //
+  // The idea is the following: if status code is not 2xx, and request header contains "Expect:
+  // 100-continue" and request body length is not zero, we don't reuse the connection.
+  //
+  // More detailed description of what might happen if we don't have this logic:
+  // 1. Storage SDK sends a PUT request with a non-empty request body (which means Content-Length
+  // request header is not 0, let's say it's 6) and Expect: 100-continue request header, but it
+  // doesn't send the header unless server returns 100 Continue status code.
+  // 2. Storage service returns 4xx status code and response headers, but it doesn't want to close
+  // this connection, so there's no Connection: close in response headers.
+  // 3. Now both client and server agree to continue using this connection. But they do not agree in
+  // the current status of this connection.
+  //    3.1. Client side thinks the previous request is finished because it has received a status
+  //    code and response headers. It should send a new HTTP request if there's any.
+  //    3.2. Server side thinks the previous request is not finished because it hasn't received the
+  //    request body. I tend to think this is a bug of server-side.
+  // 4. Client side sends a new request, for example,
+  //    HEAD /whatever/path HTTP/1.1
+  //    host: foo.bar.com
+  //    ...
+  // 5. Server side takes the first 6 bytes (HEAD /) of the send request and thinks this is the
+  // request body of the first request and discard it.
+  // 6. Server side keeps reading the remaining data on the wire and thinks the first part
+  // (whatever/path) is an HTTP verb. It fails the request with 400 invalid verb.
+  bool non2xxAfter100ContinueWithNonzeroContentLength = false;
+  {
+    auto responseHttpCodeInt
+        = static_cast<std::underlying_type<Http::HttpStatusCode>::type>(m_lastStatusCode);
+    if (responseHttpCodeInt < 200 || responseHttpCodeInt >= 300)
+    {
+      const auto requestExpectHeader = m_request.GetHeader("Expect");
+      if (requestExpectHeader.HasValue())
+      {
+        const auto requestExpectHeaderValueLowercase
+            = Core::_internal::StringExtensions::ToLower(requestExpectHeader.Value());
+        if (requestExpectHeaderValueLowercase == "100-continue")
+        {
+          const auto requestContentLengthHeaderValue = m_request.GetHeader("Content-Length");
+          if (requestContentLengthHeaderValue.HasValue()
+              && requestContentLengthHeaderValue.Value() != "0")
+          {
+            non2xxAfter100ContinueWithNonzeroContentLength = true;
+          }
+        }
+      }
+    }
+  }
+
+  if (non2xxAfter100ContinueWithNonzeroContentLength)
+  {
+    m_httpKeepAlive = false;
+  }
+  else
+  {
+    bool hasConnectionKeepAlive = false;
+    bool hasConnectionClose = false;
+    {
+      const Core::CaseInsensitiveMap& responseHeaders = m_response->GetHeaders();
+      const auto connectionHeader = responseHeaders.find("Connection");
+      if (connectionHeader != responseHeaders.cend())
+      {
+        const std::string headerValueLowercase
+            = Core::_internal::StringExtensions::ToLower(connectionHeader->second);
+        hasConnectionKeepAlive = headerValueLowercase.find("keep-alive") != std::string::npos;
+        hasConnectionClose = headerValueLowercase.find("close") != std::string::npos;
+      }
+    }
+
+    // HTTP <=1.0 is "close" by default. HTTP 1.1 is "keep-alive" by default.
+    // The value can also be "keep-alive, close" (i.e. "both are fine"), in which case we are
+    // preferring to treat it as keep-alive.
+    // (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection)
+    // Should it come to HTTP/2 and HTTP/3, they are "keep-alive", but any response from HTTP/2 or
+    // /3 containing a "Connection" header should be considered malformed. (HTTP/2:
+    // https://httpwg.org/specs/rfc9113.html#ConnectionSpecific
+    //  HTTP/3: https://httpwg.org/specs/rfc9114.html#rfc.section.4.2)
+    if (m_response->GetMajorVersion() == 1 && m_response->GetMinorVersion() >= 1)
+    {
+      m_httpKeepAlive = (!hasConnectionClose || hasConnectionKeepAlive);
+    }
+    else if (m_response->GetMajorVersion() <= 1)
+    {
+      m_httpKeepAlive = hasConnectionKeepAlive;
+    }
+    else
+    {
+      m_httpKeepAlive = true;
+    }
+  }
+
   // For Head request, set the length of body response to 0.
   // Response will give us content-length as if we were not doing Head saying what would it be the
   // length of the body. However, Server won't send body
@@ -2129,14 +2223,11 @@ std::unique_ptr<CurlNetworkConnection> CurlConnectionPool::ExtractOrCreateCurlCo
 // first connection to be picked next time some one ask for a connection to the pool (LIFO)
 void CurlConnectionPool::MoveConnectionBackToPool(
     std::unique_ptr<CurlNetworkConnection> connection,
-    HttpStatusCode lastStatusCode)
+    bool httpKeepAlive)
 {
-  auto code = static_cast<std::underlying_type<Http::HttpStatusCode>::type>(lastStatusCode);
-  // laststatusCode = 0
-  if (code < 200 || code >= 300)
+  if (!httpKeepAlive)
   {
-    // A handler with previous response with Error can't be re-use.
-    return;
+    return; // The server has asked us to not re-use this connection.
   }
 
   if (connection->IsShutdown())

sdk/core/azure-core/src/http/curl/curl_connection_pool_private.hpp

@@ -91,11 +91,12 @@ namespace Azure { namespace Core { namespace Http { namespace _detail {
      * @brief Moves a connection back to the pool to be re-used.
      *
      * @param connection CURL HTTP connection to add to the pool.
-     * @param lastStatusCode The most recent HTTP status code received from the \p connection.
+     * @param httpKeepAlive The status of keep-alive behavior, based on HTTP protocol version and
+     * the most recent response header received through the \p connection.
      */
     void MoveConnectionBackToPool(
         std::unique_ptr<CurlNetworkConnection> connection,
-        HttpStatusCode lastStatusCode);
+        bool httpKeepAlive);
 
     /**
      * @brief Keeps a unique key for each host and creates a connection pool for each key.

sdk/core/azure-core/src/http/curl/curl_session_private.hpp

@@ -339,6 +339,13 @@ namespace Azure { namespace Core { namespace Http {
      */
     Http::HttpStatusCode m_lastStatusCode = Http::HttpStatusCode::BadRequest;
 
+    /**
+     * @brief Holds information on whether the connection can be kept alive, based on HTTP protocol
+     * version and the "Connection" HTTP header.
+     *
+     */
+    bool m_httpKeepAlive = false;
+
     /**
      * @brief check whether an end of file has been reached.
      * @return `true` if end of file has been reached; otherwise, `false`.
@@ -417,7 +424,7 @@ namespace Azure { namespace Core { namespace Http {
       if (IsEOF() && m_keepAlive && !m_connectionUpgraded)
       {
         _detail::CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool(
-            std::move(m_connection), m_lastStatusCode);
+            std::move(m_connection), m_httpKeepAlive);
       }
     }
 

sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp

@@ -73,6 +73,7 @@ namespace Azure { namespace Core { namespace Test {
         // 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
       {
@@ -110,6 +111,7 @@ namespace Azure { namespace Core { namespace Test {
         // 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<std::mutex> lock(
@@ -155,6 +157,7 @@ namespace Azure { namespace Core { namespace Test {
         // 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
@@ -219,6 +222,7 @@ namespace Azure { namespace Core { namespace Test {
         // 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(
@@ -282,7 +286,7 @@ namespace Azure { namespace Core { namespace Test {
         for (int count = 0; count < 5; count++)
         {
           CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool(
-              std::move(connections[count]), Http::HttpStatusCode::Ok);
+              std::move(connections[count]), true);
         }
       }
 
@@ -351,7 +355,7 @@ namespace Azure { namespace Core { namespace Test {
 
       //       CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool(
       //           std::unique_ptr<MockCurlNetworkConnection>(curlMock),
-      //           Azure::Core::Http::HttpStatusCode::Ok);
+      //           true);
       //     }
       //     // No need to take look here because connections are mocked to never be expired.
       //     EXPECT_EQ(
@@ -389,7 +393,7 @@ namespace Azure { namespace Core { namespace Test {
 
       //       CurlConnectionPool::g_curlConnectionPool.MoveConnectionBackToPool(
       //           std::unique_ptr<MockCurlNetworkConnection>(curlMock),
-      //           Azure::Core::Http::HttpStatusCode::Ok);
+      //           true);
 
       //       EXPECT_EQ(
       //           Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool
@@ -455,7 +459,7 @@ namespace Azure { namespace Core { namespace Test {
         }
         // move connection back to the pool
         Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool
-            .MoveConnectionBackToPool(std::move(connection), Azure::Core::Http::HttpStatusCode::Ok);
+            .MoveConnectionBackToPool(std::move(connection), true);
       }
 
       {
@@ -494,7 +498,7 @@ namespace Azure { namespace Core { namespace Test {
         }
         // move connection back to the pool
         Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool
-            .MoveConnectionBackToPool(std::move(connection), Azure::Core::Http::HttpStatusCode::Ok);
+            .MoveConnectionBackToPool(std::move(connection), true);
       }
       {
         std::lock_guard<std::mutex> lock(
@@ -532,7 +536,7 @@ namespace Azure { namespace Core { namespace Test {
         EXPECT_EQ(connection->GetConnectionKey(), expectedConnectionKey);
         // move connection back to the pool
         Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool
-            .MoveConnectionBackToPool(std::move(connection), Azure::Core::Http::HttpStatusCode::Ok);
+            .MoveConnectionBackToPool(std::move(connection), true);
       }
 
       {
@@ -570,7 +574,7 @@ namespace Azure { namespace Core { namespace Test {
         }
         // move connection back to the pool
         Azure::Core::Http::_detail::CurlConnectionPool::g_curlConnectionPool
-            .MoveConnectionBackToPool(std::move(connection), Azure::Core::Http::HttpStatusCode::Ok);
+            .MoveConnectionBackToPool(std::move(connection), true);
       }
       {
         std::lock_guard<std::mutex> lock(