From 145f6dbdb6299616af7d31a83bb42967c09f6809 Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com> Date: Wed, 10 Apr 2024 08:01:29 -0700 Subject: [PATCH] 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 --- sdk/core/azure-core/CHANGELOG.md | 8 +- .../inc/azure/core/http/raw_response.hpp | 12 ++ sdk/core/azure-core/src/http/curl/curl.cpp | 103 +++++++++++++++++- .../curl/curl_connection_pool_private.hpp | 5 +- .../src/http/curl/curl_session_private.hpp | 9 +- .../test/ut/curl_connection_pool_test.cpp | 18 +-- 6 files changed, 133 insertions(+), 22 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index d7c0a2e99..4a1b5af37 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/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) diff --git a/sdk/core/azure-core/inc/azure/core/http/raw_response.hpp b/sdk/core/azure-core/inc/azure/core/http/raw_response.hpp index d9bfcca92..46db79c02 100644 --- a/sdk/core/azure-core/inc/azure/core/http/raw_response.hpp +++ b/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. * diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 0a3b81481..8b505c698 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/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::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 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 connection, - HttpStatusCode lastStatusCode) + bool httpKeepAlive) { - auto code = static_cast::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()) 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 4560ed0c2..5bef3d1dc 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 @@ -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 connection, - HttpStatusCode lastStatusCode); + bool httpKeepAlive); /** * @brief Keeps a unique key for each host and creates a connection pool for each key. diff --git a/sdk/core/azure-core/src/http/curl/curl_session_private.hpp b/sdk/core/azure-core/src/http/curl/curl_session_private.hpp index fb77013f7..6b32f224a 100644 --- a/sdk/core/azure-core/src/http/curl/curl_session_private.hpp +++ b/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); } } 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 cad4aeb25..67661c39e 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 @@ -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 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(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(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 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 lock(