From ce59405eb758e99d27a26dec4aa2499f6ea29da4 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:52:50 +0000 Subject: [PATCH 01/10] [BUG] Remove a curl easy handle before releasing what it points at Teardown touched the easy handle in the wrong order and on the wrong thread. Cleanup cleared CURLOPT_PRIVATE and called curl_easy_reset on it, from whichever thread ran the cancel, while the transfer could still be active. doRemoveSessions then freed the header list before curl_multi_remove_handle and cleaned the handle up whatever that call returned. libcurl is explicit on all three: changing options while a transfer is in progress may have undefined behaviour, the header list has to outlive the handle that points at it, and a handle has to leave the multi handle before it can be cleaned up. Cleanup now hands the resource over untouched. doRemoveSessions removes it, checks the CURLMcode, and only then frees the list and the handle. A failed removal leaves both alone: leaking one easy handle is better than freeing one the multi stack may still own. A handle that was never added reports CURLM_OK, so no separate bookkeeping is needed to tell the two apart. Clearing CURLOPT_PRIVATE was load bearing rather than tidy up. The IO loop reads it back to decide whether a CURLMSG_DONE belongs to a session that has already gone, so PerformCurlMessage checks is_cleaned_ instead. That keeps the behaviour without writing to a handle from a thread that does not own it. Part of #4391. The retry path still calls curl_multi_remove_handle and curl_multi_add_handle without checking either result, so the issue stays open. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 ++ ext/src/http/client/curl/http_client_curl.cc | 35 +++++++++++++++---- .../http/client/curl/http_operation_curl.cc | 18 ++++++---- 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc64aaef23..32df5f7744 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] Remove a curl easy handle from the multi handle before freeing that + handle and the header list it points at + [#4391](https://github.com/open-telemetry/opentelemetry-cpp/issues/4391) * [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 76f67fcd90..3cbb2eb0a0 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 @@ -23,6 +24,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 +35,6 @@ # include # include "opentelemetry/nostd/type_traits.h" -#else -# include "opentelemetry/sdk/common/global_log_handler.h" #endif OPENTELEMETRY_BEGIN_NAMESPACE @@ -811,13 +811,36 @@ bool HttpClient::doRemoveSessions() for (auto &removing_handle : pending_to_remove_session_handles) { - if (nullptr != removing_handle.second.headers_chunk) + auto &resource = removing_handle.second; + if (nullptr == resource.easy_handle) + { + continue; + } + + // Take it out of the multi handle first. libcurl does not allow an easy handle to be + // cleaned up while a multi handle still owns it, and the header list has to stay alive + // until the handle is no longer used for a transfer. A handle that was never added + // reports CURLM_OK here, so this does not need to know which is which. + const CURLMcode rc = curl_multi_remove_handle(multi_handle_, resource.easy_handle); + if (CURLM_OK != rc) + { + // The multi handle may still own it. Leaking one easy handle is the better half of + // this trade: freeing it here would corrupt whatever still holds it. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_remove_handle failed, leaving " + "the handle alone: " + << curl_multi_strerror(rc)); + continue; + } + + if (nullptr != resource.headers_chunk) { - curl_slist_free_all(removing_handle.second.headers_chunk); + curl_slist_free_all(resource.headers_chunk); + resource.headers_chunk = nullptr; } - curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle); - curl_easy_cleanup(removing_handle.second.easy_handle); + curl_easy_cleanup(resource.easy_handle); + resource.easy_handle = nullptr; } for (auto &removing_session : pending_to_remove_sessions) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 0f1bda4035..f6aa81d81a 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -542,15 +542,13 @@ void HttpOperation::Cleanup() // Only cleanup async once even in recursive calls if (async_data_) { - // Just reset and move easy_handle to owner if in async mode + // Move the easy handle to the owner untouched. It may still be attached to the multi handle + // and running, and the thread inside curl_multi_perform owns it until the IO thread takes it + // out, so writing to it here would be a write to a live transfer. It is pointless as well: + // the next thing that happens to the handle is curl_easy_cleanup. Session *session = async_data_->session.exchange(nullptr, std::memory_order_acq_rel); if (session != nullptr) { - if (curl_resource_.easy_handle != nullptr) - { - curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, NULL); - curl_easy_reset(curl_resource_.easy_handle); - } session->GetHttpClient().ScheduleRemoveSession(session->GetSessionId(), std::move(curl_resource_)); } @@ -1525,6 +1523,14 @@ void HttpOperation::Abort() void HttpOperation::PerformCurlMessage(CURLcode code) { + if (is_cleaned_.load(std::memory_order_acquire)) + { + // Already torn down, and the handle is queued for removal. The multi handle can still hold a + // message buffered from before that, and acting on it would dispatch a second round of + // terminal events and can put a handle that is waiting to be freed back on the retry queue. + return; + } + ++retry_attempts_; last_attempt_time_ = std::chrono::system_clock::now(); last_curl_result_ = code; From 388d3dbe9541df30ce227ced016ddc0b0562009f Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:06:12 +0000 Subject: [PATCH 02/10] [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/src/http/client/curl/http_client_curl.cc | 6 ++---- ext/src/http/client/curl/http_operation_curl.cc | 11 ++++------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 3cbb2eb0a0..276d6ffa27 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -817,10 +817,8 @@ bool HttpClient::doRemoveSessions() continue; } - // Take it out of the multi handle first. libcurl does not allow an easy handle to be - // cleaned up while a multi handle still owns it, and the header list has to stay alive - // until the handle is no longer used for a transfer. A handle that was never added - // reports CURLM_OK here, so this does not need to know which is which. + // Remove before cleanup: libcurl forbids freeing an easy handle, or the header list it + // points at, while the multi handle still owns it. A handle never added reports CURLM_OK. const CURLMcode rc = curl_multi_remove_handle(multi_handle_, resource.easy_handle); if (CURLM_OK != rc) { diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index f6aa81d81a..0384d3370c 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -542,10 +542,8 @@ void HttpOperation::Cleanup() // Only cleanup async once even in recursive calls if (async_data_) { - // Move the easy handle to the owner untouched. It may still be attached to the multi handle - // and running, and the thread inside curl_multi_perform owns it until the IO thread takes it - // out, so writing to it here would be a write to a live transfer. It is pointless as well: - // the next thing that happens to the handle is curl_easy_cleanup. + // Hand the easy handle over untouched. It may still be attached to the multi handle, and the + // thread inside curl_multi_perform owns it until the IO thread removes it. Session *session = async_data_->session.exchange(nullptr, std::memory_order_acq_rel); if (session != nullptr) { @@ -1525,9 +1523,8 @@ void HttpOperation::PerformCurlMessage(CURLcode code) { if (is_cleaned_.load(std::memory_order_acquire)) { - // Already torn down, and the handle is queued for removal. The multi handle can still hold a - // message buffered from before that, and acting on it would dispatch a second round of - // terminal events and can put a handle that is waiting to be freed back on the retry queue. + // Already torn down and queued for removal. A message buffered before that would dispatch a + // second round of terminal events and requeue a handle waiting to be freed. return; } From f905aa0efb3e4ff761f5aaf6b0fc4bce0d4a26f7 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:41:59 +0000 Subject: [PATCH 03/10] [BUG] Keep an aborted session alive until its handle is freed Handing the easy handle over untouched leaves CURLOPT_PRIVATE and the write, header, read and progress callback data pointing at the session and its operation. Clearing them was what made that safe before, and this branch stopped clearing them on purpose, because the cancelling thread does not own a handle the multi handle may still be driving. The owner then has to outlive the handle, and on the abort path it did not. ScheduleAbortSession takes the session out of sessions_, so doRemoveSessions cannot find it to hold one, and doAbortSessions kept the only remaining shared_ptr in a local map that died on return. The handle could reach curl_multi_remove_handle and curl_easy_cleanup with those pointers dangling, and curl_easy_cleanup is documented as able to run the progress and header callbacks. pending_to_remove_sessions_ already exists for this: doRemoveSessions swaps it out and holds it until the handle is freed. The abort path hands the session to it now. 26 tests pass, 3 of 3, and under AddressSanitizer with leak detection 2 of 2 with no leaks and no errors. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 276d6ffa27..07f42c3a1f 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -765,7 +765,7 @@ bool HttpClient::doAbortSessions() } bool has_data = false; - for (const auto &session : pending_to_abort_sessions) + for (auto &session : pending_to_abort_sessions) { if (!session.second) { @@ -777,6 +777,15 @@ bool HttpClient::doAbortSessions() session.second->FinishOperation(); has_data = true; } + + // FinishOperation queued the easy handle for removal, and that handle still holds + // CURLOPT_PRIVATE and the callback data pointing into this session and its operation. + // doRemoveSessions cannot find the session to hold, because aborting took it out of + // sessions_, so hand it the owner directly: it keeps one until the handle is freed. + { + std::lock_guard session_id_lock_guard{session_ids_m_}; + pending_to_remove_sessions_.emplace_back(std::move(session.second)); + } } return has_data; } From ba35858ea646eac4d1069b51b632b4c1fc2f6ae0 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:14:16 +0000 Subject: [PATCH 04/10] [BUG] Record what the multi handle accepted instead of reading it back curl_multi_remove_handle reports whether that call succeeded, not whether the multi handle was holding the easy handle. libcurl 8.10 and 8.11 refuse a handle the multi handle does not hold, and a multi handle that failed to initialize refuses every one, so reading the refusal as ownership dropped the record: the easy handle, the header list it points at and the session it names went with it, while the message loop still reads that session back out of CURLOPT_PRIVATE. The client now records an easy handle when curl_multi_add_handle accepts it, asks for no removal of one that was never added, and keeps the whole record, session included, when libcurl will not give a handle back. Kept handles are freed once curl_multi_cleanup has detached them. The queue holds one record per easy handle rather than one per session, so a session that starts another request no longer displaces the handle its previous request left behind, and the destructor releases what the background thread did not get to. The retry path goes through the same accounting, and reports a re-arm that libcurl would not accept rather than dropping the session from the queue in silence. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +- .../ext/http/client/curl/http_client_curl.h | 31 ++- ext/src/http/client/curl/http_client_curl.cc | 197 ++++++++++++++---- ext/test/http/curl_http_test.cc | 161 +++++++++++++- 4 files changed, 349 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32df5f7744..0f7d302b9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,7 +72,8 @@ Increment the: `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents ([#4349](https://github.com/open-telemetry/opentelemetry-cpp/pull/4349)) * [BUG] Remove a curl easy handle from the multi handle before freeing that - handle and the header list it points at + handle and the header list it points at, and keep both when libcurl will not + take the handle back [#4391](https://github.com/open-telemetry/opentelemetry-cpp/issues/4391) * [CONFIGURATION] Add SDK component builder interfaces to the registry 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..697f27de43 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 @@ -366,6 +366,9 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doRetrySessions(bool report_all); void resetMultiHandle(); + bool detachHandle(CURL *easy_handle); + void releaseQuarantinedHandles(); + std::mutex multi_handle_m_; CURLM *multi_handle_; std::atomic next_session_id_{0}; @@ -376,8 +379,32 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unordered_map> sessions_; std::unordered_set pending_to_add_session_ids_; std::unordered_map> pending_to_abort_sessions_; - std::unordered_map pending_to_remove_session_handles_; - std::list> pending_to_remove_sessions_; + // One easy handle on its way back to libcurl. There is one of these per handle rather than + // per session, because a session that starts another request hands over a second handle while + // the first is still queued, and one entry per session would drop the first one, leaving its + // easy handle and header list with no owner. The session is only filled in when the handle + // has to be kept, and names what the handle still points at. + struct PendingCurlRemoval + { + uint64_t session_id; + HttpCurlEasyResource resource; + std::shared_ptr owner; + }; + + std::list pending_to_remove_session_handles_; + std::unordered_map> pending_to_remove_sessions_; + + // Which easy handles the multi handle holds, recorded when curl_multi_add_handle accepts one. + // What curl_multi_remove_handle returns cannot answer that question afterwards: it reports + // whether the removal succeeded, and libcurl 8.10 and 8.11 reject a handle the multi handle + // does not hold where every other version accepts it. Background thread only. + std::unordered_set attached_handles_; + + // Handles libcurl would not give back. A transfer may still be running on one, and its + // CURLOPT_PRIVATE names the session, so freeing either would leave libcurl and this client + // reading storage that has been released. Both are held until curl_multi_cleanup detaches + // the handle, which is where they are freed. + std::list quarantined_handles_; std::deque> pending_to_retry_sessions_; std::mutex background_thread_m_; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 07f42c3a1f..7bd84b9b1c 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -47,6 +47,26 @@ namespace client namespace curl { +namespace +{ +// The easy handle goes first. libcurl does not copy the header list, so it is in use for as long +// as anything is still transferring on the handle that points at it. +void ReleaseCurlResource(HttpCurlEasyResource &resource) noexcept +{ + if (nullptr != resource.easy_handle) + { + curl_easy_cleanup(resource.easy_handle); + resource.easy_handle = nullptr; + } + + if (nullptr != resource.headers_chunk) + { + curl_slist_free_all(resource.headers_chunk); + resource.headers_chunk = nullptr; + } +} +} // namespace + HttpCurlGlobalInitializer::HttpCurlGlobalInitializer() { curl_global_init(CURL_GLOBAL_ALL); @@ -318,7 +338,31 @@ HttpClient::~HttpClient() { std::lock_guard lock_guard{multi_handle_m_}; curl_multi_cleanup(multi_handle_); + + // Nothing is attached to a multi handle that no longer exists, and clearing the pointer + // keeps anything released below from reaching for the one just destroyed. + attached_handles_.clear(); + multi_handle_ = nullptr; + } + + // The background thread has stopped, so nothing else is coming back for what it left behind. + // Sessions go first, because releasing one can hand over one last easy handle, and they are + // released outside the lock because that hand over takes it. + releaseQuarantinedHandles(); + { + std::unordered_map> pending_to_remove_sessions; + { + std::lock_guard session_id_lock_guard{session_ids_m_}; + pending_to_remove_sessions_.swap(pending_to_remove_sessions); + } + } + + std::lock_guard session_id_lock_guard{session_ids_m_}; + for (auto &pending : pending_to_remove_session_handles_) + { + ReleaseCurlResource(pending.resource); } + pending_to_remove_session_handles_.clear(); } std::shared_ptr HttpClient::CreateSession( @@ -396,10 +440,19 @@ void HttpClient::CleanupSession(uint64_t session_id) if (session) { - if (pending_to_remove_session_handles_.end() != - pending_to_remove_session_handles_.find(session_id)) + bool has_pending_handle = false; + for (const auto &pending : pending_to_remove_session_handles_) { - pending_to_remove_sessions_.emplace_back(std::move(session)); + if (pending.session_id == session_id) + { + has_pending_handle = true; + break; + } + } + + if (has_pending_handle) + { + pending_to_remove_sessions_.emplace(session_id, std::move(session)); } else if (session->IsSessionActive() && session->GetOperation()) { @@ -643,7 +696,6 @@ void HttpClient::ScheduleAddSession(uint64_t session_id) { std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.insert(session_id); - pending_to_remove_session_handles_.erase(session_id); pending_to_abort_sessions_.erase(session_id); } @@ -678,7 +730,8 @@ void HttpClient::ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource { std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.erase(session_id); - pending_to_remove_session_handles_[session_id] = std::move(resource); + pending_to_remove_session_handles_.push_back( + PendingCurlRemoval{session_id, std::move(resource), nullptr}); } wakeupBackgroundThread(); @@ -749,7 +802,18 @@ bool HttpClient::doAddSessions() continue; } - curl_multi_add_handle(multi_handle_, easy_handle); + const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle); + if (CURLM_OK != rc) + { + // The request never starts. Leaving the handle unrecorded is what stops the release path + // from asking the multi handle to give back something it was never given. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_add_handle failed, this request will not be sent: " + << curl_multi_strerror(rc)); + continue; + } + + attached_handles_.insert(easy_handle); has_data = true; } @@ -781,10 +845,10 @@ bool HttpClient::doAbortSessions() // FinishOperation queued the easy handle for removal, and that handle still holds // CURLOPT_PRIVATE and the callback data pointing into this session and its operation. // doRemoveSessions cannot find the session to hold, because aborting took it out of - // sessions_, so hand it the owner directly: it keeps one until the handle is freed. + // sessions_, so hand it over under the id it will look the handle up by. { std::lock_guard session_id_lock_guard{session_ids_m_}; - pending_to_remove_sessions_.emplace_back(std::move(session.second)); + pending_to_remove_sessions_.emplace(session.first, std::move(session.second)); } } return has_data; @@ -796,8 +860,8 @@ bool HttpClient::doRemoveSessions() bool should_continue{false}; do { - std::unordered_map pending_to_remove_session_handles; - std::list> pending_to_remove_sessions; + std::list pending_to_remove_session_handles; + std::unordered_map> pending_to_remove_sessions; { std::lock_guard session_id_lock_guard{session_ids_m_}; pending_to_remove_session_handles_.swap(pending_to_remove_session_handles); @@ -809,10 +873,10 @@ bool HttpClient::doRemoveSessions() std::lock_guard session_lock_guard{sessions_m_}; for (auto &removing_handle : pending_to_remove_session_handles) { - auto session = sessions_.find(removing_handle.first); + auto session = sessions_.find(removing_handle.session_id); if (session != sessions_.end()) { - pending_to_remove_sessions.emplace_back(std::move(session->second)); + pending_to_remove_sessions.emplace(session->first, std::move(session->second)); sessions_.erase(session); } } @@ -820,40 +884,35 @@ bool HttpClient::doRemoveSessions() for (auto &removing_handle : pending_to_remove_session_handles) { - auto &resource = removing_handle.second; + auto &resource = removing_handle.resource; if (nullptr == resource.easy_handle) { continue; } - // Remove before cleanup: libcurl forbids freeing an easy handle, or the header list it - // points at, while the multi handle still owns it. A handle never added reports CURLM_OK. - const CURLMcode rc = curl_multi_remove_handle(multi_handle_, resource.easy_handle); - if (CURLM_OK != rc) + // Give the handle back first. libcurl does not copy the header list, so it stays in use + // for as long as the multi handle has a transfer running on this handle. + if (!detachHandle(resource.easy_handle)) { - // The multi handle may still own it. Leaking one easy handle is the better half of - // this trade: freeing it here would corrupt whatever still holds it. - OTEL_INTERNAL_LOG_ERROR( - "[HTTP Client Curl] curl_multi_remove_handle failed, leaving " - "the handle alone: " - << curl_multi_strerror(rc)); - continue; - } + // Still held, so a transfer may still be running on it and its CURLOPT_PRIVATE still + // names this session. Keep the whole record instead of losing track of the handle. + auto owner = pending_to_remove_sessions.find(removing_handle.session_id); + if (owner != pending_to_remove_sessions.end()) + { + removing_handle.owner = owner->second; + } - if (nullptr != resource.headers_chunk) - { - curl_slist_free_all(resource.headers_chunk); - resource.headers_chunk = nullptr; + quarantined_handles_.push_back(std::move(removing_handle)); + continue; } - curl_easy_cleanup(resource.easy_handle); - resource.easy_handle = nullptr; + ReleaseCurlResource(resource); } for (auto &removing_session : pending_to_remove_sessions) { // This operation may add more pending_to_remove_session_handles - removing_session->FinishOperation(); + removing_session.second->FinishOperation(); } should_continue = @@ -890,8 +949,25 @@ bool HttpClient::doRetrySessions(bool report_all) else 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); + + // If the multi handle will not give it back, the add below is answered with + // CURLM_ADDED_ALREADY and the transfer carries on where it was. + detachHandle(easy_handle); + + const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle); + if (CURLM_OK == rc) + { + attached_handles_.insert(easy_handle); + } + else + { + // The session leaves the retry queue either way, so say so rather than let a request + // that was never re-armed look like one that was. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_add_handle failed, this retry will not be sent: " + << curl_multi_strerror(rc)); + } + retry_it = pending_to_retry_sessions_.erase(retry_it); has_data = true; } @@ -938,12 +1014,55 @@ void HttpClient::resetMultiHandle() doRemoveSessions(); - // 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_); + { + // 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_); - // Create a another multi handle to continue pending sessions - multi_handle_ = curl_multi_init(); + // The cleanup detached everything the old handle held, so nothing is attached any more and + // nothing may be offered back to the handle created below. + attached_handles_.clear(); + + // Create a another multi handle to continue pending sessions + multi_handle_ = curl_multi_init(); + } + + releaseQuarantinedHandles(); +} + +bool HttpClient::detachHandle(CURL *easy_handle) +{ + if (attached_handles_.end() == attached_handles_.find(easy_handle)) + { + // The multi handle was never given this one, so it has nothing to give back. Asking libcurl + // is not a way to find that out: 8.10 and 8.11 reject a handle the multi handle does not + // hold, and a multi handle that failed to initialize rejects every handle. + return true; + } + + const CURLMcode rc = curl_multi_remove_handle(multi_handle_, easy_handle); + if (CURLM_OK != rc) + { + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_remove_handle failed, the handle stays " + "with the multi handle: " + << curl_multi_strerror(rc)); + return false; + } + + attached_handles_.erase(easy_handle); + return true; +} + +void HttpClient::releaseQuarantinedHandles() +{ + std::list quarantined_handles; + quarantined_handles.swap(quarantined_handles_); + + for (auto &quarantined : quarantined_handles) + { + ReleaseCurlResource(quarantined.resource); + } } } // namespace curl diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 90142962d6..062b7c88d2 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 @@ -17,12 +17,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -56,6 +58,39 @@ class HttpClientTestPeer { public: static void ResetMultiHandle(HttpClient &client) { client.resetMultiHandle(); } + + static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + + // What the multi handle holds is recorded when curl_multi_add_handle accepts a handle. The + // cases below have no transfer to accept, so they say what it holds directly. + static void NoteAttached(HttpClient &client, CURL *easy_handle) + { + client.attached_handles_.insert(easy_handle); + } + + static std::size_t AttachedCount(const HttpClient &client) + { + return client.attached_handles_.size(); + } + + static std::size_t PendingRemovalCount(const HttpClient &client) + { + return client.pending_to_remove_session_handles_.size(); + } + + static std::size_t QuarantinedCount(const HttpClient &client) + { + return client.quarantined_handles_.size(); + } + + // A multi handle that failed to initialize refuses every removal, which is how the cases below + // reach the path where libcurl will not take a handle back. + static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) + { + CURLM *previous = client.multi_handle_; + client.multi_handle_ = replacement; + return previous; + } }; } // namespace curl } // namespace client @@ -629,6 +664,130 @@ TEST_F(BasicCurlHttpTests, ResetMultiHandleWithASessionDoesNotDeadlock) client->FinishAllSessions(); } +// The record queued for removal is the only thing naming the easy handle, the header list it +// points at and the session, so a removal libcurl refuses has to keep all three. The message +// loop reads that session back out of CURLOPT_PRIVATE, which makes letting go of it as much a +// live pointer left behind as a handle. +TEST_F(BasicCurlHttpTests, ARefusedRemovalKeepsTheHandleAndTheSessionItNames) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + const auto session_id = std::static_pointer_cast(session)->GetSessionId(); + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: keep"); + ASSERT_TRUE(headers_chunk != nullptr); + + http_client::curl::HttpClientTestPeer::NoteAttached(client, easy_handle); + client.ScheduleRemoveSession(session_id, {easy_handle, headers_chunk}); + + // From here the client holds the only reference, so what survives the removal is what the + // removal chose to keep. + std::weak_ptr watch = session; + session.reset(); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); + EXPECT_FALSE(watch.expired()); +} + +// A multi handle that was never given a handle has nothing to give back, and asking it is not a +// way to find that out: libcurl 8.10 and 8.11 refuse, and a multi handle that failed to +// initialize refuses on every version. Reading either refusal as ownership strands the handle. +TEST_F(BasicCurlHttpTests, AHandleTheMultiHandleNeverTookIsFreedNotStranded) +{ + curl::HttpClient client; + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: free"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(4405U, {easy_handle, headers_chunk}); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); +} + +// A handle handed over after the background thread has stopped, or before it ever started, has +// nobody left to drain it, so the client frees it on the way out. Nothing here can watch that +// happen, which leaves this case checking that the record reaches the queue and the sanitizer +// builds checking what becomes of it. +TEST_F(BasicCurlHttpTests, AHandleQueuedWithNoOneLeftToDrainItIsFreedWithTheClient) +{ + curl::HttpClient client; + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: undrained"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(4405U, {easy_handle, headers_chunk}); + + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); +} + +// One record per easy handle. A session that starts another request hands over a second handle +// while the first is still queued, and keying the queue by session would lose the first one. +TEST_F(BasicCurlHttpTests, ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst) +{ + curl::HttpClient client; + + CURL *first = curl_easy_init(); + ASSERT_TRUE(first != nullptr); + CURL *second = curl_easy_init(); + ASSERT_TRUE(second != nullptr); + curl_slist *first_headers = curl_slist_append(nullptr, "X-Test: first"); + ASSERT_TRUE(first_headers != nullptr); + curl_slist *second_headers = curl_slist_append(nullptr, "X-Test: second"); + ASSERT_TRUE(second_headers != nullptr); + + client.ScheduleRemoveSession(4405U, {first, first_headers}); + client.ScheduleAddSession(4405U); + client.ScheduleRemoveSession(4405U, {second, second_headers}); + + EXPECT_EQ(2U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); +} + +// The same ledger after a real request. The background thread is the only one that writes it and +// it only stops once nothing is running, so what a case can look at is the state it leaves +// behind: an entry left there would send a later handle allocated at the same address through a +// removal it never needed, and nothing should still be waiting to be given back. +TEST_F(BasicCurlHttpTests, TheAttachmentLedgerIsEmptyOnceARequestFinishes) +{ + received_requests_.clear(); + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000/get/"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); + + session->SendRequest(handler); + ASSERT_TRUE(waitForRequests(30, 1)); + session->FinishSession(); + client.WaitBackgroundThreadExit(); + + EXPECT_TRUE(handler->got_response_.load(std::memory_order_acquire)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::AttachedCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); +} + // The caller-thread side of the same cancel. The server handler takes mtx_requests before it // answers, so holding it keeps a response from racing the cancel and the abort lands while the // IO thread is still driving the easy handle. That pairing is what #4369 caught. From d72a930c816c06c415064285f65ec6957feb75fb Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 10:01:41 +0000 Subject: [PATCH 05/10] [TEST] Let a cancelled request end either of the two ways it can RepeatedCallerThreadCancelsAreClean summed Cancelled and Response over twenty attempts against a closed port and asked for at least one each. There is a third way such an attempt ends: the connection fails before the cancel arrives, and the cancel is then correctly a no-op. Where the caller and the IO thread share a core that is the usual outcome, and the case fails. Measured with the run pinned to one core, fifteen runs each: 15 of 15 fail on main at 3fb1d317, on #4395 at 66ab56ca, and on this branch either side of the change it carries, all with the same 3 to 7 cancels out of 20. Unpinned all four pass. The failure the valgrind job hit is that, not anything about the handle accounting. The connection failure is counted apart from terminal_count_, which two other cases pin to an exact value, and it is taken before the existing chain rather than inside it, because ACancelBeforeTheResponseReportsCancelled cancels from ConnectFailed and needs that branch to keep firing. The case now asks each attempt to end somewhere the handler is told about rather than to end the same way twenty times, which is what it was after. Pinned to one core it goes from 15 of 15 failing to 0 of 15. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 062b7c88d2..1dbbb800eb 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -143,6 +143,14 @@ class TerminalCountingHandler : public CustomEventHandler void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override { + // Counted on its own, and before the chain below rather than inside it: a case can cancel + // from one of these, and two others pin terminal_count_ to an exact value. + if (state == http_client::SessionState::ConnectFailed || + state == http_client::SessionState::SendFailed) + { + failed_count_.fetch_add(1, std::memory_order_release); + } + if (state == http_client::SessionState::Cancelled) { terminal_count_.fetch_add(1, std::memory_order_release); @@ -168,6 +176,7 @@ class TerminalCountingHandler : public CustomEventHandler http_client::SessionState cancel_at_ = http_client::SessionState::Response; std::thread::id cancelled_from_{}; std::atomic terminal_count_{0}; + std::atomic failed_count_{0}; std::atomic cancelled_from_callback_{0}; }; @@ -822,8 +831,6 @@ TEST_F(BasicCurlHttpTests, ACancelFromTheCallerThreadReportsCancelled) // thread sanitizer this reports against the unfixed client on every run. TEST_F(BasicCurlHttpTests, RepeatedCallerThreadCancelsAreClean) { - int terminal_total = 0; - for (int i = 0; i < 20; ++i) { auto session_manager = std::make_shared()->Create(); @@ -840,12 +847,15 @@ TEST_F(BasicCurlHttpTests, RepeatedCallerThreadCancelsAreClean) session_manager->FinishAllSessions(); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); - terminal_total += handler->terminal_count_.load(std::memory_order_acquire); - } - // A lower bound, not a count: #4360 tracks the same cancel arriving twice, and how many - // arrive is not what this case decides. - EXPECT_GE(terminal_total, 20); + // Whichever side wins, the attempt ends somewhere the handler is told about: the cancel + // lands first, or the connection to a closed port fails first and the cancel is then + // correctly a no-op. How many cancels arrive is not what this case decides, #4360 tracks + // that, and neither is which of the two wins. + EXPECT_GT(handler->terminal_count_.load(std::memory_order_acquire) + + handler->failed_count_.load(std::memory_order_acquire), + 0); + } } TEST_F(BasicCurlHttpTests, SendGetRequestSync) From 965e07215b3ef8dc2744276a4ac0f8db24393ec6 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:34:35 +0000 Subject: [PATCH 06/10] [BUG] Take the session with the record instead of finding it again later A queued easy handle still names its session through CURLOPT_PRIVATE, and the message loop reads that back and dereferences it. Removing a handle from an active transfer also runs the progress callback once, measured on 8.14.1, and that callback dereferences the operation the session owns. So the session has to outlive the record, and the record was relying on finding it again when it was drained, in sessions_ or in a second queue that CleanupSession filled. Neither survives the ordinary ordering. Instrumented over the suite, one or two sessions per run reach CleanupSession with a record of theirs in flight, no record for them visible because the background thread has already swapped the queue out, and the session inactive because its completion callback has run. Neither branch keeps it, so its last reference goes while the handle it names is still being released. The record now takes the session in the same critical section that publishes it. CleanupSession has nothing left to decide about a queued handle, and the queue that existed only to carry sessions found later is gone. The abort path gives its record a session directly, because scheduling an abort takes that session out of sessions_ before the handle is queued, and nothing drains in between: both steps belong to the background thread, one after the other. A handle recorded against a multi handle that gets replaced is not offered to the replacement, because curl_multi_cleanup detaches what it held and the ledger is emptied in the same step. That is what a generation on the record would be for, and there is a case for it now. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 13 +- ext/src/http/client/curl/http_client_curl.cc | 95 +++++++-------- ext/test/http/curl_http_test.cc | 111 ++++++++++++++++++ 3 files changed, 167 insertions(+), 52 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 697f27de43..6837ca147a 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 @@ -379,11 +379,13 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unordered_map> sessions_; std::unordered_set pending_to_add_session_ids_; std::unordered_map> pending_to_abort_sessions_; - // One easy handle on its way back to libcurl. There is one of these per handle rather than - // per session, because a session that starts another request hands over a second handle while - // the first is still queued, and one entry per session would drop the first one, leaving its - // easy handle and header list with no owner. The session is only filled in when the handle - // has to be kept, and names what the handle still points at. + // One easy handle on its way back to libcurl, with the session it still names through + // CURLOPT_PRIVATE. The session is taken when the record is made, so that nothing has to find + // it again later against a sessions_ the calling thread is free to change in between. + // + // There is one of these per handle rather than per session, because a session that starts + // another request hands over a second handle while the first is still queued, and one entry + // per session would drop the first one, leaving its easy handle and header list with no owner. struct PendingCurlRemoval { uint64_t session_id; @@ -392,7 +394,6 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient }; std::list pending_to_remove_session_handles_; - std::unordered_map> pending_to_remove_sessions_; // Which easy handles the multi handle holds, recorded when curl_multi_add_handle accepts one. // What curl_multi_remove_handle returns cannot answer that question afterwards: it reports diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 7bd84b9b1c..307290df8f 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -346,23 +346,29 @@ HttpClient::~HttpClient() } // The background thread has stopped, so nothing else is coming back for what it left behind. - // Sessions go first, because releasing one can hand over one last easy handle, and they are - // released outside the lock because that hand over takes it. + // A batch at a time and with no lock held: a record owns the session it names, and letting one + // go runs a destructor that comes back through ScheduleRemoveSession. That can only queue what + // a session still held, and no more sessions are made here, so this ends. releaseQuarantinedHandles(); + + while (true) { - std::unordered_map> pending_to_remove_sessions; + std::list pending_to_remove_session_handles; { std::lock_guard session_id_lock_guard{session_ids_m_}; - pending_to_remove_sessions_.swap(pending_to_remove_sessions); + pending_to_remove_session_handles_.swap(pending_to_remove_session_handles); } - } - std::lock_guard session_id_lock_guard{session_ids_m_}; - for (auto &pending : pending_to_remove_session_handles_) - { - ReleaseCurlResource(pending.resource); + if (pending_to_remove_session_handles.empty()) + { + break; + } + + for (auto &pending : pending_to_remove_session_handles) + { + ReleaseCurlResource(pending.resource); + } } - pending_to_remove_session_handles_.clear(); } std::shared_ptr HttpClient::CreateSession( @@ -438,28 +444,13 @@ void HttpClient::CleanupSession(uint64_t session_id) std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.erase(session_id); - if (session) + // A handle of this session that is queued for removal already holds the session, so there + // is nothing to decide about one here. + if (session && session->IsSessionActive() && session->GetOperation()) { - bool has_pending_handle = false; - for (const auto &pending : pending_to_remove_session_handles_) - { - if (pending.session_id == session_id) - { - has_pending_handle = true; - break; - } - } - - if (has_pending_handle) - { - pending_to_remove_sessions_.emplace(session_id, std::move(session)); - } - else if (session->IsSessionActive() && session->GetOperation()) - { - // If this session is already running, give it to the background thread for cleanup. - pending_to_abort_sessions_[session_id] = std::move(session); - need_wakeup_background_thread = true; - } + // If this session is already running, give it to the background thread for cleanup. + pending_to_abort_sessions_[session_id] = std::move(session); + need_wakeup_background_thread = true; } } @@ -728,10 +719,23 @@ void HttpClient::ScheduleAbortSession(uint64_t session_id) void HttpClient::ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource &&resource) { { - std::lock_guard lock_guard{session_ids_m_}; + // The session is taken here, in the same breath as the record. Looking it up when the record + // is drained instead leaves a gap the calling thread runs through: it returns from + // FinishSession, CleanupSession takes the session out of sessions_, and by then this queue + // has been swapped away, so neither side would be holding a session the handle still names. + std::lock_guard session_lock_guard{sessions_m_}; + std::lock_guard session_id_lock_guard{session_ids_m_}; + + std::shared_ptr owner; + auto session = sessions_.find(session_id); + if (session != sessions_.end()) + { + owner = session->second; + } + pending_to_add_session_ids_.erase(session_id); pending_to_remove_session_handles_.push_back( - PendingCurlRemoval{session_id, std::move(resource), nullptr}); + PendingCurlRemoval{session_id, std::move(resource), std::move(owner)}); } wakeupBackgroundThread(); @@ -842,13 +846,18 @@ bool HttpClient::doAbortSessions() has_data = true; } - // FinishOperation queued the easy handle for removal, and that handle still holds - // CURLOPT_PRIVATE and the callback data pointing into this session and its operation. - // doRemoveSessions cannot find the session to hold, because aborting took it out of - // sessions_, so hand it over under the id it will look the handle up by. + // Aborting took this session out of sessions_ before FinishOperation queued its easy + // handle, so the record could not take the session for itself and is given it here. Nothing + // drains the queue in between: both run on this thread, one after the other. { std::lock_guard session_id_lock_guard{session_ids_m_}; - pending_to_remove_sessions_.emplace(session.first, std::move(session.second)); + for (auto &pending : pending_to_remove_session_handles_) + { + if (pending.session_id == session.first && !pending.owner) + { + pending.owner = session.second; + } + } } } return has_data; @@ -865,7 +874,6 @@ bool HttpClient::doRemoveSessions() { std::lock_guard session_id_lock_guard{session_ids_m_}; pending_to_remove_session_handles_.swap(pending_to_remove_session_handles); - pending_to_remove_sessions_.swap(pending_to_remove_sessions); } { // If user callback do not call CancelSession or FinishSession, We still need to remove it @@ -895,13 +903,8 @@ bool HttpClient::doRemoveSessions() if (!detachHandle(resource.easy_handle)) { // Still held, so a transfer may still be running on it and its CURLOPT_PRIVATE still - // names this session. Keep the whole record instead of losing track of the handle. - auto owner = pending_to_remove_sessions.find(removing_handle.session_id); - if (owner != pending_to_remove_sessions.end()) - { - removing_handle.owner = owner->second; - } - + // names this session. The record carries that session already, so keeping the record + // keeps both. quarantined_handles_.push_back(std::move(removing_handle)); continue; } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 1dbbb800eb..6095ab9d19 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -61,6 +61,8 @@ class HttpClientTestPeer static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + static bool AbortSessions(HttpClient &client) { return client.doAbortSessions(); } + // What the multi handle holds is recorded when curl_multi_add_handle accepts a handle. The // cases below have no transfer to accept, so they say what it holds directly. static void NoteAttached(HttpClient &client, CURL *easy_handle) @@ -78,6 +80,19 @@ class HttpClientTestPeer return client.pending_to_remove_session_handles_.size(); } + static std::size_t PendingRemovalOwnerCount(const HttpClient &client) + { + std::size_t owned = 0; + for (const auto &pending : client.pending_to_remove_session_handles_) + { + if (pending.owner) + { + ++owned; + } + } + return owned; + } + static std::size_t QuarantinedCount(const HttpClient &client) { return client.quarantined_handles_.size(); @@ -706,6 +721,72 @@ TEST_F(BasicCurlHttpTests, ARefusedRemovalKeepsTheHandleAndTheSessionItNames) EXPECT_FALSE(watch.expired()); } +// A queued handle still names its session through CURLOPT_PRIVATE, and the message loop reads +// that back. The session is taken when the record is made rather than found when the record is +// drained, because between those two the calling thread returns from FinishSession and takes the +// session out of sessions_, and by then the queue has been swapped away. +TEST_F(BasicCurlHttpTests, AQueuedHandleTakesTheSessionItNamesWithIt) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + const auto session_id = std::static_pointer_cast(session)->GetSessionId(); + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: owner"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(session_id, {easy_handle, headers_chunk}); + + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalOwnerCount(client)); + + // What the calling thread does next takes nothing away, because there is nothing left for it + // to decide. + std::weak_ptr watch = session; + session.reset(); + client.CleanupSession(session_id); + + EXPECT_FALSE(watch.expired()); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + EXPECT_TRUE(watch.expired()); +} + +// Scheduling an abort takes the session out of sessions_, so a handle queued after that finds +// nothing to take and the abort hands the session over itself. Both steps are the background +// thread's, one after the other, and no request is sent here so there is no background thread to +// run them out of order. +TEST_F(BasicCurlHttpTests, AnAbortedSessionIsGivenToTheHandleItQueued) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + const auto session_id = std::static_pointer_cast(session)->GetSessionId(); + + client.ScheduleAbortSession(session_id); + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: aborted"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(session_id, {easy_handle, headers_chunk}); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalOwnerCount(client)); + + http_client::curl::HttpClientTestPeer::AbortSessions(client); + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalOwnerCount(client)); + + std::weak_ptr watch = session; + session.reset(); + EXPECT_FALSE(watch.expired()); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + EXPECT_TRUE(watch.expired()); +} + // A multi handle that was never given a handle has nothing to give back, and asking it is not a // way to find that out: libcurl 8.10 and 8.11 refuse, and a multi handle that failed to // initialize refuses on every version. Reading either refusal as ownership strands the handle. @@ -746,6 +827,36 @@ TEST_F(BasicCurlHttpTests, AHandleQueuedWithNoOneLeftToDrainItIsFreedWithTheClie EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); } +// curl_multi_cleanup detaches whatever the old multi handle held, so a handle recorded against +// it is not attached to anything once it has been replaced, and nothing may be handed back to +// the replacement. Emptying the ledger in the same step is what says so. The replacement is made +// to refuse every removal here, so an offer to it would leave the handle kept back instead of +// freed. +TEST_F(BasicCurlHttpTests, AHandleFromTheReplacedMultiHandleIsNotOfferedToItsSuccessor) +{ + curl::HttpClient client; + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: generation"); + ASSERT_TRUE(headers_chunk != nullptr); + + http_client::curl::HttpClientTestPeer::NoteAttached(client, easy_handle); + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::AttachedCount(client)); + + http_client::curl::HttpClientTestPeer::ResetMultiHandle(client); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::AttachedCount(client)); + + client.ScheduleRemoveSession(4405U, {easy_handle, headers_chunk}); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); +} + // One record per easy handle. A session that starts another request hands over a second handle // while the first is still queued, and keying the queue by session would lose the first one. TEST_F(BasicCurlHttpTests, ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst) From 9813478f59c7fea73ab7a61ff075d656a7b1a005 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:42:31 +0000 Subject: [PATCH 07/10] [BUG] Read a curl message and take it on again as one decision The guard added earlier on this branch returns before the message is read, and the caller then asked IsRetryable() anyway. That predicate reads response_code_, last_curl_result_ and retry_attempts_, none of which the guard updates, so for an operation that has been cleaned up it answers with whatever the message before it left behind. A retryable status from that earlier message puts a session back in the retry queue while the easy handle it names is on its way out of the client, and doRetrySessions offers a moved-out handle to the multi handle without reading either return code. PerformCurlMessage now answers whether it has rewound the operation for another attempt, which is the only thing that makes the push correct, and the predicate is read once rather than twice with a Cleanup possible in between. The guard's comment claimed more than the flag carries. is_cleaned_ is exchanged at the top of Cleanup, ahead of the terminal event, the hand over of the easy handle, the completion callback and the promise, so reading it true says only that some thread has entered Cleanup. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../http/client/curl/http_operation_curl.h | 7 ++++- ext/src/http/client/curl/http_client_curl.cc | 6 +++-- .../http/client/curl/http_operation_curl.cc | 13 +++++++--- ext/test/http/curl_http_test.cc | 26 +++++++++++++++++++ 4 files changed, 45 insertions(+), 7 deletions(-) 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..7448b71075 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 @@ -285,7 +285,12 @@ class HttpOperation * * @param code CURLcode */ - void PerformCurlMessage(CURLcode code); + /** + * Read one completed curl message. + * @return true when the operation has been rewound for another attempt, and the caller is to + * take it on. False when the message was not this operation's to read, or it is finished. + */ + bool PerformCurlMessage(CURLcode code); inline CURL *GetCurlEasyHandle() noexcept { return curl_resource_.easy_handle; } diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 307290df8f..92bda6a89c 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -568,9 +568,11 @@ bool HttpClient::MaybeSpawnBackgroundThread() { // Session can not be destroyed when calling PerformCurlMessage auto hold_session = session->shared_from_this(); - operation->PerformCurlMessage(result); - if (operation->IsRetryable()) + // Reading the message and taking it on for another attempt are one decision. + // Asking again afterwards reads whatever the last message left behind, which for + // an operation that has since been cleaned up is a status from before that. + if (operation->PerformCurlMessage(result)) { self->pending_to_retry_sessions_.push_back(hold_session); } diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 0384d3370c..8e2614c5f7 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1519,13 +1519,15 @@ void HttpOperation::Abort() } } -void HttpOperation::PerformCurlMessage(CURLcode code) +bool HttpOperation::PerformCurlMessage(CURLcode code) { if (is_cleaned_.load(std::memory_order_acquire)) { - // Already torn down and queued for removal. A message buffered before that would dispatch a - // second round of terminal events and requeue a handle waiting to be freed. - return; + // Some thread has entered Cleanup for this operation, which is as much as the flag says: it + // is taken before the terminal event, the hand over of the easy handle and the completion + // callback. Either way this message is not this operation's to read, and it is not to be + // taken on for another attempt on the strength of what an earlier message left behind. + return false; } ++retry_attempts_; @@ -1632,7 +1634,10 @@ void HttpOperation::PerformCurlMessage(CURLcode code) { // Cleanup and unbind easy handle from multi handle, and finish callback Cleanup(); + return false; } + + return true; } } // namespace curl diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 6095ab9d19..65a1d5c32f 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -520,6 +520,32 @@ TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) ASSERT_TRUE(operation.IsRetryable()); } +// Reading a message and taking the operation on for another attempt are one decision. The status +// an operation reports does not change when it is cleaned up, so a caller that asks about it +// separately would put a session whose easy handle is on its way out back in the retry queue. +TEST_F(BasicCurlHttpTests, ACleanedOperationDoesNotAskToBeRetried) +{ + RetryEventHandler handler; + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy = {5, std::chrono::duration{1.0f}, + std::chrono::duration{5.0f}, 1.5f}; + + curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, + &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_TRUE(operation.IsRetryable()); + + operation.Cleanup(); + + EXPECT_TRUE(operation.IsRetryable()); + EXPECT_FALSE(operation.PerformCurlMessage(CURLE_OK)); +} + TEST_F(BasicCurlHttpTests, RetryPolicyDisabled) { RetryEventHandler handler; From 18bb79db90be2884a7a7467cfede733d43ce5ac1 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:56:22 +0000 Subject: [PATCH 08/10] [BUG] Clear a detached easy handle before freeing it curl_easy_cleanup documents that it can reach the application: "Occasionally you may get your progress callback or header callback called from within curl_easy_cleanup (if previously set for the handle using curl_easy_setopt)", for protocols that need a command and response before disconnecting. By then the event handler is gone. The only shared_ptr to it is captured in the completion callback that SendRequest builds, Cleanup swaps that out and runs it, and it is released when Cleanup returns. The write, header and read callbacks all dispatch through the raw pointer the operation keeps, so the one libcurl says it may call is the one with nothing behind it. Clearing the handle first takes the callbacks and their data off it, so freeing it cannot reach anything the client has let go of, and the header list is freed after that because libcurl does not copy it and the clear is what stops it being read. This runs on the background thread with the handle detached, which is what the version removed from Cleanup earlier on this branch could not say: that one ran on whichever thread cancelled, against a handle the multi handle still held. Measured on 8.14.1 over HTTP, neither curl_multi_remove_handle nor curl_easy_cleanup reaches a callback once the transfer has finished, and removing a handle from a live transfer reaches the progress callback once. So nothing here is what stops a failure today over HTTP. What it stops is the call libcurl reserves the right to make. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 17 +++++++++++++---- 1 file changed, 13 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 92bda6a89c..cd27b6f61c 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -49,14 +49,17 @@ namespace curl namespace { -// The easy handle goes first. libcurl does not copy the header list, so it is in use for as long -// as anything is still transferring on the handle that points at it. +// Only ever called with the handle detached, either because it has been given back or because +// the multi handle that held it has been cleaned up. +// +// The reset clears the callbacks and the pointers they are given, so that freeing the handle +// cannot reach the operation or the event handler the caller has let go of by then. The header +// list goes next, because libcurl does not copy it and the reset is what stops it being read. void ReleaseCurlResource(HttpCurlEasyResource &resource) noexcept { if (nullptr != resource.easy_handle) { - curl_easy_cleanup(resource.easy_handle); - resource.easy_handle = nullptr; + curl_easy_reset(resource.easy_handle); } if (nullptr != resource.headers_chunk) @@ -64,6 +67,12 @@ void ReleaseCurlResource(HttpCurlEasyResource &resource) noexcept curl_slist_free_all(resource.headers_chunk); resource.headers_chunk = nullptr; } + + if (nullptr != resource.easy_handle) + { + curl_easy_cleanup(resource.easy_handle); + resource.easy_handle = nullptr; + } } } // namespace From 4a894896241e20c49aefea141c90fe48f8857ae3 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 26 Aug 2026 21:36:55 +0000 Subject: [PATCH 09/10] [TEST] Cover the CA certificate passed as a blob A CA certificate given as a string goes to curl through CURLOPT_CAINFO_BLOB rather than as a path, and nothing reached that branch: gcovr reported it at zero hits over the whole suite. curl copies the blob and does not read it until a handshake, so a plain request reaches the option and the certificate does not have to be valid. The case sets use_ssl, without which the whole SSL block is skipped, and leaves ssl_ca_cert_path empty, which is what selects the string branch. It sits outside ENABLE_OTLP_RETRY_PREVIEW, since a case that compiles out is still registered by gtest_add_tests and a filter matching nothing exits zero. Measured after: the branch reports one hit, 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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 65a1d5c32f..ca1338a3ae 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -501,6 +501,26 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations) delete handler; } +// A CA certificate supplied as a string goes to curl as a blob rather than a path. curl copies it +// and does not read it until a handshake, so a plain request reaches the option and no certificate +// has to be valid for this. +TEST_F(BasicCurlHttpTests, ACaCertificateStringIsPassedAsABlob) +{ + RetryEventHandler handler; + http_client::HttpSslOptions ssl_options; + ssl_options.use_ssl = true; + ssl_options.ssl_ca_cert_string = + "-----BEGIN CERTIFICATE-----\nnot a certificate\n-----END CERTIFICATE-----\n"; + 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()); +} + #ifdef ENABLE_OTLP_RETRY_PREVIEW TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) { From 79744430b702947bdff076d2f39d3aab530699f1 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:10:08 +0000 Subject: [PATCH 10/10] [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 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index ca1338a3ae..545fb0b4c1 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -514,8 +514,14 @@ TEST_F(BasicCurlHttpTests, ACaCertificateStringIsPassedAsABlob) 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());