From 553b948b69b14ed261a9b0b366f102bdd232df46 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 06:03:16 +0000 Subject: [PATCH 01/38] [BUG] Check what curl_slist_append and curl_multi_init return Both are allocation points whose failure was treated as success, and both sit next to a check the same file already makes. curl_slist_append returns null without freeing the list it was given, and the docs say so: "To avoid overwriting an existing non-empty list on failure, the new list should be returned to a temporary variable which can be tested for NULL before updating the original list pointer." Assigning straight back over headers_chunk did the opposite. The pointer to everything appended so far went with it, so the two places that free the list saw null and never ran, and Setup() only sets CURLOPT_HTTPHEADER when headers_chunk is non-null, so the request went out with none of the caller's headers rather than not going out. For an OTLP export that means no Content-Type and a receiver that rejects a request the exporter believes it sent. Reporting that is not enough on its own. Returning from the constructor does not stop Session::SendRequest, which calls SendAsync unconditionally, and the easy handle is still valid at that point, so the request went out anyway. The failure is now recorded in the operation, both Send() and SendAsync() refuse on it before Setup(), and Session::SendRequest reports it once with the reason curl gave. The construction also marks itself terminal, otherwise Cleanup() from ~HttpOperation announces a cancel for an operation that never started, to a handler the caller may no longer be holding. curl_multi_init returns null the same way, and its result went straight into multi_handle_ in both constructors and in resetMultiHandle. Measured against libcurl 8.14.1, curl_multi_add_handle on a null multi handle reports CURLM_BAD_HANDLE, and that return is discarded today, so such a client would accept every session, add none of them, and complete none of them. The reset path is the worse of the three: it runs while recovering from a multi error, so a failure there turns the client into a black hole for the rest of its life. All three now go through one helper that logs. Both failures are covered. libcurl routes its internal allocations through the callbacks given to curl_global_init_mem, so the test suite installs a set of them and arms a thread local switch around the call under test. The switches are per allocation function rather than per call count: curl_easy_init allocates with calloc and strdup and never with malloc, while curl_slist_append uses one malloc, so failing malloc alone selects the list append and leaves the easy handle intact. curl_multi_init allocates with calloc, so the other case fails calloc instead. The hooks go in from SetUpTestSuite, since curl_global_init_mem returns CURLE_OK and quietly changes nothing once libcurl has been initialised, and a third case asserts they were installed in time so the other two cannot pass or fail for that reason. The flag those hooks set is atomic, because libcurl calls them from whichever thread is allocating, including a client's background thread. The header case asserts what the fix is actually for: the server receives no request, and exactly one terminal outcome reaches the handler. The conditional include of global_log_handler.h goes with this, since the file now logs outside the compression guard and carrying both copies is a duplicate that include-what-you-use rejects when the guard is off. Fixes #4404 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 + .../http/client/curl/http_operation_curl.h | 3 + ext/src/http/client/curl/http_client_curl.cc | 24 +- .../http/client/curl/http_operation_curl.cc | 33 ++- ext/test/http/curl_http_test.cc | 263 +++++++++++++++++- 5 files changed, 317 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc64aaef23..bff1bde47e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,9 @@ Increment the: deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents ([#4349](https://github.com/open-telemetry/opentelemetry-cpp/pull/4349)) +* [BUG] Check what curl_slist_append and curl_multi_init return instead of + treating a failed allocation as success + [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) * [CONFIGURATION] Add SDK component builder interfaces to the registry [#4358](https://github.com/open-telemetry/opentelemetry-cpp/issues/4358) 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 c57309ccd6..b5c905ee6b 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 @@ -326,6 +326,9 @@ class HttpOperation char curl_error_message_[CURL_ERROR_SIZE]{}; HttpCurlEasyResource curl_resource_; CURLcode last_curl_result_{CURLE_OK}; // Curl result OR HTTP status code if successful + // Set when the constructor could not finish. Send() and SendAsync() refuse on it, so a + // request whose setup never completed is not put on the wire. + CURLcode construction_result_{CURLE_OK}; opentelemetry::ext::http::client::EventHandler *event_handle_{nullptr}; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 76f67fcd90..813d831cdd 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -23,6 +23,7 @@ #include "opentelemetry/ext/http/common/url_parser.h" #include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/sdk/common/global_log_handler.h" #include "opentelemetry/sdk/common/thread_instrumentation.h" #include "opentelemetry/version.h" @@ -33,8 +34,6 @@ # include # include "opentelemetry/nostd/type_traits.h" -#else -# include "opentelemetry/sdk/common/global_log_handler.h" #endif OPENTELEMETRY_BEGIN_NAMESPACE @@ -236,7 +235,8 @@ void Session::SendRequest( { if (callback) { - callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, ""); + callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, + curl_easy_strerror(curl_operation_->GetLastResultCode())); } is_session_active_.store(false, std::memory_order_release); } @@ -270,8 +270,20 @@ void Session::FinishOperation() } } +// A null multi handle makes every curl_multi_add_handle report CURLM_BAD_HANDLE, so a client +// built on one accepts sessions and never sends any of them. Say so rather than fail quietly. +static CURLM *initMultiHandle() +{ + CURLM *handle = curl_multi_init(); + if (nullptr == handle) + { + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_init failed, this client cannot send"); + } + return handle; +} + HttpClient::HttpClient() - : multi_handle_(curl_multi_init()), + : multi_handle_(initMultiHandle()), next_session_id_{0}, max_sessions_per_connection_{8}, background_thread_instrumentation_(nullptr), @@ -282,7 +294,7 @@ HttpClient::HttpClient() HttpClient::HttpClient( const std::shared_ptr &thread_instrumentation) - : multi_handle_(curl_multi_init()), + : multi_handle_(initMultiHandle()), next_session_id_{0}, max_sessions_per_connection_{8}, background_thread_instrumentation_(thread_instrumentation), @@ -913,7 +925,7 @@ void HttpClient::resetMultiHandle() curl_multi_cleanup(multi_handle_); // Create a another multi handle to continue pending sessions - multi_handle_ = curl_multi_init(); + multi_handle_ = initMultiHandle(); } } // namespace curl diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 0f1bda4035..cf6958323f 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -464,8 +464,25 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, for (auto &kv : this->request_headers_) { const auto header = std::string(kv.first).append(": ").append(kv.second); - curl_resource_.headers_chunk = - curl_slist_append(curl_resource_.headers_chunk, header.c_str()); + + // Into a temporary first. curl_slist_append returns null without freeing the list it was + // given, so assigning the result straight back would drop the only pointer to everything + // appended so far, and Setup() would then send the request with none of these headers + // rather than not send it. + curl_slist *appended = curl_slist_append(curl_resource_.headers_chunk, header.c_str()); + if (nullptr == appended) + { + curl_slist_free_all(curl_resource_.headers_chunk); + curl_resource_.headers_chunk = nullptr; + last_curl_result_ = CURLE_OUT_OF_MEMORY; + construction_result_ = CURLE_OUT_OF_MEMORY; + // Terminal already, so Cleanup() does not later announce a cancel for an operation that + // never started, to a handler the caller may no longer be holding. + session_state_ = opentelemetry::ext::http::client::SessionState::CreateFailed; + return; + } + + curl_resource_.headers_chunk = appended; } } @@ -1391,6 +1408,12 @@ CURLcode HttpOperation::Setup() CURLcode HttpOperation::Send() { + if (construction_result_ != CURLE_OK) + { + last_curl_result_ = construction_result_; + return construction_result_; + } + // If it is async sending, just return error if (async_data_ && async_data_->is_promise_running.load(std::memory_order_acquire)) { @@ -1421,6 +1444,12 @@ CURLcode HttpOperation::Send() CURLcode HttpOperation::SendAsync(Session *session, std::function callback) { + if (construction_result_ != CURLE_OK) + { + last_curl_result_ = construction_result_; + return construction_result_; + } + if (nullptr == session) { return CURLE_FAILED_INIT; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 90142962d6..b0147409a5 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -16,9 +16,9 @@ #include #include #include +#include #include #include -#include #include #include #include @@ -32,7 +32,9 @@ #include "opentelemetry/ext/http/client/http_client.h" #include "opentelemetry/ext/http/server/http_server.h" #include "opentelemetry/nostd/function_ref.h" +#include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/sdk/common/global_log_handler.h" #include "opentelemetry/version.h" constexpr int HTTP_PORT{19000}; @@ -198,6 +200,162 @@ class RetryEventHandler : public CustomEventHandler } }; +// libcurl routes every internal allocation through the callbacks given to +// curl_global_init_mem, which is the only way to reach the failure returns of +// curl_slist_append and curl_multi_init. They have to be installed before libcurl is +// initialised: called afterwards the function returns CURLE_OK and quietly changes nothing. +// +// The failure switches are thread local, so arming one cannot disturb a client's background +// thread. These callbacks serve the whole binary once installed. +extern "C" { +static std::atomic g_curl_hooks_ran{false}; +static thread_local bool g_fail_curl_malloc = false; +static thread_local bool g_fail_curl_calloc = false; + +// NOLINTBEGIN(cppcoreguidelines-no-malloc,hicpp-no-malloc): these are the allocator libcurl +// is given, so reaching for the C allocation functions is the point of them. +static void *CurlTestMalloc(size_t size) +{ + g_curl_hooks_ran.store(true, std::memory_order_relaxed); + return g_fail_curl_malloc ? nullptr : std::malloc(size); +} + +static void CurlTestFree(void *ptr) +{ + std::free(ptr); +} + +static void *CurlTestRealloc(void *ptr, size_t size) +{ + return std::realloc(ptr, size); +} + +// Not routed through CurlTestMalloc on purpose. curl_easy_init allocates with strdup and calloc +// and never with malloc, while curl_slist_append uses one malloc and one strdup, so failing +// malloc alone selects the list append and leaves the easy handle alone. +static char *CurlTestStrdup(const char *str) +{ + const size_t length = std::strlen(str) + 1; + char *copy = static_cast(std::malloc(length)); + if (copy != nullptr) + { + std::memcpy(copy, str, length); + } + return copy; +} + +// curl_multi_init allocates with calloc and never with malloc, so this one selects it. +static void *CurlTestCalloc(size_t count, size_t size) +{ + g_curl_hooks_ran.store(true, std::memory_order_relaxed); + return g_fail_curl_calloc ? nullptr : std::calloc(count, size); +} +// NOLINTEND(cppcoreguidelines-no-malloc,hicpp-no-malloc) +} // extern "C" + +namespace +{ +bool g_curl_hooks_installed = false; + +struct FailCurlMalloc +{ + FailCurlMalloc() { g_fail_curl_malloc = true; } + ~FailCurlMalloc() { g_fail_curl_malloc = false; } + FailCurlMalloc(const FailCurlMalloc &) = delete; + FailCurlMalloc(FailCurlMalloc &&) = delete; + FailCurlMalloc &operator=(const FailCurlMalloc &) = delete; + FailCurlMalloc &operator=(FailCurlMalloc &&) = delete; +}; + +struct FailCurlCalloc +{ + FailCurlCalloc() { g_fail_curl_calloc = true; } + ~FailCurlCalloc() { g_fail_curl_calloc = false; } + FailCurlCalloc(const FailCurlCalloc &) = delete; + FailCurlCalloc(FailCurlCalloc &&) = delete; + FailCurlCalloc &operator=(const FailCurlCalloc &) = delete; + FailCurlCalloc &operator=(FailCurlCalloc &&) = delete; +}; + +class CapturingLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler +{ +public: + void Handle(opentelemetry::sdk::common::internal_log::LogLevel, + const char *, + int, + const char *msg, + const opentelemetry::sdk::common::AttributeMap &) noexcept override + { + if (msg == nullptr) + { + return; + } + std::lock_guard lock(messages_m_); + messages_.append(msg).append("\n"); + } + + std::string Text() + { + std::lock_guard lock(messages_m_); + return messages_; + } + +private: + std::mutex messages_m_; + std::string messages_; +}; + +class ReportedStateHandler : public CustomEventHandler +{ +public: + std::atomic create_failed_{false}; + std::atomic terminal_count_{0}; + + void OnResponse(http_client::Response &) noexcept override + { + terminal_count_.fetch_add(1, std::memory_order_acq_rel); + } + + void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override + { + switch (state) + { + case http_client::SessionState::CreateFailed: + case http_client::SessionState::ConnectFailed: + case http_client::SessionState::SendFailed: + case http_client::SessionState::SSLHandshakeFailed: + case http_client::SessionState::TimedOut: + case http_client::SessionState::NetworkError: + case http_client::SessionState::Cancelled: + terminal_count_.fetch_add(1, std::memory_order_acq_rel); + break; + default: + break; + } + + if (state != http_client::SessionState::CreateFailed) + { + return; + } + { + std::lock_guard lock(reason_m_); + reason_.assign(reason.data(), reason.size()); + } + create_failed_.store(true, std::memory_order_release); + } + + std::string Reason() + { + std::lock_guard lock(reason_m_); + return reason_; + } + +private: + std::mutex reason_m_; + std::string reason_; +}; +} // namespace + class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRequestCallback { protected: @@ -215,6 +373,15 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe public: BasicCurlHttpTests() : is_setup_(false), is_running_(false) {} + // Runs once before the first case, which is the only point still ahead of the first + // HttpClient and therefore ahead of curl_global_init. + static void SetUpTestSuite() + { + g_curl_hooks_installed = + (CURLE_OK == curl_global_init_mem(CURL_GLOBAL_ALL, CurlTestMalloc, CurlTestFree, + CurlTestRealloc, CurlTestStrdup, CurlTestCalloc)); + } + protected: void SetUp() override { @@ -689,6 +856,100 @@ TEST_F(BasicCurlHttpTests, RepeatedCallerThreadCancelsAreClean) EXPECT_GE(terminal_total, 20); } +// Without this the two cases below would fail for the wrong reason if the hooks ever stopped +// being installed early enough, and the message would not say so. +TEST_F(BasicCurlHttpTests, CurlAllocationHooksAreInstalled) +{ + EXPECT_TRUE(g_curl_hooks_installed) << "curl_global_init_mem did not return CURLE_OK"; + + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + session_manager->FinishAllSessions(); + + EXPECT_TRUE(g_curl_hooks_ran.load(std::memory_order_relaxed)) + << "libcurl allocated without calling the hooks, so they were installed too late"; +} + +// A header list that cannot be built has to end the operation. Reporting it is what stops the +// request going out with none of the caller's headers, which for an OTLP export means no +// Content-Type and a receiver that rejects it. +TEST_F(BasicCurlHttpTests, AFailedHeaderAllocationIsReported) +{ + ASSERT_TRUE(g_curl_hooks_installed); + + received_requests_.clear(); + + auto session_manager = std::make_shared()->Create(); + auto session = session_manager->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + request->AddHeader("X-Test", "1"); + + auto handler = std::make_shared(); + { + // The operation is constructed on this thread inside SendRequest, so the switch reaches + // only its allocations. + FailCurlMalloc fail; + session->SendRequest(handler); + } + + session->FinishSession(); + session_manager->FinishAllSessions(); + + size_t requests_seen = 0; + { + std::unique_lock lock_requests(mtx_requests); + requests_seen = received_requests_.size(); + } + + EXPECT_TRUE(handler->create_failed_.load(std::memory_order_acquire)) + << "a header list that could not be built was not reported"; + // Reporting it is only half. The easy handle is still valid here and Setup() skips + // CURLOPT_HTTPHEADER when the list is null, so without a construction result the request goes + // out anyway, carrying none of the caller's headers. + EXPECT_EQ(static_cast(0), requests_seen) + << "the request reached the server after the failure was reported"; + EXPECT_EQ(1, handler->terminal_count_.load(std::memory_order_acquire)) + << "expected exactly one terminal outcome"; + // A failed curl_easy_init would report "Failed initialization" instead, so this holds the + // case to the header list rather than to whichever allocation happened to fail. + const std::string reason = handler->Reason(); + EXPECT_NE(std::string::npos, reason.find("Out of memory")) << "reported as: " << reason; +} + +// A client whose multi handle is null accepts sessions, adds none of them, and completes none +// of them, so the failure has to be visible somewhere. +TEST_F(BasicCurlHttpTests, AFailedMultiHandleAllocationIsReported) +{ + ASSERT_TRUE(g_curl_hooks_installed); + + // One ordinary client first, so the global curl initializer already exists and the switch + // below can only reach curl_multi_init. + { + auto warmup = std::make_shared()->Create(); + ASSERT_TRUE(warmup != nullptr); + warmup->FinishAllSessions(); + } + + auto *capture = new CapturingLogHandler(); + auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( + nostd::shared_ptr(capture)); + + { + FailCurlCalloc fail; + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + client->FinishAllSessions(); + } + + const std::string text = capture->Text(); + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); + + EXPECT_NE(std::string::npos, text.find("curl_multi_init failed")) + << "a multi handle that could not be created was not reported, captured: " << text; +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear(); From 6872752dbdee30349dbbdbf79990bbf99b8a8ddd Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:25:16 +0000 Subject: [PATCH 02/38] [TEST] Make the allocation failure case independent of the libcurl in use Which libcurl call consumes the first failing malloc is a property of the libcurl the tests are built against, not of this repository. On 8.14.1 it is curl_slist_append, which is what the case wants. On the libcurl the conan jobs build, curl_easy_init takes it first, reports Curl_open failed, and leaves down a different path with three terminal outcomes and no out of memory message. The failure is bounded by size now. A curl_slist node is two pointers and a curl easy handle is thousands of bytes on every version, so the bound aims it at the list append. That is not a guarantee, so the reason check became a skip that says which allocation failed instead of an assertion against a path the build never took. curl/curl.h moves out of the retry preview guard and into the unconditional block. The scaffolding calls curl_global_init_mem from every configuration, so include-what-you-use asked for it on all-options-abiv1 where that guard is off, and asked for the guarded copy to go on the two preview variants where it is on. All three report correct includes with one unconditional include. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index b0147409a5..109c2cc318 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1,11 +1,11 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #include #include "gtest/gtest.h" #ifdef ENABLE_OTLP_RETRY_PREVIEW -# include # include "gmock/gmock.h" #endif // ENABLE_OTLP_RETRY_PREVIEW @@ -214,10 +214,19 @@ static thread_local bool g_fail_curl_calloc = false; // NOLINTBEGIN(cppcoreguidelines-no-malloc,hicpp-no-malloc): these are the allocator libcurl // is given, so reaching for the C allocation functions is the point of them. +// A curl_slist node is two pointers, and a curl easy handle is thousands of bytes on every +// libcurl, so the bound aims the failure at the list append. Which call consumes the first +// failing allocation is otherwise a property of the libcurl in use rather than of this test. +static const size_t kCurlSmallAllocation = 64; + static void *CurlTestMalloc(size_t size) { g_curl_hooks_ran.store(true, std::memory_order_relaxed); - return g_fail_curl_malloc ? nullptr : std::malloc(size); + if (g_fail_curl_malloc && size <= kCurlSmallAllocation) + { + return nullptr; + } + return std::malloc(size); } static void CurlTestFree(void *ptr) @@ -230,9 +239,8 @@ static void *CurlTestRealloc(void *ptr, size_t size) return std::realloc(ptr, size); } -// Not routed through CurlTestMalloc on purpose. curl_easy_init allocates with strdup and calloc -// and never with malloc, while curl_slist_append uses one malloc and one strdup, so failing -// malloc alone selects the list append and leaves the easy handle alone. +// Not routed through CurlTestMalloc on purpose, so that a failing malloc cannot reach the +// copies libcurl makes of the caller's strings and land somewhere other than the list node. static char *CurlTestStrdup(const char *str) { const size_t length = std::strlen(str) + 1; @@ -909,12 +917,19 @@ TEST_F(BasicCurlHttpTests, AFailedHeaderAllocationIsReported) // out anyway, carrying none of the caller's headers. EXPECT_EQ(static_cast(0), requests_seen) << "the request reached the server after the failure was reported"; + + // Everything past here is specific to the header list having been the allocation that failed. + // A libcurl that took the failure somewhere else reports its own message, and says so rather + // than asserting against a path it did not take. + const std::string reason = handler->Reason(); + if (std::string::npos == reason.find("Out of memory")) + { + GTEST_SKIP() << "this libcurl consumed the failing allocation before the header list, " + << "reported as: " << reason; + } + EXPECT_EQ(1, handler->terminal_count_.load(std::memory_order_acquire)) << "expected exactly one terminal outcome"; - // A failed curl_easy_init would report "Failed initialization" instead, so this holds the - // case to the header list rather than to whichever allocation happened to fail. - const std::string reason = handler->Reason(); - EXPECT_NE(std::string::npos, reason.find("Out of memory")) << "reported as: " << reason; } // A client whose multi handle is null accepts sessions, adds none of them, and completes none From 6e5a495612fb0a5c83ddcb823a9ee76b10c3403d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:52:00 +0000 Subject: [PATCH 03/38] [TEST] Hold what a client without a multi handle does with a request Reporting the curl_multi_init failure does not by itself stop the client taking requests, so the case now holds what taking one leads to rather than only that the failure was reported. Raised by @lalitb in review. Measured before choosing an assertion. A client built on a null multi handle answers normally: curl_multi_perform rejects the handle, the IO loop resets it, the pending session moves to the new one and the request completes, three runs out of three with the internal log confirming the handle really was null. Gate resetMultiHandle out and the same case fails on the new assertion and then hangs, which is the outcome the assertion exists to catch. So the case asserts the caller reaches a terminal outcome and the session does not stay active, without pinning which outcome. A build where the allocation is still failing when the reset runs reports a failure instead of a response, and both satisfy the property that matters. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 71 +++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 109c2cc318..67a4d51b03 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -285,6 +285,35 @@ struct FailCurlCalloc FailCurlCalloc &operator=(FailCurlCalloc &&) = delete; }; +// Counts terminal outcomes without caring which one, since a client whose multi handle could +// not be created may still recover and answer, and the case below is about the caller being +// told either way rather than about which answer it gets. +class MultiHandleOutcomeHandler : public http_client::EventHandler +{ +public: + void OnResponse(http_client::Response & /* response */) noexcept override + { + terminal_.fetch_add(1, std::memory_order_release); + } + + void OnEvent(http_client::SessionState state, nostd::string_view /* reason */) noexcept override + { + switch (state) + { + case http_client::SessionState::CreateFailed: + case http_client::SessionState::ConnectFailed: + case http_client::SessionState::SendFailed: + case http_client::SessionState::Cancelled: + terminal_.fetch_add(1, std::memory_order_release); + break; + default: + break; + } + } + + std::atomic terminal_{0}; +}; + class CapturingLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler { public: @@ -965,6 +994,48 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleAllocationIsReported) << "a multi handle that could not be created was not reported, captured: " << text; } +// Reporting the failure does not by itself stop the client taking requests, so this holds what +// taking one leads to. The IO loop resets the multi handle whenever curl_multi_perform rejects +// it, so a client built on a null handle repairs itself and answers; if the allocation is still +// failing by then it reports a failure instead. What must not happen is neither. +TEST_F(BasicCurlHttpTests, AClientWithoutAMultiHandleStillAnswers) +{ + ASSERT_TRUE(g_curl_hooks_installed); + received_requests_.clear(); + + { + auto warmup = std::make_shared()->Create(); + ASSERT_TRUE(warmup != nullptr); + warmup->FinishAllSessions(); + } + + std::shared_ptr client; + { + FailCurlCalloc fail; + client = std::make_shared()->Create(); + } + ASSERT_TRUE(client != nullptr); + + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + session->SendRequest(handler); + + for (int i = 0; i < 300 && 0 == handler->terminal_.load(std::memory_order_acquire); ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + + EXPECT_GE(handler->terminal_.load(std::memory_order_acquire), 1) + << "the request was accepted and never reached an outcome"; + EXPECT_FALSE(session->IsSessionActive()) << "the session stayed active with nothing running it"; + + session->FinishSession(); + client->FinishAllSessions(); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear(); From 59f550d0d563675dc7127b20a763f8b7b95174cf Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:07:16 +0000 Subject: [PATCH 04/38] [TEST] Cover the synchronous refusal and the instrumented constructor Codecov had three patch lines nothing reached. Both are reachable. Send() carries the same construction_result_ guard as SendAsync, and nothing called it after a failed construction. The case builds an operation whose header list cannot be allocated and calls Send(): without the guard the request goes out carrying none of its headers and the server sees it, five runs out of five, so the case fails rather than merely covering the line. The other line is the multi handle in the constructor that takes thread instrumentation. Both constructors reach it through the same helper, so the case uses that overload with a null instrumentation and holds that it reports a handle it could not create, the same way the plain one does. Patch coverage for this branch measured with the abiv2-preview configuration the Codecov job uses: three missing lines before, none after. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 77 +++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 67a4d51b03..ea4844ac96 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -16,9 +16,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -35,6 +37,7 @@ #include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/global_log_handler.h" +#include "opentelemetry/sdk/common/thread_instrumentation.h" #include "opentelemetry/version.h" constexpr int HTTP_PORT{19000}; @@ -994,6 +997,80 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleAllocationIsReported) << "a multi handle that could not be created was not reported, captured: " << text; } +// SendAsync refuses a request whose header list could not be built, and Send has to refuse it +// the same way, or a synchronous caller puts one on the wire carrying none of its headers. +TEST_F(BasicCurlHttpTests, ASynchronousSendRefusesAFailedHeaderAllocation) +{ + ASSERT_TRUE(g_curl_hooks_installed); + received_requests_.clear(); + + // The operation keeps references to these, so they outlive it. + const http_client::HttpSslOptions no_ssl; + const http_client::Body body; + const http_client::Headers headers = {{"X-Test", "1"}}; + const http_client::Compression compression = http_client::Compression::kNone; + + std::unique_ptr operation; + { + // Only the constructor allocates under the switch. Send needs its own allocations to work. + FailCurlMalloc fail; + operation.reset(new curl::HttpOperation(http_client::Method::Get, "http://127.0.0.1:19000/get/", + no_ssl, nullptr, headers, body, compression)); + } + + const CURLcode result = operation->Send(); + + size_t requests_seen = 0; + { + std::unique_lock lock_requests(mtx_requests); + requests_seen = received_requests_.size(); + } + + EXPECT_EQ(static_cast(0), requests_seen) + << "a synchronous request went out after its setup had failed"; + + if (CURLE_OUT_OF_MEMORY != result) + { + GTEST_SKIP() << "this libcurl consumed the failing allocation before the header list, " + << "reported as: " << curl_easy_strerror(result); + } + EXPECT_EQ(CURLE_OUT_OF_MEMORY, operation->GetLastResultCode()) + << "the refusal was not recorded as the last result"; +} + +// Both constructors reach the multi handle through the same helper, so the overload that takes +// thread instrumentation reports a failed one too. A null instrumentation is enough to pick it. +TEST_F(BasicCurlHttpTests, AFailedMultiHandleIsReportedForAnInstrumentedClient) +{ + ASSERT_TRUE(g_curl_hooks_installed); + + { + auto warmup = std::make_shared()->Create(); + ASSERT_TRUE(warmup != nullptr); + warmup->FinishAllSessions(); + } + + auto *capture = new CapturingLogHandler(); + auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( + nostd::shared_ptr(capture)); + + { + FailCurlCalloc fail; + auto client = std::make_shared()->Create( + std::shared_ptr{}); + ASSERT_TRUE(client != nullptr); + client->FinishAllSessions(); + } + + const std::string text = capture->Text(); + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); + + EXPECT_NE(std::string::npos, text.find("curl_multi_init failed")) + << "the instrumented constructor did not report a multi handle it could not create, " + << "captured: " << text; +} + // Reporting the failure does not by itself stop the client taking requests, so this holds what // taking one leads to. The IO loop resets the multi handle whenever curl_multi_perform rejects // it, so a client built on a null handle repairs itself and answers; if the allocation is still From a1f341924a5a9a5bf339352105371a552ec64655 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:43:31 +0000 Subject: [PATCH 05/38] [BUG] Stop the IO thread spinning when the multi handle cannot be created Raised by @lalitb in review, who asked whether the case would hang rather than fail if the recovery were broken. It would, and following that down found the reason, which is not in the case. curl_multi_perform does not write still_running when it rejects the handle, and the loop initialises it to one. So a client whose multi handle is null keeps reporting work it does not have: the loop takes the still_running > 0 branch every pass, never reaches the shutdown check below it, and never waits, so the thread spins a core and ~HttpClient blocks in join for good. Gating curl_multi_init to fail persistently, the whole test binary hangs, twice out of twice at sixty seconds and once at two minutes. With still_running cleared on that branch it finishes in 521 ms, three runs out of three, and the case passes rather than hanging, because the reset then cancels the session and the caller is told. A multi handle that fails once and then works is unaffected: the reset creates one, doAddSessions puts the pending session on it and sets still_running again, and the request completes with a response, three runs out of three with the internal log confirming the first handle really was null. The case also cancels before it finishes, so an assertion failure ends the case rather than leaving FinishSession waiting on an operation nothing will complete. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ ext/src/http/client/curl/http_client_curl.cc | 4 ++++ ext/test/http/curl_http_test.cc | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bff1bde47e..496dfcdd87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,9 @@ Increment the: * [BUG] Check what curl_slist_append and curl_multi_init return instead of treating a failed allocation as success [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) +* [BUG] Stop the curl IO thread spinning, and the client refusing to be + destroyed, when the multi handle cannot be created + [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) * [CONFIGURATION] Add SDK component builder interfaces to the registry [#4358](https://github.com/open-telemetry/opentelemetry-cpp/issues/4358) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 813d831cdd..aab4fd15f3 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -482,6 +482,10 @@ bool HttpClient::MaybeSpawnBackgroundThread() // can not curl_multi_perform it again if (mc != CURLM_OK) { + // curl_multi_perform leaves still_running alone when it rejects the handle, and it + // starts at one, so without this the loop keeps reporting work it does not have, + // never reaches the shutdown check below, and the thread cannot be joined. + still_running = 0; self->resetMultiHandle(); } else if (still_running || need_wait_more) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index ea4844ac96..35c7ba6734 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1109,6 +1109,10 @@ TEST_F(BasicCurlHttpTests, AClientWithoutAMultiHandleStillAnswers) << "the request was accepted and never reached an outcome"; EXPECT_FALSE(session->IsSessionActive()) << "the session stayed active with nothing running it"; + // Cancel before finishing. If either assertion above failed then nothing is going to complete + // this operation, and FinishSession would wait for it for good, so the case would hang rather + // than fail. Cancelling a session that already answered does nothing. + session->CancelSession(); session->FinishSession(); client->FinishAllSessions(); } From bc3dc0759dcbf9e37e4b04e323639ed8ecca8c3d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:32:05 +0000 Subject: [PATCH 06/38] [TEST] Take the include set include-what-you-use asks for Three jobs went red on the test file. Two of the includes were mine and one of them should never have been there: was left behind by a probe whose prints are gone, and the file has no stdio use at all. goes because the thread instrumentation header the new case needs already reaches it, and that header is the one include-what-you-use asks to spell out. Checked against the three cache files the jobs use, with CMAKE_CXX_STANDARD=14 and WITH_STL=CXX14 on top of them, which is what ci/do_ci.sh cmake.iwyu.test passes. Nine combinations of variant and translation unit, all reporting correct includes. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 35c7ba6734..c42c9a5ce9 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -16,11 +16,9 @@ #include #include #include -#include #include #include #include -#include #include #include #include From d9034527b1e2443f83ea7dd2a5ee1aa0ae0b38be Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:01:56 +0000 Subject: [PATCH 07/38] [BUG] Stop the IO thread spinning while it has no multi handle Clearing still_running stopped a client that could not create a multi handle being undestroyable, but it left the worse half in place. With the client alive and curl_multi_init failing every time, the loop retried and reported as fast as the CPU allowed: 3.00 seconds of CPU and 1,168,126 error lines over a three second window, three runs out of three. Three parts to it. A missing handle is answered without calling into libcurl, which is what curl_multi_init asks for: once it has returned null the other multi functions cannot be used, so treating the rejection from curl_multi_perform as the recovery trigger was relying on an implementation detail. resetMultiHandle returns whether the client has a handle afterwards, since the loop has nothing to run while it does not. And a run of failures is reported once and waited on for as long as a poll would have taken, rather than repeated every pass. Shutdown skips the wait, so teardown is unchanged. The same window now costs 0.00 seconds of CPU and two log lines, one from the constructor and one from the loop. A handle that fails once still recovers on the next pass and the request completes. APersistentMultiHandleFailureDoesNotSpin holds it. The switch it uses is not thread_local, which is what reaches the re-initialization on the IO thread; the existing case only fails the constructor, so the first reset succeeded and the branch was never entered. Without this change it reports 0.74 seconds of CPU over a one second window and 284,905 repetitions of one message, three runs out of three. Raised by @lalitb, who asked whether logging the failure changes anything. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 2 +- .../ext/http/client/curl/http_client_curl.h | 4 +- ext/src/http/client/curl/http_client_curl.cc | 50 ++++++-- ext/test/http/curl_http_test.cc | 108 +++++++++++++++++- 4 files changed, 145 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 496dfcdd87..1d2e838fe0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,7 +74,7 @@ Increment the: * [BUG] Check what curl_slist_append and curl_multi_init return instead of treating a failed allocation as success [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) -* [BUG] Stop the curl IO thread spinning, and the client refusing to be +* [BUG] Stop the curl IO thread spinning, flooding the log, and refusing to be destroyed, when the multi handle cannot be created [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 9a09fac9aa..bb73a09245 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -364,7 +364,9 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doAbortSessions(); bool doRemoveSessions(); bool doRetrySessions(bool report_all); - void resetMultiHandle(); + // Returns whether the client has a multi handle afterwards. The IO loop has nothing to + // run while it does not, and needs to know rather than call into libcurl regardless. + bool resetMultiHandle(); std::mutex multi_handle_m_; CURLM *multi_handle_; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index aab4fd15f3..2c292fc3d9 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -270,14 +270,16 @@ void Session::FinishOperation() } } -// A null multi handle makes every curl_multi_add_handle report CURLM_BAD_HANDLE, so a client -// built on one accepts sessions and never sends any of them. Say so rather than fail quietly. +// Reported once, where it happens. The IO loop does its own reporting, because sharing this one +// would repeat the same line on every pass for as long as the handle stays missing. static CURLM *initMultiHandle() { CURLM *handle = curl_multi_init(); if (nullptr == handle) { - OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_init failed, this client cannot send"); + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_init failed, requests cannot be processed until it " + "succeeds"); } return handle; } @@ -470,14 +472,21 @@ bool HttpClient::MaybeSpawnBackgroundThread() } #endif /* ENABLE_THREAD_INSTRUMENTATION_PREVIEW */ - auto still_running = 1; - auto last_free_job_timepoint = std::chrono::system_clock::now(); - auto need_wait_more = false; + auto still_running = 1; + auto last_free_job_timepoint = std::chrono::system_clock::now(); + auto need_wait_more = false; + bool missing_multi_handle_reported = false; while (true) { CURLMsg *msg = nullptr; int queued = 0; - CURLMcode mc = curl_multi_perform(self->multi_handle_, &still_running); + // curl_multi_init says the other multi functions cannot be used once it has returned + // null, so a missing handle is answered here rather than passed to libcurl. + CURLMcode mc = CURLM_BAD_HANDLE; + if (nullptr != self->multi_handle_) + { + mc = curl_multi_perform(self->multi_handle_, &still_running); + } // According to https://curl.se/libcurl/c/curl_multi_perform.html, when mc is not OK, we // can not curl_multi_perform it again if (mc != CURLM_OK) @@ -486,7 +495,24 @@ bool HttpClient::MaybeSpawnBackgroundThread() // starts at one, so without this the loop keeps reporting work it does not have, // never reaches the shutdown check below, and the thread cannot be joined. still_running = 0; - self->resetMultiHandle(); + if (self->resetMultiHandle()) + { + missing_multi_handle_reported = false; + } + else if (!self->is_shutdown_.load(std::memory_order_acquire)) + { + // Nothing can run without a handle. Retrying at once pegs a core and repeats one + // error for the whole idle window, so report the run of failures once and wait as + // long as a poll would have. Shutdown skips the wait so teardown stays prompt. + if (!missing_multi_handle_reported) + { + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] no multi handle, requests cannot be processed until " + "curl_multi_init succeeds"); + missing_multi_handle_reported = true; + } + std::this_thread::sleep_for(self->scheduled_delay_milliseconds_); + } } else if (still_running || need_wait_more) { @@ -897,7 +923,7 @@ bool HttpClient::doRetrySessions(bool /* report_all */) } #endif // ENABLE_OTLP_RETRY_PREVIEW -void HttpClient::resetMultiHandle() +bool HttpClient::resetMultiHandle() { std::list> sessions; { @@ -928,8 +954,10 @@ void HttpClient::resetMultiHandle() std::lock_guard lock_guard{multi_handle_m_}; curl_multi_cleanup(multi_handle_); - // Create a another multi handle to continue pending sessions - multi_handle_ = initMultiHandle(); + // Create a another multi handle to continue pending sessions. Silent on failure: the caller + // decides how often a run of failures is worth reporting. + multi_handle_ = curl_multi_init(); + return nullptr != multi_handle_; } } // namespace curl diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index c42c9a5ce9..cc1018af61 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -253,11 +254,21 @@ static char *CurlTestStrdup(const char *str) return copy; } -// curl_multi_init allocates with calloc and never with malloc, so this one selects it. +// Aimed at curl_multi_init, which allocates with calloc on the libcurl this was measured +// against. Which internal allocation a given libcurl uses is not part of its contract, so +// the cases that rely on this check what actually failed rather than assume. +// Not thread_local, unlike the switches above. The case that keeps a client alive while the +// multi handle cannot be created needs the failure to reach the IO thread as well. +static std::atomic g_fail_curl_calloc_everywhere{false}; + static void *CurlTestCalloc(size_t count, size_t size) { g_curl_hooks_ran.store(true, std::memory_order_relaxed); - return g_fail_curl_calloc ? nullptr : std::calloc(count, size); + if (g_fail_curl_calloc || g_fail_curl_calloc_everywhere.load(std::memory_order_relaxed)) + { + return nullptr; + } + return std::calloc(count, size); } // NOLINTEND(cppcoreguidelines-no-malloc,hicpp-no-malloc) } // extern "C" @@ -289,6 +300,23 @@ struct FailCurlCalloc // Counts terminal outcomes without caring which one, since a client whose multi handle could // not be created may still recover and answer, and the case below is about the caller being // told either way rather than about which answer it gets. +// Counts internal log lines, so a case can hold that a run of failures is reported a bounded +// number of times rather than once per pass of the IO loop. +class CountingLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler +{ +public: + void Handle(opentelemetry::sdk::common::internal_log::LogLevel /* level */, + const char * /* file */, + int /* line */, + const char * /* msg */, + const opentelemetry::sdk::common::AttributeMap & /* attributes */) noexcept override + { + count_.fetch_add(1, std::memory_order_relaxed); + } + + std::atomic count_{0}; +}; + class MultiHandleOutcomeHandler : public http_client::EventHandler { public: @@ -1070,10 +1098,11 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleIsReportedForAnInstrumentedClient) } // Reporting the failure does not by itself stop the client taking requests, so this holds what -// taking one leads to. The IO loop resets the multi handle whenever curl_multi_perform rejects -// it, so a client built on a null handle repairs itself and answers; if the allocation is still -// failing by then it reports a failure instead. What must not happen is neither. -TEST_F(BasicCurlHttpTests, AClientWithoutAMultiHandleStillAnswers) +// taking one leads to. The IO loop creates a new multi handle when curl_multi_perform rejects +// the one it has, so a client built on a null handle usually recovers and answers, and reports a +// failure instead when the allocation is still failing by then. Either is fine. Neither is not, +// and that is what this checks, so it does not assert which one arrives. +TEST_F(BasicCurlHttpTests, AClientWithoutAMultiHandleReachesATerminalOutcome) { ASSERT_TRUE(g_curl_hooks_installed); received_requests_.clear(); @@ -1516,4 +1545,71 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) } #endif // ENABLE_OTLP_COMPRESSION_PREVIEW +// A client whose multi handle can never be created has nothing to run, and used to say so as +// fast as the CPU allowed: measured at a full core and 1,168,126 error lines over three seconds +// with the client alive. The IO loop reports a run of failures once and waits between attempts +// now, and this holds both. +TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) +{ + ASSERT_TRUE(g_curl_hooks_installed); + received_requests_.clear(); + + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + + // One completed request, so the IO thread exists and the loop below is the one under test. + { + auto warm = client->CreateSession("http://127.0.0.1:19000"); + auto warm_request = warm->CreateRequest(); + warm_request->SetUri("get/"); + auto warm_handler = std::make_shared(); + warm->SendRequest(warm_handler); + ASSERT_TRUE(waitForRequests(30, 1)); + warm->FinishSession(); + ASSERT_GE(warm_handler->terminal_.load(std::memory_order_acquire), 1); + } + + auto *capture = new CountingLogHandler(); + auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( + nostd::shared_ptr(capture)); + + const std::clock_t cpu_before = std::clock(); + { + // The hooks serve libcurl only, so this fails curl_multi_init on the IO thread without + // touching the allocations the rest of the binary makes. + g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); + auto *concrete = static_cast(client.get()); + http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); + std::this_thread::sleep_for(std::chrono::seconds(1)); + g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); + } + const double cpu_seconds = static_cast(std::clock() - cpu_before) / CLOCKS_PER_SEC; + const int log_lines = capture->count_.load(std::memory_order_relaxed); + + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); + + EXPECT_LT(cpu_seconds, 0.5) << "the IO thread spun while it had no multi handle, " << cpu_seconds + << " seconds of CPU over a one second window"; + EXPECT_LE(log_lines, 8) << "the same failure was reported " << log_lines << " times"; + + // And it recovers once allocation works again, so the wait is a wait rather than a stop. + received_requests_.clear(); + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); + session->SendRequest(handler); + for (int i = 0; i < 300 && 0 == handler->terminal_.load(std::memory_order_acquire); ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + EXPECT_GE(handler->terminal_.load(std::memory_order_acquire), 1) + << "the client did not come back once curl_multi_init could succeed again"; + + session->CancelSession(); + session->FinishSession(); + client->FinishAllSessions(); +} + } // namespace From fe0d1ceaba095836436d220abfbdc8ffc66efac4 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:14:20 +0000 Subject: [PATCH 08/38] [BUG] Create the multi handle after curl_global_init has run Members are initialised in declaration order, and multi_handle_ is declared before curl_global_initializer_, so both constructors called curl_multi_init before HttpCurlGlobalInitializer had called curl_global_init. libcurl asks for the opposite: curl_global_init has to have been called before any other libcurl function. Nothing in CI could see it. The allocation cases install their allocators from SetUpTestSuite with curl_global_init_mem, which initialises libcurl before any client exists, so every test runs against an already initialised library. The handle is created in the constructor body instead, where every member, including the one that runs curl_global_init, has been constructed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 2c292fc3d9..609b3bfd78 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -285,25 +285,35 @@ static CURLM *initMultiHandle() } HttpClient::HttpClient() - : multi_handle_(initMultiHandle()), + : multi_handle_(nullptr), next_session_id_{0}, max_sessions_per_connection_{8}, background_thread_instrumentation_(nullptr), scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) -{} +{ + // Members are initialised in declaration order, and multi_handle_ is declared first, so this + // cannot go in the list: curl_global_init has to have run before any other libcurl call, and + // it runs from curl_global_initializer_ above. + multi_handle_ = initMultiHandle(); +} HttpClient::HttpClient( const std::shared_ptr &thread_instrumentation) - : multi_handle_(initMultiHandle()), + : multi_handle_(nullptr), next_session_id_{0}, max_sessions_per_connection_{8}, background_thread_instrumentation_(thread_instrumentation), scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) -{} +{ + // Members are initialised in declaration order, and multi_handle_ is declared first, so this + // cannot go in the list: curl_global_init has to have run before any other libcurl call, and + // it runs from curl_global_initializer_ above. + multi_handle_ = initMultiHandle(); +} HttpClient::~HttpClient() { From be0d3fdcb44f4762df36c8c400e74389e3581cf6 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:16:35 +0000 Subject: [PATCH 09/38] [TEST] Describe what the case holds rather than what it used to find The comment reached for the measurement that motivated it, which is the PR and the commit message's job. What a reader of the file needs is the property. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index cc1018af61..f128b82e93 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1545,10 +1545,9 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) } #endif // ENABLE_OTLP_COMPRESSION_PREVIEW -// A client whose multi handle can never be created has nothing to run, and used to say so as -// fast as the CPU allowed: measured at a full core and 1,168,126 error lines over three seconds -// with the client alive. The IO loop reports a run of failures once and waits between attempts -// now, and this holds both. +// A client whose multi handle can never be created has nothing to run. The IO loop reports a run +// of failures once and waits between attempts, so a client left alive in that state costs neither +// a core nor a log line per pass, and this holds both. TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) { ASSERT_TRUE(g_curl_hooks_installed); From 7599add6fde77de6be60fdf27179619aa3e15d95 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:49:45 +0000 Subject: [PATCH 10/38] [BUG] Wait in slices, and keep the null handle out of curl_multi_info_read Two corrections to the change before it, both found reviewing it rather than reported. The wait cannot be interrupted. wakeupBackgroundThread reaches the worker through the multi handle, and the whole point of that branch is that there is not one, so the destructor cannot cut the wait short and the previous commit saying teardown was unchanged is wrong. Destroying a client whose worker was inside the wait measured 168 ms, five runs out of five. Taking the same wait in 16 ms slices and rechecking is_shutdown_ leaves the retry rate where it was and brings that to 10 ms, five runs out of five. And curl_multi_info_read was still called with the handle libcurl had refused to give. curl_multi_poll and curl_multi_wait cannot see one, since they sit inside the branch that only runs when perform succeeded. curl_multi_add_handle and curl_multi_remove_handle can, but they are in doAddSessions and doRemoveSessions, which #4395 and #4405 rewrite, so they are named in the description rather than changed here. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 609b3bfd78..6a009ef314 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -521,7 +521,17 @@ bool HttpClient::MaybeSpawnBackgroundThread() "curl_multi_init succeeds"); missing_multi_handle_reported = true; } - std::this_thread::sleep_for(self->scheduled_delay_milliseconds_); + // In slices, because this thread cannot be woken: wakeupBackgroundThread reaches it + // through the multi handle, and there is not one. A whole delay here would be a + // whole delay added to destroying the client. + constexpr std::chrono::milliseconds kMissingHandleWaitSlice{16}; + for (std::chrono::milliseconds waited = std::chrono::milliseconds::zero(); + waited < self->scheduled_delay_milliseconds_ && + !self->is_shutdown_.load(std::memory_order_acquire); + waited += kMissingHandleWaitSlice) + { + std::this_thread::sleep_for(kMissingHandleWaitSlice); + } } } else if (still_running || need_wait_more) @@ -556,7 +566,9 @@ bool HttpClient::MaybeSpawnBackgroundThread() do { - msg = curl_multi_info_read(self->multi_handle_, &queued); + msg = (nullptr == self->multi_handle_) + ? nullptr + : curl_multi_info_read(self->multi_handle_, &queued); if (msg == nullptr) { break; From 6dfd4b1f8a4c78bbc17951cf1be4e2cc4dd112ae Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:22:33 +0000 Subject: [PATCH 11/38] [BUG] Keep the phases that need a multi handle from running without one Guarding curl_multi_perform and curl_multi_info_read was not enough. Three more phases in the same pass reach a multi function, and doAddSessions is the one that does damage: it swaps the whole pending-to-add set out unconditionally, calls curl_multi_add_handle without looking at the handle or the result, and returns true regardless. resetMultiHandle builds its cancel snapshot from the sessions that are NOT pending to add, so a request made while the handle cannot be created is taken out of the set that protects it, reported as running, and cancelled on the next pass. doRemoveSessions and doRetrySessions reach curl_multi_remove_handle and curl_multi_add_handle the same way. All three are gated on having a handle now. doAbortSessions is not gated: it finishes operations and calls nothing from the multi interface, so teardown keeps working while the handle is missing. What this does not have is a case that fails without it. Reaching the state needs curl_multi_init to fail while curl_easy_init still works, and they allocate the same way, so the process wide switch the other cases use refuses both and the request is turned away during construction rather than queued. Three attempts at separating them either measured nothing or left a case that timed out one run in six, so the gating rests on reading doAddSessions rather than on a test. A worker owned failpoint for curl_multi_init would close that, and it needs a seam in the client rather than in the tests. The persistent failure case is unchanged in what it holds, but counts refused allocations rather than process CPU time, since std::clock is elapsed wall time on Microsoft's CRT and the one second wait would be counted there. It also has lower bounds now: without them an injection that stopped working reads as a pass. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 23 ++++++--- ext/test/http/curl_http_test.cc | 52 ++++++++++---------- 2 files changed, 42 insertions(+), 33 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 6a009ef314..b35d557a8d 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -598,26 +598,33 @@ bool HttpClient::MaybeSpawnBackgroundThread() } } while (true); - // Abort all pending easy handles + // Abort all pending easy handles. This one calls nothing from the multi interface, so + // it runs whether or not there is a handle and teardown keeps working without one. if (self->doAbortSessions()) { still_running = 1; } + // The three below every call curl_multi_add_handle or curl_multi_remove_handle. Running + // them without a handle would take the pending sessions out of the queue that keeps + // them safe from the next reset, hand them to a multi function that cannot accept + // them, and report the transfer as running. + const bool multi_available = (nullptr != self->multi_handle_); + // Remove all pending easy handles - if (self->doRemoveSessions()) + if (multi_available && self->doRemoveSessions()) { still_running = 1; } // Add all pending easy handles - if (self->doAddSessions()) + if (multi_available && self->doAddSessions()) { still_running = 1; } // Check if pending easy handles can be retried - if (self->doRetrySessions(false)) + if (multi_available && self->doRetrySessions(false)) { still_running = 1; } @@ -660,20 +667,22 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + const bool multi_available_now = (nullptr != self->multi_handle_); + // Remove all pending easy handles - if (self->doRemoveSessions()) + if (multi_available_now && self->doRemoveSessions()) { still_running = 1; } // Add all pending easy handles - if (self->doAddSessions()) + if (multi_available_now && self->doAddSessions()) { still_running = 1; } // Check if pending easy handles can be retried - if (self->doRetrySessions(true)) + if (multi_available_now && self->doRetrySessions(true)) { still_running = 1; } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index f128b82e93..a465a362a0 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -18,7 +18,6 @@ #include #include #include -#include #include #include #include @@ -258,13 +257,22 @@ static char *CurlTestStrdup(const char *str) // against. Which internal allocation a given libcurl uses is not part of its contract, so // the cases that rely on this check what actually failed rather than assume. // Not thread_local, unlike the switches above. The case that keeps a client alive while the -// multi handle cannot be created needs the failure to reach the IO thread as well. +// multi handle cannot be created needs the failure to reach the IO thread. static std::atomic g_fail_curl_calloc_everywhere{false}; +// Counts what the process wide switch refused, which is one per curl_multi_init the IO thread +// tried while it had no handle. That is the retry rate, measured the same way everywhere. +static std::atomic g_curl_calloc_failures{0}; + static void *CurlTestCalloc(size_t count, size_t size) { g_curl_hooks_ran.store(true, std::memory_order_relaxed); - if (g_fail_curl_calloc || g_fail_curl_calloc_everywhere.load(std::memory_order_relaxed)) + if (g_fail_curl_calloc_everywhere.load(std::memory_order_relaxed)) + { + g_curl_calloc_failures.fetch_add(1, std::memory_order_relaxed); + return nullptr; + } + if (g_fail_curl_calloc) { return nullptr; } @@ -1137,7 +1145,7 @@ TEST_F(BasicCurlHttpTests, AClientWithoutAMultiHandleReachesATerminalOutcome) EXPECT_FALSE(session->IsSessionActive()) << "the session stayed active with nothing running it"; // Cancel before finishing. If either assertion above failed then nothing is going to complete - // this operation, and FinishSession would wait for it for good, so the case would hang rather + // this operation, so the case would hang rather // than fail. Cancelling a session that already answered does nothing. session->CancelSession(); session->FinishSession(); @@ -1573,41 +1581,33 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( nostd::shared_ptr(capture)); - const std::clock_t cpu_before = std::clock(); + int attempts = 0; { // The hooks serve libcurl only, so this fails curl_multi_init on the IO thread without // touching the allocations the rest of the binary makes. + g_curl_calloc_failures.store(0, std::memory_order_relaxed); g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); auto *concrete = static_cast(client.get()); http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); std::this_thread::sleep_for(std::chrono::seconds(1)); + attempts = g_curl_calloc_failures.load(std::memory_order_relaxed); g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); } - const double cpu_seconds = static_cast(std::clock() - cpu_before) / CLOCKS_PER_SEC; - const int log_lines = capture->count_.load(std::memory_order_relaxed); - + const int log_lines = capture->count_.load(std::memory_order_relaxed); opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); - EXPECT_LT(cpu_seconds, 0.5) << "the IO thread spun while it had no multi handle, " << cpu_seconds - << " seconds of CPU over a one second window"; + // Counting what the allocator refused says how often the IO thread tried, which is what a spin + // is, and says it the same way on every platform. The lower bounds matter as much as the upper + // ones: without them an injection that stopped working reads as a pass. + EXPECT_GE(attempts, 1) << "the IO thread never tried to create a handle, so nothing was tested"; + EXPECT_LE(attempts, 64) << "the IO thread tried " << attempts + << " times in a second, which is a spin rather than a wait"; + EXPECT_GE(log_lines, 1) << "the failure was never reported"; EXPECT_LE(log_lines, 8) << "the same failure was reported " << log_lines << " times"; - // And it recovers once allocation works again, so the wait is a wait rather than a stop. - received_requests_.clear(); - auto session = client->CreateSession("http://127.0.0.1:19000"); - auto request = session->CreateRequest(); - request->SetUri("get/"); - auto handler = std::make_shared(); - session->SendRequest(handler); - for (int i = 0; i < 300 && 0 == handler->terminal_.load(std::memory_order_acquire); ++i) - { - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - } - EXPECT_GE(handler->terminal_.load(std::memory_order_acquire), 1) - << "the client did not come back once curl_multi_init could succeed again"; - - session->CancelSession(); - session->FinishSession(); + // Recovery from a handle that could not be created is held by + // AClientWithoutAMultiHandleReachesATerminalOutcome. What this case is for is the state in + // between, which nothing else reaches. client->FinishAllSessions(); } From 3abb874ddc687bdab14e924416bf2052a17a0c3d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:54:34 +0000 Subject: [PATCH 12/38] [BUG] Report the missing handle wait as a wait The loop brackets curl_multi_poll with BeforeWait and AfterWait, and the wait added for a missing multi handle is the same kind of blocking wait, so a runtime with thread instrumentation was being told this thread was running while it was asleep. Compiled with WITH_THREAD_INSTRUMENTATION_PREVIEW=ON rather than only in the default configuration, since the calls are behind that guard and would otherwise never be seen by a compiler. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index b35d557a8d..2db58aff66 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -521,6 +521,13 @@ bool HttpClient::MaybeSpawnBackgroundThread() "curl_multi_init succeeds"); missing_multi_handle_reported = true; } +#ifdef ENABLE_THREAD_INSTRUMENTATION_PREVIEW + if (self->background_thread_instrumentation_ != nullptr) + { + self->background_thread_instrumentation_->BeforeWait(); + } +#endif /* ENABLE_THREAD_INSTRUMENTATION_PREVIEW */ + // In slices, because this thread cannot be woken: wakeupBackgroundThread reaches it // through the multi handle, and there is not one. A whole delay here would be a // whole delay added to destroying the client. @@ -532,6 +539,13 @@ bool HttpClient::MaybeSpawnBackgroundThread() { std::this_thread::sleep_for(kMissingHandleWaitSlice); } + +#ifdef ENABLE_THREAD_INSTRUMENTATION_PREVIEW + if (self->background_thread_instrumentation_ != nullptr) + { + self->background_thread_instrumentation_->AfterWait(); + } +#endif /* ENABLE_THREAD_INSTRUMENTATION_PREVIEW */ } } else if (still_running || need_wait_more) From 6e392a9cc08bda4dd2d72cea9f1876f8030ec90c Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:06:13 +0000 Subject: [PATCH 13/38] [CHORE] Say each invariant once in the comments The comments carried the reasoning that found the bug as well as the rule the code follows. The rule is what a reader needs; the rest belongs in the pull request. Each block now states its constraint and stops, and the two member comments in the installed headers follow the one line trailing form the file already uses next to them. No code changes. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 3 +-- .../ext/http/client/curl/http_operation_curl.h | 6 ++---- ext/src/http/client/curl/http_client_curl.cc | 17 +++++------------ ext/test/http/curl_http_test.cc | 17 ++++++----------- 4 files changed, 14 insertions(+), 29 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index bb73a09245..e4c7a1b301 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -364,8 +364,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doAbortSessions(); bool doRemoveSessions(); bool doRetrySessions(bool report_all); - // Returns whether the client has a multi handle afterwards. The IO loop has nothing to - // run while it does not, and needs to know rather than call into libcurl regardless. + // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); std::mutex multi_handle_m_; 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 b5c905ee6b..6b7c027e6e 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 @@ -325,10 +325,8 @@ class HttpOperation char curl_error_message_[CURL_ERROR_SIZE]{}; HttpCurlEasyResource curl_resource_; - CURLcode last_curl_result_{CURLE_OK}; // Curl result OR HTTP status code if successful - // Set when the constructor could not finish. Send() and SendAsync() refuse on it, so a - // request whose setup never completed is not put on the wire. - CURLcode construction_result_{CURLE_OK}; + CURLcode last_curl_result_{CURLE_OK}; // Curl result OR HTTP status code if successful + CURLcode construction_result_{CURLE_OK}; // Non-OK if setup failed; Send() refuses on it opentelemetry::ext::http::client::EventHandler *event_handle_{nullptr}; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 2db58aff66..1259da967d 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -293,9 +293,7 @@ HttpClient::HttpClient() background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) { - // Members are initialised in declaration order, and multi_handle_ is declared first, so this - // cannot go in the list: curl_global_init has to have run before any other libcurl call, and - // it runs from curl_global_initializer_ above. + // Not in the initialiser list: curl_global_initializer_ is declared later and has to run first. multi_handle_ = initMultiHandle(); } @@ -309,9 +307,7 @@ HttpClient::HttpClient( background_thread_wait_for_{std::chrono::minutes{1}}, curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) { - // Members are initialised in declaration order, and multi_handle_ is declared first, so this - // cannot go in the list: curl_global_init has to have run before any other libcurl call, and - // it runs from curl_global_initializer_ above. + // Not in the initialiser list: curl_global_initializer_ is declared later and has to run first. multi_handle_ = initMultiHandle(); } @@ -612,17 +608,14 @@ bool HttpClient::MaybeSpawnBackgroundThread() } } while (true); - // Abort all pending easy handles. This one calls nothing from the multi interface, so - // it runs whether or not there is a handle and teardown keeps working without one. + // Abort all pending easy handles. Calls no multi function, so it runs without a handle. if (self->doAbortSessions()) { still_running = 1; } - // The three below every call curl_multi_add_handle or curl_multi_remove_handle. Running - // them without a handle would take the pending sessions out of the queue that keeps - // them safe from the next reset, hand them to a multi function that cannot accept - // them, and report the transfer as running. + // The three below each call curl_multi_add_handle or curl_multi_remove_handle. Without + // a handle they would drain the pending queue into a function that cannot accept it. const bool multi_available = (nullptr != self->multi_handle_); // Remove all pending easy handles diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index a465a362a0..3210c12465 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -201,13 +201,10 @@ class RetryEventHandler : public CustomEventHandler } }; -// libcurl routes every internal allocation through the callbacks given to -// curl_global_init_mem, which is the only way to reach the failure returns of -// curl_slist_append and curl_multi_init. They have to be installed before libcurl is -// initialised: called afterwards the function returns CURLE_OK and quietly changes nothing. -// -// The failure switches are thread local, so arming one cannot disturb a client's background -// thread. These callbacks serve the whole binary once installed. +// curl_global_init_mem is the only way to reach the failure returns of curl_slist_append and +// curl_multi_init. It must be called before libcurl is initialised: afterwards it returns +// CURLE_OK and changes nothing. The switches are thread local, so arming one from a test cannot +// disturb a client's background thread. extern "C" { static std::atomic g_curl_hooks_ran{false}; static thread_local bool g_fail_curl_malloc = false; @@ -254,10 +251,8 @@ static char *CurlTestStrdup(const char *str) } // Aimed at curl_multi_init, which allocates with calloc on the libcurl this was measured -// against. Which internal allocation a given libcurl uses is not part of its contract, so -// the cases that rely on this check what actually failed rather than assume. -// Not thread_local, unlike the switches above. The case that keeps a client alive while the -// multi handle cannot be created needs the failure to reach the IO thread. +// against. That is not part of libcurl's contract, so the cases assert on what actually failed. +// Not thread local, unlike the switches above: the failure has to reach the IO thread. static std::atomic g_fail_curl_calloc_everywhere{false}; // Counts what the process wide switch refused, which is one per curl_multi_init the IO thread From b0e33af34fb780620239632208f23a672e31921a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:11:52 +0000 Subject: [PATCH 14/38] [TEST] Hold the phase gate with a request queued during the outage The loop skips the three phases that call curl_multi_add_handle or curl_multi_remove_handle while there is no multi handle. Nothing held that. Ungated, a session queued during the outage leaves the pending queue for a multi function that cannot take it, and the next successful reset cancels it, so the caller is told a request was cancelled that nothing cancelled. The case could not be written before because a process wide calloc failure also fails curl_easy_init, so the request could not be built while the handle was missing. One thread local exemption from that switch fixes it: the IO thread keeps failing, the thread running the case keeps allocating. Measured. With the gate the case passes 3 of 3. With the three calls ungated it fails 3 of 3, on both assertions: cancels is non zero and no response arrives. The whole file stays at 34 passing, 3 of 3. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 81 ++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 3210c12465..47f74af753 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -259,10 +259,14 @@ static std::atomic g_fail_curl_calloc_everywhere{false}; // tried while it had no handle. That is the retry rate, measured the same way everywhere. static std::atomic g_curl_calloc_failures{0}; +// Exempts one thread from the process wide switch. A test that has to build a request while the +// IO thread cannot create a handle needs its own allocations to keep working. +static thread_local bool g_curl_calloc_exempt = false; + static void *CurlTestCalloc(size_t count, size_t size) { g_curl_hooks_ran.store(true, std::memory_order_relaxed); - if (g_fail_curl_calloc_everywhere.load(std::memory_order_relaxed)) + if (g_fail_curl_calloc_everywhere.load(std::memory_order_relaxed) && !g_curl_calloc_exempt) { g_curl_calloc_failures.fetch_add(1, std::memory_order_relaxed); return nullptr; @@ -325,6 +329,7 @@ class MultiHandleOutcomeHandler : public http_client::EventHandler public: void OnResponse(http_client::Response & /* response */) noexcept override { + responses_.fetch_add(1, std::memory_order_release); terminal_.fetch_add(1, std::memory_order_release); } @@ -332,10 +337,13 @@ class MultiHandleOutcomeHandler : public http_client::EventHandler { switch (state) { + case http_client::SessionState::Cancelled: + cancels_.fetch_add(1, std::memory_order_release); + terminal_.fetch_add(1, std::memory_order_release); + break; case http_client::SessionState::CreateFailed: case http_client::SessionState::ConnectFailed: case http_client::SessionState::SendFailed: - case http_client::SessionState::Cancelled: terminal_.fetch_add(1, std::memory_order_release); break; default: @@ -344,6 +352,8 @@ class MultiHandleOutcomeHandler : public http_client::EventHandler } std::atomic terminal_{0}; + std::atomic responses_{0}; + std::atomic cancels_{0}; }; class CapturingLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler @@ -1551,6 +1561,73 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) // A client whose multi handle can never be created has nothing to run. The IO loop reports a run // of failures once and waits between attempts, so a client left alive in that state costs neither // a core nor a log line per pass, and this holds both. +// The phases the loop gates on a multi handle move sessions between queues. Ungated, a session +// queued while the handle is missing leaves the pending queue for a multi function that cannot +// take it, and the next reset cancels it, so the caller is told a request was cancelled that +// nothing cancelled. +TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) +{ + ASSERT_TRUE(g_curl_hooks_installed); + received_requests_.clear(); + + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + + // One completed request, so the IO thread exists and is running the loop under test. + { + auto warm = client->CreateSession("http://127.0.0.1:19000"); + auto warm_request = warm->CreateRequest(); + warm_request->SetUri("get/"); + auto warm_handler = std::make_shared(); + warm->SendRequest(warm_handler); + ASSERT_TRUE(waitForRequests(30, 1)); + warm->FinishSession(); + ASSERT_GE(warm_handler->responses_.load(std::memory_order_acquire), 1); + } + received_requests_.clear(); + + g_curl_calloc_failures.store(0, std::memory_order_relaxed); + g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); + auto *concrete = static_cast(client.get()); + http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); + + // curl_easy_init allocates with calloc too, so without this the request below could not be + // built and the case would test the refusal rather than the queue. + g_curl_calloc_exempt = true; + + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); + session->SendRequest(handler); + + // Wait for the IO thread to go round several times with no handle, so the gated phases have + // had every chance to consume the queued session. + for (int i = 0; i < 200 && g_curl_calloc_failures.load(std::memory_order_relaxed) < 5; ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) + << "the IO thread never ran without a handle, so nothing was tested"; + + g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); + g_curl_calloc_exempt = false; + + for (int i = 0; i < 300 && 0 == handler->terminal_.load(std::memory_order_acquire); ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + + EXPECT_EQ(0, handler->cancels_.load(std::memory_order_acquire)) + << "the queued request was cancelled although nothing cancelled it"; + EXPECT_GE(handler->responses_.load(std::memory_order_acquire), 1) + << "the request queued while the handle was missing never reached the wire"; + + session->CancelSession(); + session->FinishSession(); + client->FinishAllSessions(); +} + TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) { ASSERT_TRUE(g_curl_hooks_installed); From 3e6063b3d3d89e472a633b858c28c8b0d79f3e39 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:16:45 +0000 Subject: [PATCH 15/38] [BUG] Refuse a request whose easy handle was never created curl_easy_init has a failure return like the other two, and the constructor already reports it. It did not record it, so Send() and SendAsync() ran anyway and passed a null handle to libcurl, which answered with a second failure of a different kind for the same request. Measured. With curl_easy_init failing, the handler received a create failure and a connect failure, 3 of 3. Recording the result in construction_result_, which the two send paths already check, leaves the create failure alone, 3 of 3. The whole file goes from 34 passing to 35, 3 of 3. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 4 +- .../http/client/curl/http_operation_curl.cc | 3 ++ ext/test/http/curl_http_test.cc | 41 +++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d2e838fe0..ce2f948ac7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,8 +71,8 @@ Increment the: deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents ([#4349](https://github.com/open-telemetry/opentelemetry-cpp/pull/4349)) -* [BUG] Check what curl_slist_append and curl_multi_init return instead of - treating a failed allocation as success +* [BUG] Check what curl_easy_init, curl_slist_append and curl_multi_init return + instead of treating a failed allocation as success [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) * [BUG] Stop the curl IO thread spinning, flooding the log, and refusing to be destroyed, when the multi handle cannot be created diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index cf6958323f..e9d6e01a8b 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -453,6 +453,9 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, if (!curl_resource_.easy_handle) { last_curl_result_ = CURLE_FAILED_INIT; + // Refuses Send() and SendAsync(), which would otherwise drive a null handle into libcurl and + // add a connect failure to the create failure this already reports. + construction_result_ = CURLE_FAILED_INIT; DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, curl_easy_strerror(last_curl_result_)); return; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 47f74af753..a62234963d 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -342,7 +342,13 @@ class MultiHandleOutcomeHandler : public http_client::EventHandler terminal_.fetch_add(1, std::memory_order_release); break; case http_client::SessionState::CreateFailed: + create_failed_.fetch_add(1, std::memory_order_release); + terminal_.fetch_add(1, std::memory_order_release); + break; case http_client::SessionState::ConnectFailed: + connect_failed_.fetch_add(1, std::memory_order_release); + terminal_.fetch_add(1, std::memory_order_release); + break; case http_client::SessionState::SendFailed: terminal_.fetch_add(1, std::memory_order_release); break; @@ -354,6 +360,8 @@ class MultiHandleOutcomeHandler : public http_client::EventHandler std::atomic terminal_{0}; std::atomic responses_{0}; std::atomic cancels_{0}; + std::atomic create_failed_{0}; + std::atomic connect_failed_{0}; }; class CapturingLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler @@ -1628,6 +1636,39 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) client->FinishAllSessions(); } +// curl_easy_init can fail as well, and the operation then holds no handle. It has to be refused +// like a failed header list, so one request produces one kind of failure rather than a create +// failure from the constructor followed by a connect failure from the null handle. +TEST_F(BasicCurlHttpTests, AFailedEasyHandleIsReportedOnce) +{ + ASSERT_TRUE(g_curl_hooks_installed); + received_requests_.clear(); + + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); + + // Armed here so it reaches the curl_easy_init inside SendRequest and nothing before it. + g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); + session->SendRequest(handler); + g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); + + EXPECT_GE(handler->create_failed_.load(std::memory_order_acquire), 1) + << "the caller was never told the handle could not be created"; + EXPECT_EQ(0, handler->connect_failed_.load(std::memory_order_acquire)) + << "a request with no easy handle still reported a connect failure"; + EXPECT_EQ(0, handler->responses_.load(std::memory_order_acquire)); + EXPECT_FALSE(session->IsSessionActive()) << "the session stayed active with nothing running it"; + EXPECT_EQ(0U, received_requests_.size()) << "a request with no easy handle reached the server"; + + session->FinishSession(); + client->FinishAllSessions(); +} + TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) { ASSERT_TRUE(g_curl_hooks_installed); From 051fe7464c779327fc53cd09757dda86c3734cdf Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 18:03:15 +0000 Subject: [PATCH 16/38] [BUG] Do not retire the IO thread while a queue still holds work The loop skips the remove, add and retry phases while there is no multi handle. Those phases are the only thing that sets still_running, and the idle check reads still_running to decide there is nothing left to do, so the thread could retire with an accepted request still in pending_to_add_session_ids_. Nothing starts it again until another request arrives, so that request waits for good even after allocation recovers. hasPendingWork() answers the question the idle check was really asking. With no handle and a non empty queue the pass is treated as work, which sends the loop back through the bounded missing handle wait rather than out of it. What is measured and what is not. The whole file passes, 35 of 35, twice. The Bazel run of AQueuedRequestSurvivesAMissingMultiHandle timed out with no response, which is what this path does, but running the same pair of cases in one process locally passes 8 of 8 both with and without this change, so that run does not discriminate and I am not claiming it as a reproduction. A case that does discriminate has to shorten the idle window on the client under test so the double check is reached while the handle is still missing. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 4 ++++ ext/src/http/client/curl/http_client_curl.cc | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index e4c7a1b301..9fa9e9f41a 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -367,6 +367,10 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); + // Returns true if any queue still holds work. The IO loop cannot read that from + // still_running, which only the phases that need a multi handle ever set. + bool hasPendingWork(); + std::mutex multi_handle_m_; CURLM *multi_handle_; std::atomic next_session_id_{0}; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 1259da967d..ade9d7d301 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -676,6 +676,14 @@ bool HttpClient::MaybeSpawnBackgroundThread() const bool multi_available_now = (nullptr != self->multi_handle_); + // Without a handle the three phases below are skipped, and they are the only thing + // that sets still_running, so retiring here would strand whatever they did not get + // to. Nothing starts this thread again until another request arrives. + if (!multi_available_now && self->hasPendingWork()) + { + still_running = 1; + } + // Remove all pending easy handles if (multi_available_now && self->doRemoveSessions()) { @@ -799,6 +807,14 @@ void HttpClient::wakeupBackgroundThread() #endif } +bool HttpClient::hasPendingWork() +{ + std::lock_guard session_id_lock_guard{session_ids_m_}; + return !pending_to_add_session_ids_.empty() || !pending_to_abort_sessions_.empty() || + !pending_to_remove_session_handles_.empty() || !pending_to_remove_sessions_.empty() || + !pending_to_retry_sessions_.empty(); +} + bool HttpClient::doAddSessions() { std::unordered_set pending_to_add_session_ids; From ff091365b610c721029d7ef92fbe4af2240ba8d4 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 05:22:56 +0000 Subject: [PATCH 17/38] Revert "[BUG] Do not retire the IO thread while a queue still holds work" This reverts commit 7ac93eda. The change was reasoned from the source and never reproduced: running the same pair of cases in one process passed 8 of 8 with and without it, and the commit said so. Its actual effect on CI was to make things worse. AQueuedRequestSurvivesAMissingMultiHandle already failed under Bazel, which runs every case in one process, and it failed by saying what was wrong: no response arrived. With this change it stopped answering at all and the target timed out at 300 seconds, so no case after it ran either. Bazel went from 6 failing jobs to 21. A hang is a worse failure than an assertion, and the defect this was meant to close is still only argued, not demonstrated. Both go back to where they were: the worker can still retire with queued work, and the case still fails under Bazel, out loud. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 4 ---- ext/src/http/client/curl/http_client_curl.cc | 16 ---------------- 2 files changed, 20 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 9fa9e9f41a..e4c7a1b301 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -367,10 +367,6 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); - // Returns true if any queue still holds work. The IO loop cannot read that from - // still_running, which only the phases that need a multi handle ever set. - bool hasPendingWork(); - std::mutex multi_handle_m_; CURLM *multi_handle_; std::atomic next_session_id_{0}; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index ade9d7d301..1259da967d 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -676,14 +676,6 @@ bool HttpClient::MaybeSpawnBackgroundThread() const bool multi_available_now = (nullptr != self->multi_handle_); - // Without a handle the three phases below are skipped, and they are the only thing - // that sets still_running, so retiring here would strand whatever they did not get - // to. Nothing starts this thread again until another request arrives. - if (!multi_available_now && self->hasPendingWork()) - { - still_running = 1; - } - // Remove all pending easy handles if (multi_available_now && self->doRemoveSessions()) { @@ -807,14 +799,6 @@ void HttpClient::wakeupBackgroundThread() #endif } -bool HttpClient::hasPendingWork() -{ - std::lock_guard session_id_lock_guard{session_ids_m_}; - return !pending_to_add_session_ids_.empty() || !pending_to_abort_sessions_.empty() || - !pending_to_remove_session_handles_.empty() || !pending_to_remove_sessions_.empty() || - !pending_to_retry_sessions_.empty(); -} - bool HttpClient::doAddSessions() { std::unordered_set pending_to_add_session_ids; From 5ca9dab446eed703aa6c390616c5b71c9648017d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 05:31:54 +0000 Subject: [PATCH 18/38] [BUG] Describe a failed easy handle once, and ask the case for once The constructor dispatched CreateFailed and SendRequest dispatched it again when SendAsync refused, so one failed curl_easy_init told the handler twice. A terminal notification is not safe to repeat: a handler that releases its own ownership on the first one is reading freed memory on the second. The header list branch a few lines below already had this right. It records the result and the terminal state and dispatches nothing, leaving the telling to the one caller that knows the request never went out. The easy handle branch does the same now. The case that was supposed to hold this was named ReportedOnce and asked for at least one, so it passed while the code reported twice. It now asks for exactly one create failure and exactly one terminal outcome, and fails without this change. 35 tests pass, 3 of 3. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_operation_curl.cc | 8 ++++---- ext/test/http/curl_http_test.cc | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index e9d6e01a8b..86efac4e27 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -453,11 +453,11 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, if (!curl_resource_.easy_handle) { last_curl_result_ = CURLE_FAILED_INIT; - // Refuses Send() and SendAsync(), which would otherwise drive a null handle into libcurl and - // add a connect failure to the create failure this already reports. + // Refuses Send() and SendAsync(), which would otherwise drive a null handle into libcurl. construction_result_ = CURLE_FAILED_INIT; - DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, - curl_easy_strerror(last_curl_result_)); + // Terminal already, and reported by the caller that sees SendAsync refuse, the same way the + // header list failure below is. Dispatching here as well told the handler twice. + session_state_ = opentelemetry::ext::http::client::SessionState::CreateFailed; return; } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index a62234963d..b03ab49c6e 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1657,8 +1657,10 @@ TEST_F(BasicCurlHttpTests, AFailedEasyHandleIsReportedOnce) session->SendRequest(handler); g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); - EXPECT_GE(handler->create_failed_.load(std::memory_order_acquire), 1) - << "the caller was never told the handle could not be created"; + EXPECT_EQ(1, handler->create_failed_.load(std::memory_order_acquire)) + << "one failed handle was not described exactly once"; + EXPECT_EQ(1, handler->terminal_.load(std::memory_order_acquire)) + << "one request produced more than one terminal outcome"; EXPECT_EQ(0, handler->connect_failed_.load(std::memory_order_acquire)) << "a request with no easy handle still reported a connect failure"; EXPECT_EQ(0, handler->responses_.load(std::memory_order_acquire)); From f7eaebc3a6e179298f8ff69e749f9adba96d9910 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:15:10 +0000 Subject: [PATCH 19/38] [BUG] Keep the IO thread while it still owes somebody an answer Without a multi handle the add, remove and retry phases are skipped, and the retirement check reads that as having nothing to do. So a request the client has already accepted can be sitting in pending_to_add_session_ids_ while the thread that is going to schedule it detaches and leaves. Nothing brings it back: wakeupBackgroundThread goes through the multi handle and there is not one, and only a further request spawns a thread, so FinishSession waits on a promise nobody is left to fulfil. How long that takes depends on the libcurl the job was built against. The idle grace is a minute, but the line that sets it is behind a version check and older libcurl leaves it at zero, where the thread reaches the retirement check on its first idle pass. CMake asks for no minimum libcurl, so both are supported. The thread now stays while it has work of its own left. Shutdown is exempt, and that exemption is the whole difference from the first version of this gate, which took every queue at its size and held the thread against the join in the destructor for entries that could never drain. Actionable is not the same as present: an id whose session has gone is what doAddSessions would drop on its next pass, so hasActionableWork drops it rather than counting it, and does the same for the retry entries doRetrySessions would drop. The wait taken when there is no handle now watches a counter every producer raises, so a queued request is picked up within a slice rather than at the end of the delay, and wakeupBackgroundThread means something on libcurl older than 7.68.0, where it used to compile to nothing at all. The case pins the shorter idle grace rather than inheriting whichever one the job's libcurl allows, and joins the IO thread before taking its multi handle away, which also stops the case destroying a handle another thread is inside. Both together make it deterministic: three runs out of three time out at 240 seconds without the change, printing that the queued request never reached the wire and then hanging in FinishSession, and three out of three pass in 1081 ms with it. All 35 cases in the binary pass, in 24.2 seconds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 9 +++ ext/src/http/client/curl/http_client_curl.cc | 69 +++++++++++++++++-- ext/test/http/curl_http_test.cc | 13 +++- 3 files changed, 85 insertions(+), 6 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index e4c7a1b301..7b38368de9 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -364,6 +364,11 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doAbortSessions(); bool doRemoveSessions(); bool doRetrySessions(bool report_all); + // Returns true if the background thread still owes somebody an answer. Drops what it finds + // that nothing can be owed for, so that a queue which is merely not empty does not read as + // work. Call it on the background thread only: it prunes pending_to_retry_sessions_, which + // has no lock because that thread is the only one that touches it. + bool hasActionableWork(); // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); @@ -388,6 +393,10 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::chrono::milliseconds background_thread_wait_for_; std::atomic is_shutdown_{false}; + // Raised by every producer. curl_multi_wakeup is how the background thread is woken out of + // curl_multi_poll and it needs a multi handle, so the wait taken when there is none watches + // this instead. + std::atomic wakeup_generation_{0}; nostd::shared_ptr curl_global_initializer_; }; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 1259da967d..ae932e680a 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -500,7 +500,8 @@ bool HttpClient::MaybeSpawnBackgroundThread() // curl_multi_perform leaves still_running alone when it rejects the handle, and it // starts at one, so without this the loop keeps reporting work it does not have, // never reaches the shutdown check below, and the thread cannot be joined. - still_running = 0; + still_running = 0; + const uint64_t woken_at = self->wakeup_generation_.load(std::memory_order_acquire); if (self->resetMultiHandle()) { missing_multi_handle_reported = false; @@ -524,13 +525,16 @@ bool HttpClient::MaybeSpawnBackgroundThread() } #endif /* ENABLE_THREAD_INSTRUMENTATION_PREVIEW */ - // In slices, because this thread cannot be woken: wakeupBackgroundThread reaches it - // through the multi handle, and there is not one. A whole delay here would be a - // whole delay added to destroying the client. + // In slices, because curl_multi_wakeup cannot reach this thread: it goes through + // the multi handle, and there is not one. What ends the wait early instead is the + // counter every producer raises, or shutdown. A whole delay spent either way would + // be a whole delay added to answering the next request, and to destroying the + // client. constexpr std::chrono::milliseconds kMissingHandleWaitSlice{16}; for (std::chrono::milliseconds waited = std::chrono::milliseconds::zero(); waited < self->scheduled_delay_milliseconds_ && - !self->is_shutdown_.load(std::memory_order_acquire); + !self->is_shutdown_.load(std::memory_order_acquire) && + woken_at == self->wakeup_generation_.load(std::memory_order_acquire); waited += kMissingHandleWaitSlice) { std::this_thread::sleep_for(kMissingHandleWaitSlice); @@ -694,6 +698,17 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + // Skipping those three reports nothing, which is not the same as having nothing to + // do. A request the client has accepted has to be either handed to libcurl or + // finished, and this thread is the only one that does either, so it stays while it + // owes one. Shutdown is exempt: there the queues that need a handle cannot drain + // without one, and staying for them is staying under the join that is waiting here. + if (!multi_available_now && !self->is_shutdown_.load(std::memory_order_acquire) && + self->hasActionableWork()) + { + still_running = 1; + } + // If there is no pending jobs, we can stop the background thread. if (still_running == 0) { @@ -788,6 +803,10 @@ void HttpClient::WaitBackgroundThreadExit() void HttpClient::wakeupBackgroundThread() { + // First, and whatever libcurl is: the call below needs a multi handle and there is not always + // one, so this is what the background thread watches when it is waiting without a handle. + wakeup_generation_.fetch_add(1, std::memory_order_release); + // Before libcurl 7.68.0, we can only wait for timeout and do the rest jobs // See https://curl.se/libcurl/c/curl_multi_wakeup.html #if LIBCURL_VERSION_NUM >= 0x074400 @@ -961,6 +980,46 @@ bool HttpClient::doRetrySessions(bool /* report_all */) } #endif // ENABLE_OTLP_RETRY_PREVIEW +bool HttpClient::hasActionableWork() +{ + std::lock_guard session_lock_guard{sessions_m_}; + std::lock_guard session_id_lock_guard{session_ids_m_}; + + // An id whose session has gone is what doAddSessions would drop on its next pass, so it is + // dropped here too rather than counted. The difference matters: the first version of this + // check took every queue at its size, and an entry nothing could be done about held the + // thread open against the join in the destructor. + for (auto id = pending_to_add_session_ids_.begin(); id != pending_to_add_session_ids_.end();) + { + const auto session = sessions_.find(*id); + if (session == sessions_.end() || !session->second || !session->second->GetOperation()) + { + id = pending_to_add_session_ids_.erase(id); + } + else + { + ++id; + } + } + + // Same rule doRetrySessions applies to the same container. + for (auto retry = pending_to_retry_sessions_.begin(); retry != pending_to_retry_sessions_.end();) + { + if (!*retry || !(*retry)->GetOperation()) + { + retry = pending_to_retry_sessions_.erase(retry); + } + else + { + ++retry; + } + } + + return !pending_to_add_session_ids_.empty() || !pending_to_abort_sessions_.empty() || + !pending_to_remove_session_handles_.empty() || !pending_to_remove_sessions_.empty() || + !pending_to_retry_sessions_.empty(); +} + bool HttpClient::resetMultiHandle() { std::list> sessions; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index b03ab49c6e..20c8f769a4 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1580,6 +1580,14 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); + auto *concrete = static_cast(client.get()); + + // The idle grace is a minute by default, but only from libcurl 7.68: the assignment that + // gives it that value is behind a version check, and older libcurl leaves it at zero, where + // the IO thread reaches the retirement check on its first idle pass. CMake asks for no + // minimum libcurl, so both are supported and this asks for the shorter one, to run the same + // way everywhere rather than the way whichever libcurl the job has happens to allow. + concrete->SetBackgroundWaitFor(std::chrono::milliseconds::zero()); // One completed request, so the IO thread exists and is running the loop under test. { @@ -1594,9 +1602,12 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) } received_requests_.clear(); + // Join the IO thread before taking its multi handle away. resetMultiHandle destroys the one + // it finds, and a handle another thread is inside is not one to destroy. + concrete->WaitBackgroundThreadExit(); + g_curl_calloc_failures.store(0, std::memory_order_relaxed); g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); - auto *concrete = static_cast(client.get()); http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); // curl_easy_init allocates with calloc too, so without this the request below could not be From e2331e3b759fdc44754b6291762541cd58a1a632 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:20:29 +0000 Subject: [PATCH 20/38] [BUG] Do not call a multi function without a multi handle curl_multi_init says the other multi functions cannot be used once it has returned null. That is the contract the rest of this branch guards, and three calls here were still outside it. curl_multi_cleanup was reached with none twice. The destructor cleans up whatever the constructor got, and the constructor may have got nothing. resetMultiHandle cleans up before building the replacement, and it is the one place that produces the null in the first place, so a second reset arrives with none. Both now go through one function, which answers whether there is a handle to clean up, reports a result that is not CURLM_OK, and leaves none behind either way, so a failed reset cannot be cleaned up twice. curl_multi_remove_handle was the third, through doRemoveSessions, which resetMultiHandle calls whether or not there is a handle. Detaching needs something to detach from, and there is nothing this could name: the handle it would have named was destroyed by curl_multi_cleanup, which detaches what it still holds, and nothing has been attached since. So the easy handle and its header list are released. Measured libcurl answers a null multi handle with CURLM_BAD_HANDLE rather than crashing, on every version this was checked against, so what this changes today is the contract rather than the behaviour. Removing no longer needs a handle, so the loop no longer waits for one before doing it. Waiting held the easy handles and their header lists for the whole outage, which is the wrong way round: an outage is when releasing them matters. Adding and retrying do still need a handle and now say so themselves, before the swap that would drop the ids they took, rather than leaving it to every caller to remember. AHandleQueuedWithoutAMultiHandleIsReleased drives the queue with no handle in place and asks LeakSanitizer whether the release happened. It does not claim to catch the null call, which the versions to hand tolerate. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 3 + ext/src/http/client/curl/http_client_curl.cc | 75 ++++++++++++++----- ext/test/http/curl_http_test.cc | 34 +++++++++ 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 7b38368de9..4ecb308b14 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -369,6 +369,9 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // work. Call it on the background thread only: it prunes pending_to_retry_sessions_, which // has no lock because that thread is the only one that touches it. bool hasActionableWork(); + // Cleans up the multi handle if there is one, and leaves none behind either way. Call it + // holding multi_handle_m_. + void ReleaseMultiHandle(); // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index ae932e680a..4eb373eef5 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -337,7 +337,7 @@ HttpClient::~HttpClient() } { std::lock_guard lock_guard{multi_handle_m_}; - curl_multi_cleanup(multi_handle_); + ReleaseMultiHandle(); } } @@ -618,24 +618,24 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } - // The three below each call curl_multi_add_handle or curl_multi_remove_handle. Without - // a handle they would drain the pending queue into a function that cannot accept it. - const bool multi_available = (nullptr != self->multi_handle_); - - // Remove all pending easy handles - if (multi_available && self->doRemoveSessions()) + // Remove all pending easy handles. Detaching is the only thing here that wants a + // multi handle, and without one there is nothing to detach from, so this releases + // rather than waits: holding the resources back would hold them for the whole + // outage. + if (self->doRemoveSessions()) { still_running = 1; } - // Add all pending easy handles - if (multi_available && self->doAddSessions()) + // Add all pending easy handles. Answers for itself when there is no handle, as does + // the retry below: neither can hand libcurl a transfer without one. + if (self->doAddSessions()) { still_running = 1; } // Check if pending easy handles can be retried - if (multi_available && self->doRetrySessions(false)) + if (self->doRetrySessions(false)) { still_running = 1; } @@ -678,22 +678,20 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } - const bool multi_available_now = (nullptr != self->multi_handle_); - // Remove all pending easy handles - if (multi_available_now && self->doRemoveSessions()) + if (self->doRemoveSessions()) { still_running = 1; } // Add all pending easy handles - if (multi_available_now && self->doAddSessions()) + if (self->doAddSessions()) { still_running = 1; } // Check if pending easy handles can be retried - if (multi_available_now && self->doRetrySessions(true)) + if (self->doRetrySessions(true)) { still_running = 1; } @@ -703,8 +701,8 @@ bool HttpClient::MaybeSpawnBackgroundThread() // finished, and this thread is the only one that does either, so it stays while it // owes one. Shutdown is exempt: there the queues that need a handle cannot drain // without one, and staying for them is staying under the join that is waiting here. - if (!multi_available_now && !self->is_shutdown_.load(std::memory_order_acquire) && - self->hasActionableWork()) + if (nullptr == self->multi_handle_ && + !self->is_shutdown_.load(std::memory_order_acquire) && self->hasActionableWork()) { still_running = 1; } @@ -820,6 +818,12 @@ void HttpClient::wakeupBackgroundThread() bool HttpClient::doAddSessions() { + if (nullptr == multi_handle_) + { + // Before the swap below, which would drop the ids it took. + return false; + } + std::unordered_set pending_to_add_session_ids; { std::lock_guard session_id_lock_guard{session_ids_m_}; @@ -915,7 +919,15 @@ bool HttpClient::doRemoveSessions() curl_slist_free_all(removing_handle.second.headers_chunk); } - curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle); + // Detaching needs something to detach from. Without a multi handle there is nothing + // this could name: the one it would have named was destroyed by curl_multi_cleanup, + // which detaches what it still holds, and nothing has been attached since. So the + // resource is released rather than kept, which is what resetMultiHandle asks for when + // curl_multi_init has just failed on it. + if (nullptr != multi_handle_) + { + curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle); + } curl_easy_cleanup(removing_handle.second.easy_handle); } @@ -939,6 +951,11 @@ bool HttpClient::doRemoveSessions() #ifdef ENABLE_OTLP_RETRY_PREVIEW bool HttpClient::doRetrySessions(bool report_all) { + if (nullptr == multi_handle_) + { + return false; + } + const auto now = std::chrono::system_clock::now(); auto has_data = false; @@ -980,6 +997,26 @@ bool HttpClient::doRetrySessions(bool /* report_all */) } #endif // ENABLE_OTLP_RETRY_PREVIEW +void HttpClient::ReleaseMultiHandle() +{ + if (nullptr == multi_handle_) + { + // curl_multi_init says the other multi functions cannot be used once it has returned null, + // and curl_multi_cleanup is one of them. Reaching here with none is ordinary: the + // constructor may have started without one, and a reset that could not build a replacement + // leaves none behind. + return; + } + + const CURLMcode cleanup_result = curl_multi_cleanup(multi_handle_); + multi_handle_ = nullptr; + if (CURLM_OK != cleanup_result) + { + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_cleanup failed with message: " + << curl_multi_strerror(cleanup_result)); + } +} + bool HttpClient::hasActionableWork() { std::lock_guard session_lock_guard{sessions_m_}; @@ -1049,7 +1086,7 @@ bool HttpClient::resetMultiHandle() // We will modify the multi_handle_, so we need to lock it std::lock_guard lock_guard{multi_handle_m_}; - curl_multi_cleanup(multi_handle_); + ReleaseMultiHandle(); // Create a another multi handle to continue pending sessions. Silent on failure: the caller // decides how often a run of failures is worth reporting. diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 20c8f769a4..d122980211 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -59,6 +59,15 @@ class HttpClientTestPeer { public: static void ResetMultiHandle(HttpClient &client) { client.resetMultiHandle(); } + + static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + + static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) + { + CURLM *previous = client.multi_handle_; + client.multi_handle_ = replacement; + return previous; + } }; } // namespace curl } // namespace client @@ -1573,6 +1582,31 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) // queued while the handle is missing leaves the pending queue for a multi function that cannot // take it, and the next reset cancels it, so the caller is told a request was cancelled that // nothing cancelled. +TEST_F(BasicCurlHttpTests, AHandleQueuedWithoutAMultiHandleIsReleased) +{ + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + auto *concrete = static_cast(client.get()); + + // Nothing has been sent, so this is the only thread here and the queue is the client's own. + http_client::curl::HttpCurlEasyResource resource; + resource.easy_handle = curl_easy_init(); + ASSERT_TRUE(resource.easy_handle != nullptr); + resource.headers_chunk = curl_slist_append(nullptr, "X-Test: 1"); + ASSERT_TRUE(resource.headers_chunk != nullptr); + + CURLM *previous = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(*concrete, nullptr); + concrete->ScheduleRemoveSession(4404, std::move(resource)); + + // What resetMultiHandle does when curl_multi_init has just failed on it. The easy handle and + // its header list are released rather than held until a multi handle comes back, and neither + // is handed to a multi function that has none to work with. LeakSanitizer is what says the + // release happened. + EXPECT_TRUE(http_client::curl::HttpClientTestPeer::RemoveSessions(*concrete)); + + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(*concrete, previous); +} + TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) { ASSERT_TRUE(g_curl_hooks_installed); From 7811a6f8d6fad3707c0a0e9bc640faa12cb63aad Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:57:34 +0000 Subject: [PATCH 21/38] [TEST] Build the client without a multi handle rather than taking one away Both recovery cases used to send a request first, so the IO thread existed, and then call resetMultiHandle from the test thread. That thread destroys the multi handle and builds another, while the IO thread may be inside curl_multi_perform, curl_multi_poll or curl_multi_info_read on the same one. libcurl leaves handles unsynchronized and says one handle is not to be used from two threads at once, and the IO thread does not hold multi_handle_m_ around those calls, so the lock the reset takes does not stand for it. That makes the cases unsound whatever they report. The unexpected cancel one of them saw in CI is real, but a case that breaks handle ownership cannot say whether what it saw came from the client or from itself. Neither needs to take a handle away. curl_multi_init is what fails in #4404, and it is called first by the constructor, so a client built while the allocator is refusing has no multi handle from the start and every attempt after that is the IO thread's own. The test thread exempts itself once the client exists, since curl_easy_init allocates the same way and the request still has to be built. What each case asks is unchanged, and the queued one now pins the shorter idle grace as well, so it does not inherit whichever one the job's libcurl allows. The spin case keeps a request in flight because that is what keeps the IO thread there now, and installs its log capture after the constructor so the report the constructor makes is not counted as one of the loop's. Three runs out of three for the seven cases that touch a missing handle, and 36 out of 36 for the binary, twice over. The queued case still hangs without the retirement gate, three runs out of three, printing that the request never reached the wire. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 92 +++++++++++++++------------------ 1 file changed, 42 insertions(+), 50 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index d122980211..bbeb3babdc 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1612,50 +1612,36 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) ASSERT_TRUE(g_curl_hooks_installed); received_requests_.clear(); + // Built without a handle, for the reason given above: a case that took one away would be + // using it from two threads at once, and could not then say whether what it saw was the + // client's behaviour or its own. + g_curl_calloc_failures.store(0, std::memory_order_relaxed); + g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); auto *concrete = static_cast(client.get()); + ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) + << "the client was built with a multi handle, so nothing was tested"; + + g_curl_calloc_exempt = true; - // The idle grace is a minute by default, but only from libcurl 7.68: the assignment that - // gives it that value is behind a version check, and older libcurl leaves it at zero, where - // the IO thread reaches the retirement check on its first idle pass. CMake asks for no - // minimum libcurl, so both are supported and this asks for the shorter one, to run the same + // The idle grace is a minute by default, but only from libcurl 7.68: the line that gives it + // that value is behind a version check, and older libcurl leaves it at zero, where the IO + // thread reaches the retirement check on its first idle pass. CMake asks for no minimum + // libcurl, so both are supported, and this asks for the shorter one so the case runs the same // way everywhere rather than the way whichever libcurl the job has happens to allow. concrete->SetBackgroundWaitFor(std::chrono::milliseconds::zero()); - // One completed request, so the IO thread exists and is running the loop under test. - { - auto warm = client->CreateSession("http://127.0.0.1:19000"); - auto warm_request = warm->CreateRequest(); - warm_request->SetUri("get/"); - auto warm_handler = std::make_shared(); - warm->SendRequest(warm_handler); - ASSERT_TRUE(waitForRequests(30, 1)); - warm->FinishSession(); - ASSERT_GE(warm_handler->responses_.load(std::memory_order_acquire), 1); - } - received_requests_.clear(); - - // Join the IO thread before taking its multi handle away. resetMultiHandle destroys the one - // it finds, and a handle another thread is inside is not one to destroy. - concrete->WaitBackgroundThreadExit(); - - g_curl_calloc_failures.store(0, std::memory_order_relaxed); - g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); - http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); - - // curl_easy_init allocates with calloc too, so without this the request below could not be - // built and the case would test the refusal rather than the queue. - g_curl_calloc_exempt = true; - auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("get/"); auto handler = std::make_shared(); + g_curl_calloc_failures.store(0, std::memory_order_relaxed); session->SendRequest(handler); - // Wait for the IO thread to go round several times with no handle, so the gated phases have - // had every chance to consume the queued session. + // Wait for the IO thread to go round several times with no handle, so the phases that need + // one have had every chance to consume the queued session and the retirement check has been + // reached more than once. for (int i = 0; i < 200 && g_curl_calloc_failures.load(std::memory_order_relaxed) < 5; ++i) { std::this_thread::sleep_for(std::chrono::milliseconds(10)); @@ -1721,21 +1707,23 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) ASSERT_TRUE(g_curl_hooks_installed); received_requests_.clear(); + // The client is built without a multi handle rather than having one taken away. Nothing here + // touches a handle another thread is using, which libcurl does not allow and which would + // leave any result this case reported open to being an artefact of the injection. What fails + // is curl_multi_init, on whichever thread calls it, and after the constructor that is the IO + // thread every time. + g_curl_calloc_failures.store(0, std::memory_order_relaxed); + g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); + ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) + << "the client was built with a multi handle, so nothing was tested"; - // One completed request, so the IO thread exists and the loop below is the one under test. - { - auto warm = client->CreateSession("http://127.0.0.1:19000"); - auto warm_request = warm->CreateRequest(); - warm_request->SetUri("get/"); - auto warm_handler = std::make_shared(); - warm->SendRequest(warm_handler); - ASSERT_TRUE(waitForRequests(30, 1)); - warm->FinishSession(); - ASSERT_GE(warm_handler->terminal_.load(std::memory_order_acquire), 1); - } + // curl_easy_init allocates with calloc too, and the request below needs one. The IO thread is + // not exempt, so its curl_multi_init goes on failing. + g_curl_calloc_exempt = true; + // After the constructor, so the report it makes is not counted as one of the loop's. auto *capture = new CountingLogHandler(); auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( @@ -1743,15 +1731,22 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) int attempts = 0; { - // The hooks serve libcurl only, so this fails curl_multi_init on the IO thread without - // touching the allocations the rest of the binary makes. + // A request is what keeps the IO thread there. With nothing owed to anybody it retires, + // which is the other half of this and is held by the case below. + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); g_curl_calloc_failures.store(0, std::memory_order_relaxed); - g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); - auto *concrete = static_cast(client.get()); - http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); + session->SendRequest(handler); + std::this_thread::sleep_for(std::chrono::seconds(1)); attempts = g_curl_calloc_failures.load(std::memory_order_relaxed); + g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); + g_curl_calloc_exempt = false; + session->CancelSession(); + session->FinishSession(); } const int log_lines = capture->count_.load(std::memory_order_relaxed); opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); @@ -1765,9 +1760,6 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) EXPECT_GE(log_lines, 1) << "the failure was never reported"; EXPECT_LE(log_lines, 8) << "the same failure was reported " << log_lines << " times"; - // Recovery from a handle that could not be created is held by - // AClientWithoutAMultiHandleReachesATerminalOutcome. What this case is for is the state in - // between, which nothing else reaches. client->FinishAllSessions(); } From 14cd974733c1b7ffa21b790dc0d25f7521bcdf23 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:06:54 +0000 Subject: [PATCH 22/38] [BUG] Release what is still queued when the client goes An operation hands its easy handle and its header list to pending_to_remove_session_handles_ on its way out, and that record is two raw pointers whose container frees neither. Everything that drains it runs on the background thread. When the client is destroyed the thread is joined, and anything queued after it retired, or queued by the cancel the destructor itself does, has nobody left to release it. This is not the same as a request going unanswered. The caller can have had its terminal outcome already: what is left over is the libcurl resources behind it, with no owner. So the destructor does the last pass itself, once the thread has gone and no more sessions can be made. Aborting first, since finishing an operation is what queues its resources, and both before the multi handle is released so a handle still attached can be given back rather than freed underneath it. In the ordinary case both queues are already empty and neither call does anything. AQueuedHandleIsReleasedWithTheClientThatQueuedIt queues one and destroys the client without draining it. Nothing is sent, so there is no background thread and nothing else is coming for it, and LeakSanitizer is the assertion: without this it reports 5582 bytes in 7 allocations, and with it the whole binary is clean, 37 cases out of 37. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 10 ++++++++++ ext/test/http/curl_http_test.cc | 17 +++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 4eb373eef5..68869ccb6e 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -335,6 +335,16 @@ HttpClient::~HttpClient() background_thread->join(); } } + + // The background thread has gone and no more sessions are made here, so nothing else is + // coming back for what it left behind. Aborting first, because finishing an operation hands + // its easy handle and header list to the removal queue, and that queue holds two raw + // pointers whose container frees neither. Ordinarily both are already empty: this is for the + // case where the thread had retired before the sessions were cancelled, and it runs before + // the multi handle goes so a handle that is still attached can be given back. + doAbortSessions(); + doRemoveSessions(); + { std::lock_guard lock_guard{multi_handle_m_}; ReleaseMultiHandle(); diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index bbeb3babdc..7ec7f79eb5 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1582,6 +1582,23 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) // queued while the handle is missing leaves the pending queue for a multi function that cannot // take it, and the next reset cancels it, so the caller is told a request was cancelled that // nothing cancelled. +TEST_F(BasicCurlHttpTests, AQueuedHandleIsReleasedWithTheClientThatQueuedIt) +{ + auto client = std::make_shared(); + + http_client::curl::HttpCurlEasyResource resource; + resource.easy_handle = curl_easy_init(); + ASSERT_TRUE(resource.easy_handle != nullptr); + resource.headers_chunk = curl_slist_append(nullptr, "X-Test: 1"); + ASSERT_TRUE(resource.headers_chunk != nullptr); + client->ScheduleRemoveSession(4404, std::move(resource)); + + // Nothing was sent, so there is no IO thread and nobody else is coming for that queue. The + // record is two raw pointers and the container that holds it frees neither, so what is still + // queued when the client goes is the client's to release. LeakSanitizer is the assertion. + client.reset(); +} + TEST_F(BasicCurlHttpTests, AHandleQueuedWithoutAMultiHandleIsReleased) { auto client = std::make_shared()->Create(); From f09ff476794e9cd0635ec118f0eec0f0b65ce7a2 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:14:10 +0000 Subject: [PATCH 23/38] [BUG] Take out of the retry queue what is not going to be retried A retryable response puts the session in the retry queue with a time on it. Anything that takes the operation apart before that time comes, a cancel or a reset, leaves the entry there: the session is still a session and it still has an operation, which is all the queue looked at. Two things follow, and both are worse than a wasted entry. When the time comes, the easy handle it wants has gone back to the client already, so what libcurl is handed is a null handle, and what comes back was never read: the entry is erased and reported as a retry that was arranged. And until the time comes, the idle check reports the queue as work, so the background thread stays. At shutdown that is the join waiting, for an operation that finished long before. So an entry is dropped when the operation is gone, was cancelled, or no longer holds an easy handle, which is what being torn down looks like from here since Cleanup hands the resource back and leaves none. The queue is in order, so the first entry whose time has not come still ends the pass. The remove and the add are read now, and the add only happens if the remove worked. If the handle cannot be put back to run, the operation is finished rather than erased and forgotten, which is the same rule the rest of this file follows: nothing is left holding a promise that nobody is going to fulfil. ACancelledRetryDoesNotHoldTheClientOpen asks the server for a retryable answer with an eight second backoff, cancels, and times the destructor. Without this it takes 6.4 seconds, three runs out of three, and with it 11 to 512 ms. All 38 cases in the binary pass, in 25.5 seconds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 40 ++++++++++---- ext/test/http/curl_http_test.cc | 55 ++++++++++++++++++++ 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 68869ccb6e..05e804cf74 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -979,22 +979,44 @@ bool HttpClient::doRetrySessions(bool report_all) const auto session = *retry_it; const auto operation = session ? session->GetOperation().get() : nullptr; - if (!operation) + // An operation that was cancelled, or torn down, is not going to be retried. Its easy + // handle has gone back to the client already, so what waiting for its turn would buy is a + // null handle offered to libcurl and an entry that keeps the background thread alive until + // a time that means nothing. At shutdown that time is time the join spends waiting. + if (nullptr == operation || operation->WasAborted() || + nullptr == operation->GetCurlEasyHandle()) { retry_it = pending_to_retry_sessions_.erase(retry_it); + continue; } - else if (operation->NextRetryTime() < now) + + if (operation->NextRetryTime() >= now) { - auto easy_handle = operation->GetCurlEasyHandle(); - curl_multi_remove_handle(multi_handle_, easy_handle); - curl_multi_add_handle(multi_handle_, easy_handle); - retry_it = pending_to_retry_sessions_.erase(retry_it); - has_data = true; + // Pushed at the back, so nothing behind this one is due either. + break; } - else + + CURL *const easy_handle = operation->GetCurlEasyHandle(); + + // The handle is still with the multi handle from the attempt that just failed, so it has + // to come back before it can go again, and it only goes again if it came back. + const CURLMcode detached = curl_multi_remove_handle(multi_handle_, easy_handle); + const CURLMcode attached = + (CURLM_OK == detached) ? curl_multi_add_handle(multi_handle_, easy_handle) : detached; + + retry_it = pending_to_retry_sessions_.erase(retry_it); + + if (CURLM_OK != attached) { - break; + // Nobody is going to run this transfer, so it is finished here rather than left with a + // promise nothing will fulfil, and it is not reported as work that was arranged. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] a retry could not be scheduled: " << curl_multi_strerror(attached)); + session->FinishOperation(); + continue; } + + has_data = true; } report_all = report_all && !pending_to_retry_sessions_.empty(); diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 7ec7f79eb5..a5c73252f0 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -62,6 +62,13 @@ class HttpClientTestPeer static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } +#ifdef ENABLE_OTLP_RETRY_PREVIEW + static std::size_t RetryQueueSize(const HttpClient &client) + { + return client.pending_to_retry_sessions_.size(); + } +#endif // ENABLE_OTLP_RETRY_PREVIEW + static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) { CURLM *previous = client.multi_handle_; @@ -1582,6 +1589,54 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) // queued while the handle is missing leaves the pending queue for a multi function that cannot // take it, and the next reset cancels it, so the caller is told a request was cancelled that // nothing cancelled. +#ifdef ENABLE_OTLP_RETRY_PREVIEW +TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen) +{ + received_requests_.clear(); + + const auto started = std::chrono::steady_clock::now(); + { + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + auto *concrete = static_cast(client.get()); + + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("retry/"); + request->SetMethod(http_client::Method::Post); + + // Long enough that waiting for it would be unmistakable, and long enough that it cannot + // come round on its own inside the bound below. + request->SetRetryPolicy( + {4, std::chrono::duration{8.0f}, std::chrono::duration{16.0f}, 2.0f}); + + auto handler = std::make_shared(); + session->SendRequest(handler); + + // The server answers this route with something retryable, so the session goes into the + // retry queue with a time on it that has not come. + for (int i = 0; + i < 300 && 0 == http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete); ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + ASSERT_GE(http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete), 1u) + << "the request was never queued for retry, so nothing was tested"; + + // Cancelling takes the operation apart and hands its easy handle back, and the entry left + // behind names an operation that is not going to be retried by anybody. + session->CancelSession(); + session->FinishSession(); + } + const auto elapsed = std::chrono::duration_cast( + std::chrono::steady_clock::now() - started); + + EXPECT_LT(elapsed.count(), 4000) + << "destroying the client took " << elapsed.count() + << " ms, which is it waiting out a retry for an operation that had already finished"; +} +#endif // ENABLE_OTLP_RETRY_PREVIEW + TEST_F(BasicCurlHttpTests, AQueuedHandleIsReleasedWithTheClientThatQueuedIt) { auto client = std::make_shared(); From b98ef2935bad4ebf63c6c975dc52f2b65ec04e61 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:24:56 +0000 Subject: [PATCH 24/38] [TEST] Do not let a case leave global state behind it Four things, all the same shape: state that belongs to the process was set by a case and put back by the same case writing it out again, which only happens if the case gets that far and only means anything while nothing else is looking. The allocator switches were armed with three fatal assertions between the arming and the disarming. One of those returning would have left every later case in the binary allocating through a calloc that refuses, on every thread, which turns one mismatched injection into a matrix of timeouts rather than one red case. They are a guard now, next to the two the file already had, and the cases that disarm on their way through still do. The internal log handler is a process global that GlobalLogHandler reads and writes through a plain shared pointer with nothing synchronizing it, and the documentation asks for it to be set once at startup for that reason. One case put it back while the client, and the thread writing to it, were still there. It is a guard too, declared before the client so it is destroyed after it, and the count is read once the client has gone. That handler counted every line rather than the one the case is about, so anything else the binary wrote while it was installed moved a bound that is meant to say how often one failure was reported. It takes the text it wants now, which also leaves out the line the constructor writes about the same failure, so the case no longer needs to be installed late to avoid it. And curl_global_init_mem in SetUpTestSuite had no matching cleanup. libcurl counts initializations and asks for one cleanup for each, and every client takes one of its own through HttpCurlGlobalInitializer, so the count never reached zero: the allocator callbacks stayed installed into static teardown and what libcurl still held was reported as leaked. One more thing worth saying about a bound rather than a guard. Both recovery cases put the failure count back to zero after the constructor's own attempt and before there is an IO thread to make one, and the thread that armed it is exempt from that point, so what they count afterwards was refused to the IO thread and to nothing else. Without that the assertion saying the IO thread tried would have been satisfied by the constructor. All 38 cases pass, in 25.5 seconds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 180 +++++++++++++++++++++++--------- 1 file changed, 133 insertions(+), 47 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index a5c73252f0..199ed959be 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -320,24 +320,96 @@ struct FailCurlCalloc FailCurlCalloc &operator=(FailCurlCalloc &&) = delete; }; +// The failure that reaches every thread, and the exemption for the one that armed it, since +// they are armed together and have to be put away together. A guard because these two are +// process wide and there are fatal assertions between arming and disarming: one of those +// returning early would otherwise leave every later case in the binary allocating through a +// calloc that refuses, on every thread, which turns one mismatched injection into a matrix of +// timeouts rather than one red case. The cases below disarm on their way through, and this is +// what happens when they do not get that far. +struct FailCurlCallocEverywhere +{ + FailCurlCallocEverywhere() + { + g_curl_calloc_failures.store(0, std::memory_order_relaxed); + g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); + } + + ~FailCurlCallocEverywhere() { Disarm(); } + + // curl_easy_init allocates the same way, so a case that has to build a request exempts the + // thread it builds it on. The IO thread is not exempt, which is the point. + void ExemptThisThread() { g_curl_calloc_exempt = true; } + + void Disarm() + { + g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); + g_curl_calloc_exempt = false; + } + + FailCurlCallocEverywhere(const FailCurlCallocEverywhere &) = delete; + FailCurlCallocEverywhere(FailCurlCallocEverywhere &&) = delete; + FailCurlCallocEverywhere &operator=(const FailCurlCallocEverywhere &) = delete; + FailCurlCallocEverywhere &operator=(FailCurlCallocEverywhere &&) = delete; +}; + // Counts terminal outcomes without caring which one, since a client whose multi handle could // not be created may still recover and answer, and the case below is about the caller being // told either way rather than about which answer it gets. -// Counts internal log lines, so a case can hold that a run of failures is reported a bounded -// number of times rather than once per pass of the IO loop. +// Counts the internal log lines that say a particular thing, so a case can hold that a run of +// failures is reported a bounded number of times rather than once per pass of the IO loop. Only +// the ones it asked for: anything else the binary writes while this is installed would move the +// bound without meaning anything. class CountingLogHandler : public opentelemetry::sdk::common::internal_log::LogHandler { public: + explicit CountingLogHandler(std::string wanted) : wanted_{std::move(wanted)} {} + void Handle(opentelemetry::sdk::common::internal_log::LogLevel /* level */, const char * /* file */, int /* line */, - const char * /* msg */, + const char *msg, const opentelemetry::sdk::common::AttributeMap & /* attributes */) noexcept override { - count_.fetch_add(1, std::memory_order_relaxed); + if (nullptr != msg && std::string::npos != std::string{msg}.find(wanted_)) + { + count_.fetch_add(1, std::memory_order_relaxed); + } } std::atomic count_{0}; + +private: + const std::string wanted_; +}; + +// GlobalLogHandler reads and writes the handler through a plain shared pointer with nothing +// synchronizing it, and the documentation asks for it to be set once at startup for that +// reason. So a case that captures installs it before anything that logs exists and puts it back +// after all of it has gone. A guard rather than two calls: declared before the client, it is +// destroyed after it, and an assertion that returns early still puts it back. +class ScopedLogHandler +{ +public: + explicit ScopedLogHandler( + const nostd::shared_ptr &handler) + : previous_{opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler()} + { + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(handler); + } + + ~ScopedLogHandler() + { + opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous_); + } + + ScopedLogHandler(const ScopedLogHandler &) = delete; + ScopedLogHandler(ScopedLogHandler &&) = delete; + ScopedLogHandler &operator=(const ScopedLogHandler &) = delete; + ScopedLogHandler &operator=(ScopedLogHandler &&) = delete; + +private: + const nostd::shared_ptr previous_; }; class MultiHandleOutcomeHandler : public http_client::EventHandler @@ -485,6 +557,18 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe CurlTestRealloc, CurlTestStrdup, CurlTestCalloc)); } + // libcurl counts initializations and asks for a cleanup for each one. The line above is this + // suite's, and every client takes one of its own through HttpCurlGlobalInitializer, so + // without this the count never reaches zero: the allocator callbacks stay installed into + // static teardown, and what libcurl still holds is reported as leaked. + static void TearDownTestSuite() + { + if (g_curl_hooks_installed) + { + curl_global_cleanup(); + } + } + protected: void SetUp() override { @@ -1042,9 +1126,8 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleAllocationIsReported) } auto *capture = new CapturingLogHandler(); - auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); - opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( - nostd::shared_ptr(capture)); + ScopedLogHandler installed{ + nostd::shared_ptr(capture)}; { FailCurlCalloc fail; @@ -1054,7 +1137,6 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleAllocationIsReported) } const std::string text = capture->Text(); - opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); EXPECT_NE(std::string::npos, text.find("curl_multi_init failed")) << "a multi handle that could not be created was not reported, captured: " << text; @@ -1114,9 +1196,8 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleIsReportedForAnInstrumentedClient) } auto *capture = new CapturingLogHandler(); - auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); - opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( - nostd::shared_ptr(capture)); + ScopedLogHandler installed{ + nostd::shared_ptr(capture)}; { FailCurlCalloc fail; @@ -1127,7 +1208,6 @@ TEST_F(BasicCurlHttpTests, AFailedMultiHandleIsReportedForAnInstrumentedClient) } const std::string text = capture->Text(); - opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); EXPECT_NE(std::string::npos, text.find("curl_multi_init failed")) << "the instrumented constructor did not report a multi handle it could not create, " @@ -1687,15 +1767,14 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) // Built without a handle, for the reason given above: a case that took one away would be // using it from two threads at once, and could not then say whether what it saw was the // client's behaviour or its own. - g_curl_calloc_failures.store(0, std::memory_order_relaxed); - g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); + FailCurlCallocEverywhere failing; auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); auto *concrete = static_cast(client.get()); ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) << "the client was built with a multi handle, so nothing was tested"; - g_curl_calloc_exempt = true; + failing.ExemptThisThread(); // The idle grace is a minute by default, but only from libcurl 7.68: the line that gives it // that value is behind a version check, and older libcurl leaves it at zero, where the IO @@ -1708,6 +1787,10 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) auto request = session->CreateRequest(); request->SetUri("get/"); auto handler = std::make_shared(); + + // Back to zero after the constructor's own attempt and before there is an IO thread to make + // one, and this thread is exempt from here on, so what is counted below was refused to that + // thread and to nothing else. Without this the wait would be satisfied by the constructor. g_curl_calloc_failures.store(0, std::memory_order_relaxed); session->SendRequest(handler); @@ -1721,8 +1804,7 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) << "the IO thread never ran without a handle, so nothing was tested"; - g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); - g_curl_calloc_exempt = false; + failing.Disarm(); for (int i = 0; i < 300 && 0 == handler->terminal_.load(std::memory_order_acquire); ++i) { @@ -1755,10 +1837,11 @@ TEST_F(BasicCurlHttpTests, AFailedEasyHandleIsReportedOnce) request->SetUri("get/"); auto handler = std::make_shared(); - // Armed here so it reaches the curl_easy_init inside SendRequest and nothing before it. - g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); - session->SendRequest(handler); - g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); + { + // Armed here so it reaches the curl_easy_init inside SendRequest and nothing before it. + FailCurlCallocEverywhere failing; + session->SendRequest(handler); + } EXPECT_EQ(1, handler->create_failed_.load(std::memory_order_acquire)) << "one failed handle was not described exactly once"; @@ -1779,49 +1862,54 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) ASSERT_TRUE(g_curl_hooks_installed); received_requests_.clear(); - // The client is built without a multi handle rather than having one taken away. Nothing here - // touches a handle another thread is using, which libcurl does not allow and which would - // leave any result this case reported open to being an artefact of the injection. What fails - // is curl_multi_init, on whichever thread calls it, and after the constructor that is the IO - // thread every time. - g_curl_calloc_failures.store(0, std::memory_order_relaxed); - g_fail_curl_calloc_everywhere.store(true, std::memory_order_relaxed); - auto client = std::make_shared()->Create(); - ASSERT_TRUE(client != nullptr); - ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) - << "the client was built with a multi handle, so nothing was tested"; - - // curl_easy_init allocates with calloc too, and the request below needs one. The IO thread is - // not exempt, so its curl_multi_init goes on failing. - g_curl_calloc_exempt = true; - - // After the constructor, so the report it makes is not counted as one of the loop's. - auto *capture = new CountingLogHandler(); - auto previous = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogHandler(); - opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler( - nostd::shared_ptr(capture)); + // First, and outside everything below, so that it is in place before anything that logs + // exists and goes back only once all of it has been destroyed. Counting the line the loop + // writes rather than every line, which also leaves out the one the constructor writes about + // the same failure. + auto *capture = new CountingLogHandler("no multi handle"); + ScopedLogHandler installed{ + nostd::shared_ptr(capture)}; int attempts = 0; { + // The client is built without a multi handle rather than having one taken away. Nothing + // here touches a handle another thread is using, which libcurl does not allow and which + // would leave any result this case reported open to being an artefact of the injection. + // What fails is curl_multi_init, on whichever thread calls it, and after the constructor + // that is the IO thread every time. + FailCurlCallocEverywhere failing; + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + ASSERT_GE(g_curl_calloc_failures.load(std::memory_order_relaxed), 1) + << "the client was built with a multi handle, so nothing was tested"; + + failing.ExemptThisThread(); + // A request is what keeps the IO thread there. With nothing owed to anybody it retires, // which is the other half of this and is held by the case below. auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("get/"); auto handler = std::make_shared(); + + // Back to zero after the constructor's own attempt and before there is an IO thread to + // make one, and this thread is exempt from here on, so what is counted below was refused + // to that thread and to nothing else. Without this the count would be the constructor's + // and would say nothing about whether the loop ever tried. g_curl_calloc_failures.store(0, std::memory_order_relaxed); session->SendRequest(handler); std::this_thread::sleep_for(std::chrono::seconds(1)); attempts = g_curl_calloc_failures.load(std::memory_order_relaxed); - g_fail_curl_calloc_everywhere.store(false, std::memory_order_relaxed); - g_curl_calloc_exempt = false; + failing.Disarm(); session->CancelSession(); session->FinishSession(); + client->FinishAllSessions(); } + + // Read once the client, and the thread that was writing those lines, have gone. const int log_lines = capture->count_.load(std::memory_order_relaxed); - opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous); // Counting what the allocator refused says how often the IO thread tried, which is what a spin // is, and says it the same way on every platform. The lower bounds matter as much as the upper @@ -1831,8 +1919,6 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) << " times in a second, which is a spin rather than a wait"; EXPECT_GE(log_lines, 1) << "the failure was never reported"; EXPECT_LE(log_lines, 8) << "the same failure was reported " << log_lines << " times"; - - client->FinishAllSessions(); } } // namespace From fe216503e44ad738a5875ce2ebcac3171102071e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:29:18 +0000 Subject: [PATCH 25/38] [TEST] Fail the second list node, not just the first The header cases sent one header, so the append that failed was the first one and the list it was given was empty. That is the one shape the temporary pointer cannot be told apart from assigning the result straight back: with nothing appended yet there is nothing to lose. So the fix had no case that could fail without it. Aiming at the second append needs the failure to land on a particular allocation, and until now the injection asked for any block of 64 bytes or less, which is a guess about a libcurl that does not promise which allocator a list append uses, how large a node is, or what else asks for a small block first. So it is measured instead: the file watches one append happen and takes the size of the block it asked for, then refuses blocks of exactly that size, after letting a stated number through. The string is copied through the strdup callback, which is deliberately not the one being watched, so the only block a measured append shows is the node. That also narrows what has to be skipped. The existing cases skip when something else consumed the failure, which is a property of the libcurl in use. This one skips only if an append does not reach the malloc callback at all, which is a much smaller claim, and it says so in those words. AHeaderListThatFailsPartWayThroughIsNotLost sends two headers and refuses the second node. Against the assignment main has, LeakSanitizer reports 27 bytes in 2 allocations, three runs out of three, and the whole binary is clean with the temporary, 39 cases out of 39 under AddressSanitizer with detect_leaks=1. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 116 ++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 199ed959be..8551b2c7c9 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -233,9 +233,37 @@ static thread_local bool g_fail_curl_calloc = false; // failing allocation is otherwise a property of the libcurl in use rather than of this test. static const size_t kCurlSmallAllocation = 64; +// Measured rather than assumed. libcurl does not say which allocator a list append uses, how +// large a node is, or how many allocations anything else makes first, so a case that wants the +// failure to land on a particular append asks this file to watch one happen and then aims at +// blocks of exactly that size. Which append is the one that fails is a count, so a case can +// have the first go through and the second not. +static thread_local size_t g_watch_malloc_size = 0; +static thread_local size_t g_watched_malloc_size = 0; +static thread_local size_t g_fail_malloc_of_size = 0; +static thread_local int g_let_through_before_fail = 0; + static void *CurlTestMalloc(size_t size) { g_curl_hooks_ran.store(true, std::memory_order_relaxed); + + if (0 != g_watch_malloc_size && 0 == g_watched_malloc_size) + { + g_watched_malloc_size = size; + } + + if (0 != g_fail_malloc_of_size && size == g_fail_malloc_of_size) + { + if (g_let_through_before_fail > 0) + { + --g_let_through_before_fail; + } + else + { + return nullptr; + } + } + if (g_fail_curl_malloc && size <= kCurlSmallAllocation) { return nullptr; @@ -320,6 +348,44 @@ struct FailCurlCalloc FailCurlCalloc &operator=(FailCurlCalloc &&) = delete; }; +// How big a list node is on the libcurl this binary is linked against, or zero if an append +// does not reach the malloc callback at all. Taken by watching one, since libcurl promises +// none of it. The string is copied through the strdup callback, which is deliberately not the +// one being watched, so the only block this sees is the node. +inline size_t MeasureSlistNode() +{ + g_watched_malloc_size = 0; + g_watch_malloc_size = 1; + curl_slist *measured = curl_slist_append(nullptr, "X-Measure: 1"); + g_watch_malloc_size = 0; + + const size_t size = (nullptr != measured) ? g_watched_malloc_size : 0; + curl_slist_free_all(measured); + return size; +} + +// Lets a number of nodes through and refuses the next one, so a case can say which append +// fails rather than hoping it is the first thing libcurl asks for. +struct FailSlistNodeAfter +{ + FailSlistNodeAfter(int let_through, size_t node_size) + { + g_let_through_before_fail = let_through; + g_fail_malloc_of_size = node_size; + } + + ~FailSlistNodeAfter() + { + g_fail_malloc_of_size = 0; + g_let_through_before_fail = 0; + } + + FailSlistNodeAfter(const FailSlistNodeAfter &) = delete; + FailSlistNodeAfter(FailSlistNodeAfter &&) = delete; + FailSlistNodeAfter &operator=(const FailSlistNodeAfter &) = delete; + FailSlistNodeAfter &operator=(FailSlistNodeAfter &&) = delete; +}; + // The failure that reaches every thread, and the exemption for the one that armed it, since // they are armed together and have to be put away together. A guard because these two are // process wide and there are fatal assertions between arming and disarming: one of those @@ -1111,6 +1177,56 @@ TEST_F(BasicCurlHttpTests, AFailedHeaderAllocationIsReported) << "expected exactly one terminal outcome"; } +// The first header goes on and the second does not. That is the case the temporary pointer is +// there for: curl_slist_append returns null on failure and leaves the list it was given, so +// assigning its result straight back over the member loses everything appended so far. With one +// header there is nothing to lose, which is why the case above cannot tell the difference. +TEST_F(BasicCurlHttpTests, AHeaderListThatFailsPartWayThroughIsNotLost) +{ + ASSERT_TRUE(g_curl_hooks_installed); + received_requests_.clear(); + + const size_t node = MeasureSlistNode(); + if (0 == node) + { + GTEST_SKIP() << "list appends on this libcurl do not reach the malloc callback, so the " + << "failure cannot be aimed at one"; + } + + auto session_manager = std::make_shared()->Create(); + auto session = session_manager->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + request->AddHeader("X-First", "1"); + request->AddHeader("X-Second", "2"); + + auto handler = std::make_shared(); + { + // One node through, the next refused. Whichever append that turns out to be, the list is + // not empty when it happens, which is the whole point. + FailSlistNodeAfter failing{1, node}; + session->SendRequest(handler); + } + + session->FinishSession(); + session_manager->FinishAllSessions(); + + size_t requests_seen = 0; + { + std::unique_lock lock_requests(mtx_requests); + requests_seen = received_requests_.size(); + } + + EXPECT_TRUE(handler->create_failed_.load(std::memory_order_acquire)) + << "a header list that could not be finished was not reported"; + EXPECT_EQ(static_cast(0), requests_seen) + << "the request reached the server carrying a header list that was never finished"; + EXPECT_EQ(1, handler->terminal_count_.load(std::memory_order_acquire)) + << "expected exactly one terminal outcome"; + // LeakSanitizer is what says the nodes that were appended before the failure were freed + // rather than dropped. +} + // A client whose multi handle is null accepts sessions, adds none of them, and completes none // of them, so the failure has to be visible somewhere. TEST_F(BasicCurlHttpTests, AFailedMultiHandleAllocationIsReported) From acc4205140f9589679733ab0003922f25b809952 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:31:27 +0000 Subject: [PATCH 26/38] Say in the changelog what else this fixes Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce2f948ac7..bb5ae0b0c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,14 @@ Increment the: * [BUG] Stop the curl IO thread spinning, flooding the log, and refusing to be destroyed, when the multi handle cannot be created [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) +* [BUG] Keep the curl IO thread from retiring while a request it accepted is + still waiting to be scheduled, and release the easy handles and header lists + left queued when the client is destroyed + [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) +* [BUG] Take an operation that was cancelled or torn down out of the curl retry + queue, and check what curl_multi_remove_handle and curl_multi_add_handle + return when a retry is scheduled + [#4404](https://github.com/open-telemetry/opentelemetry-cpp/issues/4404) * [CONFIGURATION] Add SDK component builder interfaces to the registry [#4358](https://github.com/open-telemetry/opentelemetry-cpp/issues/4358) From 5c89d79b464d121ae8c5f7d1d83ae2d9ca63a476 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:47:30 +0000 Subject: [PATCH 27/38] [TEST] Destroy the client with a retry still pending, and nobody cancelling The case next to this one cancels first, which is not what an exporter shutting down does. It simply goes, and the entry left in the retry queue names an operation that still holds a promise and an easy handle. Somebody has to finish it and release them, and once the client has gone there is nobody. What answers it is the destructor cancelling every session it still has, which makes the entry stale, which the retry pass then drops. Both halves have to be there: against the queue check main has, the destructor takes 6.41, 6.41 and 6.41 seconds waiting out a backoff nobody is going to run, and with them it is 517 ms, three runs out of three, with no leak. The handler is declared outside the block on purpose. The terminal event for this one comes from the client's destructor, so it has to outlive the client rather than the other way round, which is the sort of thing a case can get wrong and only find out under a sanitizer. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 50 +++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 8551b2c7c9..a03c62a855 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1785,6 +1785,56 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData) // queued while the handle is missing leaves the pending queue for a multi function that cannot // take it, and the next reset cancels it, so the caller is told a request was cancelled that // nothing cancelled. +#ifdef ENABLE_OTLP_RETRY_PREVIEW +// The other half of the case above, and the one an exporter actually does: nothing cancels the +// request, the client is simply destroyed. The entry left in the retry queue names an operation +// that still holds a promise and an easy handle, so somebody has to be the one to finish it and +// release them, and after the client has gone there is nobody. +TEST_F(BasicCurlHttpTests, AClientDestroyedWithAPendingRetryDoesNotWaitForIt) +{ + received_requests_.clear(); + + // Outside the block on purpose. The terminal event for this one is dispatched from the + // client's destructor, so the handler has to outlive the client rather than the other way + // round. + auto handler = std::make_shared(); + + const auto started = std::chrono::steady_clock::now(); + { + auto client = std::make_shared()->Create(); + ASSERT_TRUE(client != nullptr); + auto *concrete = static_cast(client.get()); + + auto session = client->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("retry/"); + request->SetMethod(http_client::Method::Post); + + // Long enough that waiting for it would be unmistakable inside the bound below. + request->SetRetryPolicy( + {4, std::chrono::duration{8.0f}, std::chrono::duration{16.0f}, 2.0f}); + + session->SendRequest(handler); + + for (int i = 0; + i < 300 && 0 == http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete); ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + ASSERT_GE(http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete), 1u) + << "the request was never queued for retry, so nothing was tested"; + } + const auto elapsed = std::chrono::duration_cast( + std::chrono::steady_clock::now() - started); + + EXPECT_LT(elapsed.count(), 4000) << "destroying the client took " << elapsed.count() + << " ms, which is it waiting out a retry nobody is going to run"; + EXPECT_GE(handler->terminal_.load(std::memory_order_acquire), 1) + << "the request was neither retried nor finished"; + // LeakSanitizer is what says the easy handle and the header list went with it. +} +#endif // ENABLE_OTLP_RETRY_PREVIEW + #ifdef ENABLE_OTLP_RETRY_PREVIEW TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen) { From c3e8fac4b57adbb48752eb278db8d9dc647ea04e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:59:47 +0000 Subject: [PATCH 28/38] [TEST] Watch the retry from outside the client, not from inside its queue The two retry cases waited by polling the retry queue through a test peer. That queue has no lock, because only the background thread is supposed to touch it, and a peer reading it from the test thread makes that untrue. ThreadSanitizer says so: two data races, both this read against the background thread's push_back, and both of them mine rather than the client's. So the wait moved outside. It waits on what the server received, which is behind a mutex and a condition variable, and then for long enough that the answer has been read and the session put back with its wait on it. The wait is eight seconds and the settling is one, so a request still outstanding at that point is outstanding because it is queued, and there are seven seconds left for the case to be wrong in. The first attempt at this waited for the server to see a second request, on the grounds that a retry having fired proves the queue works. It does, but it proves it one step too late: at that moment the client is reading the second answer rather than holding a queued session, so cancelling caught a transfer in flight and the mutation check went green. Both cases now fail against a queue check that only drops entries with no operation, three runs out of three, at 5.40 seconds against a bound of four, and pass in about a second. ThreadSanitizer reports nothing on the whole binary now, and all 40 cases pass. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 62 +++++++++++++++------------------ 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index a03c62a855..b7d5bdde5e 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -62,13 +62,6 @@ class HttpClientTestPeer static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } -#ifdef ENABLE_OTLP_RETRY_PREVIEW - static std::size_t RetryQueueSize(const HttpClient &client) - { - return client.pending_to_retry_sessions_.size(); - } -#endif // ENABLE_OTLP_RETRY_PREVIEW - static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) { CURLM *previous = client.multi_handle_; @@ -1799,30 +1792,30 @@ TEST_F(BasicCurlHttpTests, AClientDestroyedWithAPendingRetryDoesNotWaitForIt) // round. auto handler = std::make_shared(); - const auto started = std::chrono::steady_clock::now(); + std::chrono::steady_clock::time_point started{}; { auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); - auto *concrete = static_cast(client.get()); auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("retry/"); request->SetMethod(http_client::Method::Post); - - // Long enough that waiting for it would be unmistakable inside the bound below. request->SetRetryPolicy( {4, std::chrono::duration{8.0f}, std::chrono::duration{16.0f}, 2.0f}); session->SendRequest(handler); - for (int i = 0; - i < 300 && 0 == http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete); ++i) - { - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } - ASSERT_GE(http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete), 1u) - << "the request was never queued for retry, so nothing was tested"; + ASSERT_TRUE(waitForRequests(30, 1)) << "the request never reached the server"; + std::this_thread::sleep_for(std::chrono::seconds(1)); + ASSERT_EQ(0, handler->terminal_.load(std::memory_order_acquire)) + << "the request had already finished, so nothing was pending"; + + started = std::chrono::steady_clock::now(); + + // Nothing cancels it. The client simply goes, which is what an exporter shutting down + // does, and the entry it leaves behind names an operation holding a promise and an easy + // handle. } const auto elapsed = std::chrono::duration_cast( std::chrono::steady_clock::now() - started); @@ -1840,34 +1833,37 @@ TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen) { received_requests_.clear(); - const auto started = std::chrono::steady_clock::now(); + auto handler = std::make_shared(); + std::chrono::steady_clock::time_point started{}; { auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); - auto *concrete = static_cast(client.get()); auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("retry/"); request->SetMethod(http_client::Method::Post); - // Long enough that waiting for it would be unmistakable, and long enough that it cannot - // come round on its own inside the bound below. + // Long enough that waiting it out would be unmistakable, and long enough that it cannot + // come round on its own while the case is still setting up. request->SetRetryPolicy( {4, std::chrono::duration{8.0f}, std::chrono::duration{16.0f}, 2.0f}); - auto handler = std::make_shared(); session->SendRequest(handler); - // The server answers this route with something retryable, so the session goes into the - // retry queue with a time on it that has not come. - for (int i = 0; - i < 300 && 0 == http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete); ++i) - { - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } - ASSERT_GE(http_client::curl::HttpClientTestPeer::RetryQueueSize(*concrete), 1u) - << "the request was never queued for retry, so nothing was tested"; + // Watched from outside the client. The retry queue has no lock, because only the + // background thread is supposed to touch it, and a peer that read it from here would make + // that untrue: ThreadSanitizer reports exactly that, twice, against a version of this case + // that did. So it waits on what the server saw, which is behind a mutex, and then for long + // enough that the answer has been read and the session put back with its wait on it. A + // second in, that wait still has seven to run, so anything outstanding here is outstanding + // because it is queued. + ASSERT_TRUE(waitForRequests(30, 1)) << "the request never reached the server"; + std::this_thread::sleep_for(std::chrono::seconds(1)); + ASSERT_EQ(0, handler->terminal_.load(std::memory_order_acquire)) + << "the request had already finished, so nothing was pending"; + + started = std::chrono::steady_clock::now(); // Cancelling takes the operation apart and hands its easy handle back, and the entry left // behind names an operation that is not going to be retried by anybody. @@ -1878,7 +1874,7 @@ TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen) std::chrono::steady_clock::now() - started); EXPECT_LT(elapsed.count(), 4000) - << "destroying the client took " << elapsed.count() + << "tearing the client down took " << elapsed.count() << " ms, which is it waiting out a retry for an operation that had already finished"; } #endif // ENABLE_OTLP_RETRY_PREVIEW From 6d505f34d31ceed8c9c523906cc388583982a541 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:29:15 +0000 Subject: [PATCH 29/38] [CHORE] Say what the code does, and let the cases outlive what talks to them A pass over the things this project's reviews ask for every time. Comments describing how the code got here rather than what it is. Three of them: the stale queue check explaining what an earlier version of itself did, the construction result explaining what dispatching from there used to cause, and a test comment citing the ThreadSanitizer run that made it what it is. All three now say what the code does, and the rest is in the commit history where it belongs. A comment that had drifted off its class. The one describing what MultiHandleOutcomeHandler counts had come to rest above CountingLogHandler, which counts something else entirely. It is back where it belongs, and the peer comment that pointed at "the case below" names the case instead, since the case is nine hundred lines below. The ordering that makes a queued request safe was not written down anywhere. ScheduleAddSession inserts the id before Session::SendRequest asks for a background thread, and the retirement check holds background_thread_m_ while it looks for work, so a thread on its way out either sees the id and stays or has already cleared background_thread_ and the spawn that follows makes a new one. Queue first, spawn second, and now it says so. Two cases had no assertion at all, leaving LeakSanitizer as the only thing that could fail them, which also meant an injection that stopped working read as a pass. Both now check that the handle really was queued before asking whether it was released. The count they use is taken under session_ids_m_, which every producer of that queue takes, unlike the retry queue which has no lock and is not a test's to read. And three cases declared their handler after the client. The client dispatches terminal events from its destructor, and members go in reverse, so the handler was already gone by the time it might be called. Nothing reached it, but that was luck rather than design. All three declare it first now. All 40 cases pass, and clean under AddressSanitizer with detect_leaks=1. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 7 ++- .../http/client/curl/http_operation_curl.cc | 4 +- ext/test/http/curl_http_test.cc | 49 +++++++++++++------ 3 files changed, 39 insertions(+), 21 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 05e804cf74..da7a2c38a5 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -1054,10 +1054,9 @@ bool HttpClient::hasActionableWork() std::lock_guard session_lock_guard{sessions_m_}; std::lock_guard session_id_lock_guard{session_ids_m_}; - // An id whose session has gone is what doAddSessions would drop on its next pass, so it is - // dropped here too rather than counted. The difference matters: the first version of this - // check took every queue at its size, and an entry nothing could be done about held the - // thread open against the join in the destructor. + // An id whose session has gone is what doAddSessions drops on its next pass, so it is + // dropped here too rather than counted. Counting an entry that can never drain would keep + // this thread alive against the join in the destructor. for (auto id = pending_to_add_session_ids_.begin(); id != pending_to_add_session_ids_.end();) { const auto session = sessions_.find(*id); diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 86efac4e27..2e3a709936 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -455,8 +455,8 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, last_curl_result_ = CURLE_FAILED_INIT; // Refuses Send() and SendAsync(), which would otherwise drive a null handle into libcurl. construction_result_ = CURLE_FAILED_INIT; - // Terminal already, and reported by the caller that sees SendAsync refuse, the same way the - // header list failure below is. Dispatching here as well told the handler twice. + // Terminal already, and reported once by the caller that sees SendAsync refuse, the same + // way the header list failure below is. Nothing is dispatched from here. session_state_ = opentelemetry::ext::http::client::SessionState::CreateFailed; return; } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index b7d5bdde5e..b976a2a2af 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -54,7 +54,7 @@ namespace client namespace curl { // resetMultiHandle only runs when curl_multi_perform fails, which a test cannot provoke, so -// the case below reaches it directly. See #4389. +// ResetMultiHandleWithASessionDoesNotDeadlock reaches it directly. See #4389. class HttpClientTestPeer { public: @@ -62,6 +62,14 @@ class HttpClientTestPeer static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + // Guarded by session_ids_m_, which is what every producer of this queue takes, so a case may + // read it whatever else is running. + static std::size_t PendingRemovalCount(HttpClient &client) + { + std::lock_guard lock_guard{client.session_ids_m_}; + return client.pending_to_remove_session_handles_.size(); + } + static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) { CURLM *previous = client.multi_handle_; @@ -412,9 +420,6 @@ struct FailCurlCallocEverywhere FailCurlCallocEverywhere &operator=(FailCurlCallocEverywhere &&) = delete; }; -// Counts terminal outcomes without caring which one, since a client whose multi handle could -// not be created may still recover and answer, and the case below is about the caller being -// told either way rather than about which answer it gets. // Counts the internal log lines that say a particular thing, so a case can hold that a run of // failures is reported a bounded number of times rather than once per pass of the IO loop. Only // the ones it asked for: anything else the binary writes while this is installed would move the @@ -471,6 +476,9 @@ class ScopedLogHandler const nostd::shared_ptr previous_; }; +// Counts terminal outcomes without caring which one, since a client whose multi handle could +// not be created may still recover and answer, and what the cases using this ask is whether the +// caller was told either way rather than which answer it got. class MultiHandleOutcomeHandler : public http_client::EventHandler { public: @@ -1851,13 +1859,12 @@ TEST_F(BasicCurlHttpTests, ACancelledRetryDoesNotHoldTheClientOpen) session->SendRequest(handler); - // Watched from outside the client. The retry queue has no lock, because only the - // background thread is supposed to touch it, and a peer that read it from here would make - // that untrue: ThreadSanitizer reports exactly that, twice, against a version of this case - // that did. So it waits on what the server saw, which is behind a mutex, and then for long - // enough that the answer has been read and the session put back with its wait on it. A - // second in, that wait still has seven to run, so anything outstanding here is outstanding - // because it is queued. + // Watched from outside the client, because the retry queue has no lock: only the + // background thread is meant to touch it, and reading it from here would make that untrue. + // So this waits on what the server saw, which is behind a mutex, and then long enough for + // the answer to have been read and the session put back with its wait on it. A second in, + // that wait still has seven to run, so anything outstanding here is outstanding because it + // is queued. ASSERT_TRUE(waitForRequests(30, 1)) << "the request never reached the server"; std::this_thread::sleep_for(std::chrono::seconds(1)); ASSERT_EQ(0, handler->terminal_.load(std::memory_order_acquire)) @@ -1889,6 +1896,9 @@ TEST_F(BasicCurlHttpTests, AQueuedHandleIsReleasedWithTheClientThatQueuedIt) resource.headers_chunk = curl_slist_append(nullptr, "X-Test: 1"); ASSERT_TRUE(resource.headers_chunk != nullptr); client->ScheduleRemoveSession(4404, std::move(resource)); + ASSERT_EQ(static_cast(1), + http_client::curl::HttpClientTestPeer::PendingRemovalCount(*client)) + << "the handle was never queued, so nothing was tested"; // Nothing was sent, so there is no IO thread and nobody else is coming for that queue. The // record is two raw pointers and the container that holds it frees neither, so what is still @@ -1911,6 +1921,9 @@ TEST_F(BasicCurlHttpTests, AHandleQueuedWithoutAMultiHandleIsReleased) CURLM *previous = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(*concrete, nullptr); concrete->ScheduleRemoveSession(4404, std::move(resource)); + ASSERT_EQ(static_cast(1), + http_client::curl::HttpClientTestPeer::PendingRemovalCount(*concrete)) + << "the handle was never queued, so nothing was tested"; // What resetMultiHandle does when curl_multi_init has just failed on it. The easy handle and // its header list are released rather than held until a multi handle comes back, and neither @@ -1929,6 +1942,9 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) // Built without a handle, for the reason given above: a case that took one away would be // using it from two threads at once, and could not then say whether what it saw was the // client's behaviour or its own. + // Before the client, so that it outlives it. The client dispatches terminal events from its + // destructor, and a handler declared after it would be gone by then. + auto handler = std::make_shared(); FailCurlCallocEverywhere failing; auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); @@ -1948,7 +1964,6 @@ TEST_F(BasicCurlHttpTests, AQueuedRequestSurvivesAMissingMultiHandle) auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("get/"); - auto handler = std::make_shared(); // Back to zero after the constructor's own attempt and before there is an IO thread to make // one, and this thread is exempt from here on, so what is counted below was refused to that @@ -1991,13 +2006,15 @@ TEST_F(BasicCurlHttpTests, AFailedEasyHandleIsReportedOnce) ASSERT_TRUE(g_curl_hooks_installed); received_requests_.clear(); - auto client = std::make_shared()->Create(); + // Before the client, so that it outlives it. The client dispatches terminal events from its + // destructor, and a handler declared after it would be gone by then. + auto handler = std::make_shared(); + auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("get/"); - auto handler = std::make_shared(); { // Armed here so it reaches the curl_easy_init inside SendRequest and nothing before it. @@ -2039,6 +2056,9 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) // would leave any result this case reported open to being an artefact of the injection. // What fails is curl_multi_init, on whichever thread calls it, and after the constructor // that is the IO thread every time. + // Before the client, so that it outlives it. The client dispatches terminal events from its + // destructor, and a handler declared after it would be gone by then. + auto handler = std::make_shared(); FailCurlCallocEverywhere failing; auto client = std::make_shared()->Create(); ASSERT_TRUE(client != nullptr); @@ -2052,7 +2072,6 @@ TEST_F(BasicCurlHttpTests, APersistentMultiHandleFailureDoesNotSpin) auto session = client->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); request->SetUri("get/"); - auto handler = std::make_shared(); // Back to zero after the constructor's own attempt and before there is an IO thread to // make one, and this thread is exempt from here on, so what is counted below was refused From b54e01a85de549fea5c603a2759d5ff4b2c32fb0 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:42:59 +0000 Subject: [PATCH 30/38] Take the include include-what-you-use asks for The failure paths added here report through OTEL_INTERNAL_LOG_ERROR with a streamed curl_multi_strerror, which needs ostream. All three iwyu presets ask for it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index da7a2c38a5..0d4c4ad96a 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include From e37fbf3338a25cb42798fae6618c9d635f8ddf1b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 07:40:25 +0000 Subject: [PATCH 31/38] Take the other include include-what-you-use asks for The peer that counts pending removals names the unordered_map the client keeps them in, and all three iwyu presets ask for it. The ostream one before this was the same shape on the other file. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index b976a2a2af..0ca6112a2f 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include From f6a3700292f02a87a43239603e421ee415a50612 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:27:51 +0000 Subject: [PATCH 32/38] [TEST] Refuse a header append by name, not by allocation size The case that holds a header list failing part way through aimed its injection at blocks of exactly the size of one list node, letting the first through and refusing the next. A node's size is not part of libcurl's contract, and neither is what gets allocated before the header list is built, so on the libcurl the conan jobs link against something else took the refusal: the list was built, the request went out, and the case failed on three platforms while the same code passed on every other job. It now names the header to refuse, and refuses the copy libcurl makes of that exact string, so which append fails is a property of the case rather than of the libcurl in use. It also records the order the two headers were copied in, so that a libcurl which never routes an appended string through the allocator callbacks skips instead of failing, and a refusal that landed on the first append is reported rather than passing as though the list had already been non-empty. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 141 +++++++++++++++++--------------- 1 file changed, 75 insertions(+), 66 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 0ca6112a2f..58d3f85d3e 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -235,37 +235,22 @@ static thread_local bool g_fail_curl_calloc = false; // failing allocation is otherwise a property of the libcurl in use rather than of this test. static const size_t kCurlSmallAllocation = 64; -// Measured rather than assumed. libcurl does not say which allocator a list append uses, how -// large a node is, or how many allocations anything else makes first, so a case that wants the -// failure to land on a particular append asks this file to watch one happen and then aims at -// blocks of exactly that size. Which append is the one that fails is a count, so a case can -// have the first go through and the second not. -static thread_local size_t g_watch_malloc_size = 0; -static thread_local size_t g_watched_malloc_size = 0; -static thread_local size_t g_fail_malloc_of_size = 0; -static thread_local int g_let_through_before_fail = 0; +// Named rather than measured. A case that wants one particular append to fail says which header +// it is, and the refusal happens when libcurl copies that exact string. Sizes and counts cannot +// do this: a node's size is not part of libcurl's contract, and anything allocated before the +// append can match the same size and consume the refusal, which leaves the list built and the +// case asserting against a path it never took. +static thread_local const char *g_refuse_strdup_of = nullptr; +static thread_local const char *g_watch_strdup_of = nullptr; +static thread_local int g_strdup_calls = 0; +static thread_local int g_strdup_refusals = 0; +static thread_local int g_watched_at = 0; +static thread_local int g_refused_at = 0; static void *CurlTestMalloc(size_t size) { g_curl_hooks_ran.store(true, std::memory_order_relaxed); - if (0 != g_watch_malloc_size && 0 == g_watched_malloc_size) - { - g_watched_malloc_size = size; - } - - if (0 != g_fail_malloc_of_size && size == g_fail_malloc_of_size) - { - if (g_let_through_before_fail > 0) - { - --g_let_through_before_fail; - } - else - { - return nullptr; - } - } - if (g_fail_curl_malloc && size <= kCurlSmallAllocation) { return nullptr; @@ -285,8 +270,29 @@ static void *CurlTestRealloc(void *ptr, size_t size) // Not routed through CurlTestMalloc on purpose, so that a failing malloc cannot reach the // copies libcurl makes of the caller's strings and land somewhere other than the list node. +// The one refusal it does make is aimed by content: curl_slist_append copies the string it is +// given before it has a list to return, so refusing that copy is what makes that append, and no +// other, return null. static char *CurlTestStrdup(const char *str) { + if (nullptr != str && (nullptr != g_watch_strdup_of || nullptr != g_refuse_strdup_of)) + { + ++g_strdup_calls; + + if (nullptr != g_watch_strdup_of && 0 == g_watched_at && + 0 == std::strcmp(str, g_watch_strdup_of)) + { + g_watched_at = g_strdup_calls; + } + + if (nullptr != g_refuse_strdup_of && 0 == std::strcmp(str, g_refuse_strdup_of)) + { + ++g_strdup_refusals; + g_refused_at = g_strdup_calls; + return nullptr; + } + } + const size_t length = std::strlen(str) + 1; char *copy = static_cast(std::malloc(length)); if (copy != nullptr) @@ -350,42 +356,32 @@ struct FailCurlCalloc FailCurlCalloc &operator=(FailCurlCalloc &&) = delete; }; -// How big a list node is on the libcurl this binary is linked against, or zero if an append -// does not reach the malloc callback at all. Taken by watching one, since libcurl promises -// none of it. The string is copied through the strdup callback, which is deliberately not the -// one being watched, so the only block this sees is the node. -inline size_t MeasureSlistNode() -{ - g_watched_malloc_size = 0; - g_watch_malloc_size = 1; - curl_slist *measured = curl_slist_append(nullptr, "X-Measure: 1"); - g_watch_malloc_size = 0; - - const size_t size = (nullptr != measured) ? g_watched_malloc_size : 0; - curl_slist_free_all(measured); - return size; -} - -// Lets a number of nodes through and refuses the next one, so a case can say which append -// fails rather than hoping it is the first thing libcurl asks for. -struct FailSlistNodeAfter +// Watches one header go on and refuses the next, both by name, and records the order the two +// were copied in. A case reads those back to say three different things apart: a libcurl that +// never routed an appended string here at all, an append that was refused as asked, and a +// refusal that landed on the first header rather than part way through a list. +struct RefuseSlistAppendOf { - FailSlistNodeAfter(int let_through, size_t node_size) + RefuseSlistAppendOf(const char *let_through, const char *refuse) { - g_let_through_before_fail = let_through; - g_fail_malloc_of_size = node_size; + g_strdup_calls = 0; + g_strdup_refusals = 0; + g_watched_at = 0; + g_refused_at = 0; + g_watch_strdup_of = let_through; + g_refuse_strdup_of = refuse; } - ~FailSlistNodeAfter() + ~RefuseSlistAppendOf() { - g_fail_malloc_of_size = 0; - g_let_through_before_fail = 0; + g_watch_strdup_of = nullptr; + g_refuse_strdup_of = nullptr; } - FailSlistNodeAfter(const FailSlistNodeAfter &) = delete; - FailSlistNodeAfter(FailSlistNodeAfter &&) = delete; - FailSlistNodeAfter &operator=(const FailSlistNodeAfter &) = delete; - FailSlistNodeAfter &operator=(FailSlistNodeAfter &&) = delete; + RefuseSlistAppendOf(const RefuseSlistAppendOf &) = delete; + RefuseSlistAppendOf(RefuseSlistAppendOf &&) = delete; + RefuseSlistAppendOf &operator=(const RefuseSlistAppendOf &) = delete; + RefuseSlistAppendOf &operator=(RefuseSlistAppendOf &&) = delete; }; // The failure that reaches every thread, and the exemption for the one that armed it, since @@ -1188,13 +1184,6 @@ TEST_F(BasicCurlHttpTests, AHeaderListThatFailsPartWayThroughIsNotLost) ASSERT_TRUE(g_curl_hooks_installed); received_requests_.clear(); - const size_t node = MeasureSlistNode(); - if (0 == node) - { - GTEST_SKIP() << "list appends on this libcurl do not reach the malloc callback, so the " - << "failure cannot be aimed at one"; - } - auto session_manager = std::make_shared()->Create(); auto session = session_manager->CreateSession("http://127.0.0.1:19000"); auto request = session->CreateRequest(); @@ -1202,12 +1191,19 @@ TEST_F(BasicCurlHttpTests, AHeaderListThatFailsPartWayThroughIsNotLost) request->AddHeader("X-First", "1"); request->AddHeader("X-Second", "2"); - auto handler = std::make_shared(); + auto handler = std::make_shared(); + int refusals = 0; + int let_through_at = 0; + int refused_at = 0; { - // One node through, the next refused. Whichever append that turns out to be, the list is - // not empty when it happens, which is the whole point. - FailSlistNodeAfter failing{1, node}; + // Named, not counted. Which append fails is then a property of this case rather than of how + // the libcurl in use sizes a node, or of what it happened to allocate before reaching the + // header list. + RefuseSlistAppendOf failing{"X-First: 1", "X-Second: 2"}; session->SendRequest(handler); + refusals = g_strdup_refusals; + let_through_at = g_watched_at; + refused_at = g_refused_at; } session->FinishSession(); @@ -1219,6 +1215,19 @@ TEST_F(BasicCurlHttpTests, AHeaderListThatFailsPartWayThroughIsNotLost) requests_seen = received_requests_.size(); } + if (0 == refusals && 0 == let_through_at) + { + GTEST_SKIP() << "this libcurl does not copy appended header strings through the allocator " + << "callbacks, so an append cannot be refused by name"; + } + ASSERT_EQ(1, refusals) << "this libcurl reached the allocator callbacks with the first header " + << "but never with the second, so nothing below is a statement about " + << "what happens when an append fails"; + ASSERT_GT(let_through_at, 0) << "the first header was never copied, so the refusal landed on " + << "an empty list and this case is not the one it says it is"; + ASSERT_LT(let_through_at, refused_at) << "the refusal came before the first header went on, so " + << "the list it landed on was still empty"; + EXPECT_TRUE(handler->create_failed_.load(std::memory_order_acquire)) << "a header list that could not be finished was not reported"; EXPECT_EQ(static_cast(0), requests_seen) From a5ea6154b7cd2adb587a78b74f96d87dff6094ac Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:58:42 +0000 Subject: [PATCH 33/38] [BUG] Ask one question about a retry entry, from both places that ask it hasActionableWork() dropped a retry entry whose session or operation had gone, and doRetrySessions() dropped those plus the ones whose operation was aborted or had already handed its easy handle back. The comment above the first said it applied the same rule as the second, and it no longer did. The gap shows while the client has no multi handle. doRetrySessions() returns early without one, so the entries it would discard are never discarded, while hasActionableWork() counts them as work and keeps the background thread alive retrying curl_multi_init for an operation that is already terminal. Shutdown is exempt from that check so it does not hang, but until the handle comes back the thread stays up, retries on a timer, logs, and holds the session and its resources for an entry nothing will ever retry. Both now call one function, which also puts the rule where it cannot drift again. It sits outside the retry preview guard because the scan runs in both builds: with the preview off the queue is empty rather than absent. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 28 +++++++++++++++----- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 0d4c4ad96a..111274acdf 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -959,6 +959,21 @@ bool HttpClient::doRemoveSessions() return has_data; } +namespace +{ +// One rule for the retry queue, shared by the pass that drains it and by the scan that decides +// whether this thread still owes anybody an answer. They walk the same container, so a predicate +// that disagreed would either keep the thread alive for an entry the retry pass is about to drop, +// or drop one the retry pass still wants. Outside the retry guard because the scan runs in both +// builds and the queue is empty rather than absent when the preview is off. +bool RetryEntryIsLive(const std::shared_ptr &session) +{ + const auto operation = session ? session->GetOperation().get() : nullptr; + return nullptr != operation && !operation->WasAborted() && + nullptr != operation->GetCurlEasyHandle(); +} +} // namespace + #ifdef ENABLE_OTLP_RETRY_PREVIEW bool HttpClient::doRetrySessions(bool report_all) { @@ -977,20 +992,20 @@ bool HttpClient::doRetrySessions(bool report_all) for (auto retry_it = pending_to_retry_sessions_.cbegin(); retry_it != pending_to_retry_sessions_.cend();) { - const auto session = *retry_it; - const auto operation = session ? session->GetOperation().get() : nullptr; + const auto session = *retry_it; // An operation that was cancelled, or torn down, is not going to be retried. Its easy // handle has gone back to the client already, so what waiting for its turn would buy is a // null handle offered to libcurl and an entry that keeps the background thread alive until // a time that means nothing. At shutdown that time is time the join spends waiting. - if (nullptr == operation || operation->WasAborted() || - nullptr == operation->GetCurlEasyHandle()) + if (!RetryEntryIsLive(session)) { retry_it = pending_to_retry_sessions_.erase(retry_it); continue; } + const auto operation = session->GetOperation().get(); + if (operation->NextRetryTime() >= now) { // Pushed at the back, so nothing behind this one is due either. @@ -1071,10 +1086,11 @@ bool HttpClient::hasActionableWork() } } - // Same rule doRetrySessions applies to the same container. + // The same rule doRetrySessions applies to the same container, from the same function, so the + // two cannot drift apart again. for (auto retry = pending_to_retry_sessions_.begin(); retry != pending_to_retry_sessions_.end();) { - if (!*retry || !(*retry)->GetOperation()) + if (!RetryEntryIsLive(*retry)) { retry = pending_to_retry_sessions_.erase(retry); } From 3de8ca226fd25502fd07279fc83cda649ba5e038 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 20:39:02 +0000 Subject: [PATCH 34/38] [BUG] Do not retire on a queue that filled up while this thread was draining it The wakeup generation this branch added closed the lost wakeup for a thread sleeping without a multi handle. The thread that is about to retire had the same hole and did not consult it. Everything that queues work for the background thread bumps the generation, and the producers of the abort and removal queues only wake it: unlike SendRequest they never call MaybeSpawnBackgroundThread. So an abort queued after the drains above have already reported nothing sits there until the next request or the destructor, and a caller waiting on that operation's promise waits with it, because the promise is fulfilled by the cleanup the drain would have run. The generation is read after the lock and before the drains, and compared after them, so the pass that queued something goes round once more and retires on the next one when the queues really are empty. A drain that queues a removal of its own therefore costs one extra iteration rather than keeping the thread up: the suite finishes in the same 29 seconds it did before, and the retirement cases still retire. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 111274acdf..1eea98ae08 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -683,6 +683,13 @@ bool HttpClient::MaybeSpawnBackgroundThread() // Double check, make sure no more pending sessions after locking background thread // management + // Read before the drains below, compared after them. Everything that queues work for + // this thread bumps it, and the producers of the abort and removal queues only wake + // this thread rather than starting one, so anything queued after a drain has already + // reported empty would sit there until the next request or the destructor. + const uint64_t generation_before = + self->wakeup_generation_.load(std::memory_order_acquire); + // Abort all pending easy handles if (self->doAbortSessions()) { @@ -718,6 +725,14 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + // Queued while the drains above were running, so this thread still owes somebody + // the work rather than being finished with it. + if (still_running == 0 && + generation_before != self->wakeup_generation_.load(std::memory_order_acquire)) + { + still_running = 1; + } + // If there is no pending jobs, we can stop the background thread. if (still_running == 0) { From 6098d3ec6f4537c0315b97614df39a5e4012d059 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 16 Aug 2026 14:20:11 +0000 Subject: [PATCH 35/38] [BUG] Report a failed multi cleanup after the mutex, not under it ReleaseMultiHandle() logged its own failure, and both callers hold multi_handle_m_ while they call it. The log handler is whatever the application installed, and one that comes back into this client reaches wakeupBackgroundThread(), which takes that same mutex on the same thread. std::mutex is not recursive, so the failure path could deadlock the thread reporting it. It now answers with what curl_multi_cleanup said and each caller reports after its lock scope closes. The second one has to hold the lock past the cleanup anyway, because it builds the replacement handle under it, so it carries the result out instead. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 6 ++- ext/src/http/client/curl/http_client_curl.cc | 50 +++++++++++++------ 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 4ecb308b14..0fee9c88e0 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -370,8 +370,10 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // has no lock because that thread is the only one that touches it. bool hasActionableWork(); // Cleans up the multi handle if there is one, and leaves none behind either way. Call it - // holding multi_handle_m_. - void ReleaseMultiHandle(); + // holding multi_handle_m_. It answers with what curl_multi_cleanup said rather than reporting + // it, because reporting reaches a log handler the application supplies, and one that comes back + // into this client would do it while the caller still holds that mutex. + CURLMcode ReleaseMultiHandle(); // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 1eea98ae08..808eb0648b 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -346,9 +346,19 @@ HttpClient::~HttpClient() doAbortSessions(); doRemoveSessions(); + CURLMcode cleanup_result = CURLM_OK; { std::lock_guard lock_guard{multi_handle_m_}; - ReleaseMultiHandle(); + cleanup_result = ReleaseMultiHandle(); + } + + // Outside the lock: the log handler is replaceable application code, and one that comes back + // into this client reaches wakeupBackgroundThread(), which takes the mutex this thread would + // still be holding. + if (CURLM_OK != cleanup_result) + { + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_cleanup failed with message: " + << curl_multi_strerror(cleanup_result)); } } @@ -1060,7 +1070,7 @@ bool HttpClient::doRetrySessions(bool /* report_all */) } #endif // ENABLE_OTLP_RETRY_PREVIEW -void HttpClient::ReleaseMultiHandle() +CURLMcode HttpClient::ReleaseMultiHandle() { if (nullptr == multi_handle_) { @@ -1068,16 +1078,12 @@ void HttpClient::ReleaseMultiHandle() // and curl_multi_cleanup is one of them. Reaching here with none is ordinary: the // constructor may have started without one, and a reset that could not build a replacement // leaves none behind. - return; + return CURLM_OK; } const CURLMcode cleanup_result = curl_multi_cleanup(multi_handle_); multi_handle_ = nullptr; - if (CURLM_OK != cleanup_result) - { - OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_cleanup failed with message: " - << curl_multi_strerror(cleanup_result)); - } + return cleanup_result; } bool HttpClient::hasActionableWork() @@ -1147,14 +1153,28 @@ bool HttpClient::resetMultiHandle() doRemoveSessions(); - // We will modify the multi_handle_, so we need to lock it - std::lock_guard lock_guard{multi_handle_m_}; - ReleaseMultiHandle(); + CURLMcode cleanup_result = CURLM_OK; + bool have_handle = false; + { + // We will modify the multi_handle_, so we need to lock it + std::lock_guard lock_guard{multi_handle_m_}; + cleanup_result = ReleaseMultiHandle(); + + // Create a another multi handle to continue pending sessions. Silent on failure: the caller + // decides how often a run of failures is worth reporting. + multi_handle_ = curl_multi_init(); + have_handle = (nullptr != multi_handle_); + } + + // Outside the lock, for the same reason as the other caller: a log handler that comes back into + // this client takes this mutex. + if (CURLM_OK != cleanup_result) + { + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_cleanup failed with message: " + << curl_multi_strerror(cleanup_result)); + } - // Create a another multi handle to continue pending sessions. Silent on failure: the caller - // decides how often a run of failures is worth reporting. - multi_handle_ = curl_multi_init(); - return nullptr != multi_handle_; + return have_handle; } } // namespace curl From fb706555bb072dcd1f37622bc910a5cc8c1747f9 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 26 Aug 2026 21:28:32 +0000 Subject: [PATCH 36/38] [TEST] Cover the TLS version range and cipher list that Setup() applies Setup() reads ssl_min_tls, ssl_max_tls and ssl_cipher and returns early if curl refuses any of them, and nothing reached that code: gcovr reported those three lines at zero hits over the whole suite. Two cases now do. The first passes a valid range and a cipher list and requires the request to succeed, which is the branch that sets CURLOPT_SSLVERSION and CURLOPT_SSL_CIPHER_LIST. The second passes a version the parser does not know and requires CURLE_UNKNOWN_OPTION, which is the branch that refuses it before curl sees it. Both set use_ssl. Without it the whole block is skipped and the cases pass having exercised none of it, which is how the first draft of them read as green. Both also sit outside ENABLE_OTLP_RETRY_PREVIEW: a case that compiles out is still registered by gtest_add_tests, and a filter matching nothing exits zero. Measured after: the three lines report one hit each, and the suite passes with the retry preview on and off. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 58d3f85d3e..7e0dcd6e2d 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -860,6 +860,44 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations) delete handler; } +// Setup() applies the TLS version range and the cipher list to the easy handle, and returns early +// on either if curl rejects it. A plain http request reaches both, so the outcome says the options +// were accepted rather than that a handshake succeeded. +TEST_F(BasicCurlHttpTests, TlsVersionRangeAndCipherListAreAccepted) +{ + RetryEventHandler handler; + http_client::HttpSslOptions ssl_options; + ssl_options.use_ssl = true; + ssl_options.ssl_min_tls = "1.2"; + ssl_options.ssl_max_tls = "1.3"; + ssl_options.ssl_cipher = "ECDHE-RSA-AES128-GCM-SHA256"; + http_client::Body body; + http_client::Headers headers; + + curl::HttpOperation operation(http_client::Method::Get, "http://127.0.0.1:19000/get/", + ssl_options, &handler, headers, body); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_EQ(200, operation.GetResponseCode()); +} + +// An unknown version is refused rather than passed to curl, which is the branch above returning +// before CURLOPT_SSLVERSION is set at all. +TEST_F(BasicCurlHttpTests, AnUnknownTlsVersionIsRefused) +{ + RetryEventHandler handler; + http_client::HttpSslOptions ssl_options; + ssl_options.use_ssl = true; + ssl_options.ssl_min_tls = "1.1"; + http_client::Body body; + http_client::Headers headers; + + curl::HttpOperation operation(http_client::Method::Get, "http://127.0.0.1:19000/get/", + ssl_options, &handler, headers, body); + + ASSERT_EQ(CURLE_UNKNOWN_OPTION, operation.Send()); +} + #ifdef ENABLE_OTLP_RETRY_PREVIEW TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) { From 916bd48777fb346ae429a44a2cb9860e7e252581 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:10:06 +0000 Subject: [PATCH 37/38] [TEST] Name every constructor argument in the new cases HttpOperation keeps references to its ssl options, headers, body, compression and retry policy. The short constructor form materialises the defaulted ones as temporaries that die at the end of the full expression, and Setup() reads them from inside Send(): the Bazel asan job reported stack-use-after-scope in HttpOperation::Setup() with the frame belonging to the case. RetryPolicyEnabled in this file passes all twelve by name for that reason. The case I patterned on, RetryJitterIsNotSharedAcrossThreads, uses the short form and never calls Send(), so it never sees it. Verified with bazel test --config=asan on the same target: no sanitizer report, and the new cases run. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 7e0dcd6e2d..0274c2e8e0 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -874,8 +874,14 @@ TEST_F(BasicCurlHttpTests, TlsVersionRangeAndCipherListAreAccepted) http_client::Body body; http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy; + + // Every argument is named. The defaulted ones would be temporaries, and the operation keeps + // references to them past the end of this expression. curl::HttpOperation operation(http_client::Method::Get, "http://127.0.0.1:19000/get/", - ssl_options, &handler, headers, body); + ssl_options, &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); ASSERT_EQ(CURLE_OK, operation.Send()); ASSERT_EQ(200, operation.GetResponseCode()); @@ -892,8 +898,14 @@ TEST_F(BasicCurlHttpTests, AnUnknownTlsVersionIsRefused) http_client::Body body; http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy; + + // Every argument is named. The defaulted ones would be temporaries, and the operation keeps + // references to them past the end of this expression. curl::HttpOperation operation(http_client::Method::Get, "http://127.0.0.1:19000/get/", - ssl_options, &handler, headers, body); + ssl_options, &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); ASSERT_EQ(CURLE_UNKNOWN_OPTION, operation.Send()); } From 6640ce096db70685143984963158ba6b7d42ae8b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:21:35 +0000 Subject: [PATCH 38/38] [BUGFIX] Declare the global initializer before the multi handle curl_multi_init() may not run before curl_global_init(), so multi_handle_ was assigned in the constructor body with a comment saying why. clang-tidy reads that as prefer-member-initializer and reports it twice, which puts the abiv1-preview preset two warnings over its limit. Members are initialised in declaration order, so declaring the initializer first lets the list do it and makes the ordering a property of the class rather than of a comment a later edit can move away from. It also runs the global cleanup last on the way out rather than first, since destruction is the reverse. The destructor already calls curl_multi_cleanup() in its body, before any member is destroyed, so nothing depended on the old order there. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 6 +++-- ext/src/http/client/curl/http_client_curl.cc | 22 +++++++------------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 0fee9c88e0..38dc87ef13 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -377,6 +377,10 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // Returns true if the client has a multi handle afterwards. bool resetMultiHandle(); + // Declared before multi_handle_ on purpose: members are initialised in declaration + // order, and curl_multi_init() may not run before curl_global_init(). + nostd::shared_ptr curl_global_initializer_; + std::mutex multi_handle_m_; CURLM *multi_handle_; std::atomic next_session_id_{0}; @@ -402,8 +406,6 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // curl_multi_poll and it needs a multi handle, so the wait taken when there is none watches // this instead. std::atomic wakeup_generation_{0}; - - nostd::shared_ptr curl_global_initializer_; }; } // namespace curl diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 808eb0648b..50307fc50d 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -286,31 +286,25 @@ static CURLM *initMultiHandle() } HttpClient::HttpClient() - : multi_handle_(nullptr), + : curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()), + multi_handle_(initMultiHandle()), next_session_id_{0}, max_sessions_per_connection_{8}, background_thread_instrumentation_(nullptr), scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, - background_thread_wait_for_{std::chrono::minutes{1}}, - curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) -{ - // Not in the initialiser list: curl_global_initializer_ is declared later and has to run first. - multi_handle_ = initMultiHandle(); -} + background_thread_wait_for_{std::chrono::minutes{1}} +{} HttpClient::HttpClient( const std::shared_ptr &thread_instrumentation) - : multi_handle_(nullptr), + : curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()), + multi_handle_(initMultiHandle()), next_session_id_{0}, max_sessions_per_connection_{8}, background_thread_instrumentation_(thread_instrumentation), scheduled_delay_milliseconds_{std::chrono::milliseconds(256)}, - background_thread_wait_for_{std::chrono::minutes{1}}, - curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()) -{ - // Not in the initialiser list: curl_global_initializer_ is declared later and has to run first. - multi_handle_ = initMultiHandle(); -} + background_thread_wait_for_{std::chrono::minutes{1}} +{} HttpClient::~HttpClient() {