diff --git a/CHANGELOG.md b/CHANGELOG.md index 469726701..5fea049ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,9 @@ Increment the: request body on a 32-bit build [#4630](https://github.com/open-telemetry/opentelemetry-cpp/pull/4630) +* [BUG] Do not queue a curl session closed by the Retry-After cap + [#4632](https://github.com/open-telemetry/opentelemetry-cpp/pull/4632) + ## [1.29.0] 2026-09-13 * [RELEASE] Bump main branch to 1.29.0-dev (#4259) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index ce62d9f1e..6987ea5ff 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -298,8 +298,9 @@ class HttpOperation * be called when got a CURLMSG_DONE. * * @param code CURLcode + * @return true if the request was re-armed for a retry, false if the operation was cleaned up */ - void PerformCurlMessage(CURLcode code); + bool PerformCurlMessage(CURLcode code); inline CURL *GetCurlEasyHandle() noexcept { return curl_resource_.easy_handle; } diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index cb5f0800f..55435b38d 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -549,9 +549,7 @@ bool HttpClient::MaybeSpawnBackgroundThread() { // Session can not be destroyed when calling PerformCurlMessage auto hold_session = session->shared_from_this(); - operation->PerformCurlMessage(result); - - if (operation->IsRetryable()) + if (operation->PerformCurlMessage(result)) { self->pending_to_retry_sessions_.push_back(hold_session); } diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index a92299597..77686bf4b 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1562,7 +1562,7 @@ void HttpOperation::Abort() } } -void HttpOperation::PerformCurlMessage(CURLcode code) +bool HttpOperation::PerformCurlMessage(CURLcode code) { ++retry_attempts_; last_attempt_time_ = std::chrono::system_clock::now(); @@ -1668,7 +1668,10 @@ void HttpOperation::PerformCurlMessage(CURLcode code) { // Cleanup and unbind easy handle from multi handle, and finish callback Cleanup(); + return false; } + + return true; } } // namespace curl diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 66b218024..c0da5d880 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -295,6 +295,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe server_.addHandler("/get/", *this); server_.addHandler("/post/", *this); server_.addHandler("/retry/", *this); + server_.addHandler("/retry-after/", *this); server_.addHandler("/close/", *this); server_.start(); is_running_ = true; @@ -335,6 +336,14 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe response.headers["Content-Type"] = "text/plain"; response_status = 429; } + else if (request.uri == "/retry-after/") + { + std::unique_lock lk1(mtx_requests); + received_requests_.push_back(request); + response.headers["Content-Type"] = "text/plain"; + response.headers["Retry-After"] = "30"; + response_status = 429; + } else if (request.uri == "/close/") { // -1 is the documented way for a handler to ask the server to terminate the @@ -677,6 +686,90 @@ TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry) ASSERT_EQ(CURLE_OK, operation.Send()); ASSERT_FALSE(operation.IsRetryable()); } + +// A Retry-After beyond max_backoff closes the session. The IO loop used to queue it anyway, where +// it held back later retries and the background thread until the server's time. +TEST_F(BasicCurlHttpTests, RetryAfterBeyondMaxBackoffIsNotQueued) +{ + received_requests_.clear(); + curl::HttpClient http_client; + const http_client::RetryPolicy retry_policy = {2, std::chrono::duration{0.1f}, + std::chrono::duration{1.0f}, 1.0f}; + + auto capped_session = http_client.CreateSession("http://127.0.0.1:19000"); + auto capped_request = capped_session->CreateRequest(); + capped_request->SetMethod(http_client::Method::Post); + capped_request->SetUri("retry-after/"); + capped_request->SetRetryPolicy(retry_policy); + auto capped_handler = std::make_shared(); + capped_session->SendRequest(capped_handler); + capped_session->FinishSession(); + ASSERT_TRUE(capped_handler->got_response_.load(std::memory_order_acquire)); + + auto session = http_client.CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetMethod(http_client::Method::Post); + request->SetUri("retry/"); + request->SetRetryPolicy(retry_policy); + auto handler = std::make_shared(); + auto started_at = std::chrono::steady_clock::now(); + session->SendRequest(handler); + session->FinishSession(); + const auto retried_in = std::chrono::steady_clock::now() - started_at; + ASSERT_TRUE(handler->got_response_.load(std::memory_order_acquire)); + + started_at = std::chrono::steady_clock::now(); + http_client.WaitBackgroundThreadExit(); + const auto joined_in = std::chrono::steady_clock::now() - started_at; + + // The server asks for 30 s; the policy backs off for about 0.1 s. + EXPECT_TRUE(retried_in < std::chrono::seconds{10}) + << "retry ms: " << std::chrono::duration_cast(retried_in).count(); + EXPECT_TRUE(joined_in < std::chrono::seconds{10}) + << "join ms: " << std::chrono::duration_cast(joined_in).count(); + + std::unique_lock lock_requests(mtx_requests); + const auto hits = [this](const char *uri) { + return std::count_if( + received_requests_.begin(), received_requests_.end(), + [uri](const HTTP_SERVER_NS::HttpRequest &received) { return received.uri == uri; }); + }; + EXPECT_EQ(1, hits("/retry-after/")); + EXPECT_EQ(2, hits("/retry/")); +} + +// The shutdown half of #4631: the closed session used to hold the join until the server's time. +TEST_F(BasicCurlHttpTests, RetryAfterBeyondMaxBackoffDoesNotDelayShutdown) +{ + received_requests_.clear(); + curl::HttpClient http_client; + const http_client::RetryPolicy retry_policy = {2, std::chrono::duration{0.1f}, + std::chrono::duration{1.0f}, 1.0f}; + + auto session = http_client.CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetMethod(http_client::Method::Post); + request->SetUri("retry-after/"); + request->SetRetryPolicy(retry_policy); + auto handler = std::make_shared(); + session->SendRequest(handler); + session->FinishSession(); + ASSERT_TRUE(handler->got_response_.load(std::memory_order_acquire)); + + const auto started_at = std::chrono::steady_clock::now(); + http_client.WaitBackgroundThreadExit(); + const auto joined_in = std::chrono::steady_clock::now() - started_at; + + // The server asks for 30 s. + EXPECT_TRUE(joined_in < std::chrono::seconds{10}) + << "join ms: " << std::chrono::duration_cast(joined_in).count(); + + std::unique_lock lock_requests(mtx_requests); + EXPECT_EQ(1, std::count_if(received_requests_.begin(), received_requests_.end(), + [](const HTTP_SERVER_NS::HttpRequest &received) { + return received.uri == "/retry-after/"; + })); +} #endif // ENABLE_OTLP_RETRY_PREVIEW // A cancel that arrives once the server has answered used to deliver Cancelled and the response,