From 39f6efb37cc30a15191c09627dbda3ca89cb8bf7 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 14:21:40 +0000 Subject: [PATCH 01/23] [BUG] Complete a curl operation that is never scheduled An operation could be given a promise and then never handed to the IO thread, which left the caller waiting on a future nobody was in a position to complete. FinishSession() blocked forever. Three ways in. SendAsync dispatched Connecting before it reset the operation flags and published the cancel route, so a handler that cancelled from that event had its cancel overwritten and the session removed from the client behind it. CreateSession hands back an unregistered session when the URL does not parse, and CURLOPT_URL is not checked when it is set, so Setup succeeds and the same dead end is reached with no handler involved. And doAddSessions dropped the result of curl_multi_add_handle, so a rejected handle left an operation nobody would ever run. The flags and the cancel route are published before the event now, and the future is published after it, so a handler calling FinishSession() from the event still returns rather than waiting on a transfer that has not been scheduled. ScheduleAddSession reports whether the session was still registered, and SendAsync finishes the operation itself when it was not, or when the event cancelled it. doAddSessions finishes the ones the multi handle rejects, and does it outside sessions_m_, since FinishOperation reaches the handler. The completion callback is published with them rather than after the event. Cleanup() takes is_cleaned_ at its start and swaps async_data_->callback about forty lines later, so the is_cleaned_ recheck only says cleanup has begun, not that it has finished reading the callback. A handler cancelling from Connecting wakes the IO thread, which can reach the swap first, find an empty callback, and never run the completion, which is also what clears is_session_active_. The two accesses to the std::function race as well. Found by @lalitb in review. The conditional include of global_log_handler.h goes too. The file already pulled it in under the else branch of ENABLE_OTLP_COMPRESSION_PREVIEW, and this change needs it unguarded, so with compression preview off both were live and include-what-you-use reported one too many. Fixes #4390. Fixes #4393. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 + .../ext/http/client/curl/http_client_curl.h | 2 +- ext/src/http/client/curl/http_client_curl.cc | 69 ++++++++++++++----- .../http/client/curl/http_operation_curl.cc | 39 +++++++++-- ext/test/http/curl_http_test.cc | 68 ++++++++++++++++++ 5 files changed, 154 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 821e2fb2b2..52e1017493 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -110,6 +110,9 @@ Increment the: * [BUG] Cancel a curl session without writing to the easy handle from the cancelling thread ([#4392](https://github.com/open-telemetry/opentelemetry-cpp/pull/4392)) +* [BUG] Finish a curl operation that never gets scheduled, instead of leaving + FinishSession blocked forever + ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) * [CODE HEALTH] Enable clang-tidy `modernize-deprecated-headers` and replace deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents 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..b0465b6dfc 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 @@ -348,7 +348,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient // return true if create background thread, false is already exist background thread bool MaybeSpawnBackgroundThread(); - void ScheduleAddSession(uint64_t session_id); + bool ScheduleAddSession(uint64_t session_id); void ScheduleAbortSession(uint64_t session_id); void ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource &&resource); diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 87f2c123ad..77bc955193 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 @@ -638,9 +638,19 @@ bool HttpClient::MaybeSpawnBackgroundThread() return true; } -void HttpClient::ScheduleAddSession(uint64_t session_id) +bool HttpClient::ScheduleAddSession(uint64_t session_id) { { + std::lock_guard sessions_lock{sessions_m_}; + if (sessions_.end() == sessions_.find(session_id)) + { + // Either the URL never parsed, so CreateSession handed the caller a session it did not + // register, or a handler took this one out while the first events were dispatched. + // Whatever removed it owns the teardown; adding it back would run an operation nobody + // is waiting on and leave the caller waiting on one nobody runs. + return false; + } + std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.insert(session_id); pending_to_remove_session_handles_.erase(session_id); @@ -648,6 +658,7 @@ void HttpClient::ScheduleAddSession(uint64_t session_id) } wakeupBackgroundThread(); + return true; } void HttpClient::ScheduleAbortSession(uint64_t session_id) @@ -728,29 +739,49 @@ bool HttpClient::doAddSessions() } bool has_data = false; + std::list> rejected_by_multi; - std::lock_guard lock_guard{sessions_m_}; - for (auto &session_id : pending_to_add_session_ids) { - auto session = sessions_.find(session_id); - if (session == sessions_.end()) + std::lock_guard lock_guard{sessions_m_}; + for (auto &session_id : pending_to_add_session_ids) { - continue; - } + auto session = sessions_.find(session_id); + if (session == sessions_.end()) + { + continue; + } - if (!session->second->GetOperation()) - { - continue; - } + if (!session->second->GetOperation()) + { + continue; + } - CURL *easy_handle = session->second->GetOperation()->GetCurlEasyHandle(); - if (nullptr == easy_handle) - { - continue; + CURL *easy_handle = session->second->GetOperation()->GetCurlEasyHandle(); + if (nullptr == easy_handle) + { + continue; + } + + const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle); + if (CURLM_OK != rc) + { + // Nothing will drive this transfer. Leaving it here is what makes a caller wait on a + // future nobody can complete, so hand it to the loop below to be finished. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_add_handle failed: " << curl_multi_strerror(rc)); + rejected_by_multi.push_back(session->second); + continue; + } + + has_data = true; } + } - curl_multi_add_handle(multi_handle_, easy_handle); - has_data = true; + // Outside sessions_m_ on purpose. FinishOperation runs the caller's handler, and a handler + // that cancels from it takes that lock again. See #4389. + for (auto &session : rejected_by_multi) + { + session->FinishOperation(); } return has_data; diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 0f1bda4035..f876fd4e7f 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1452,20 +1452,45 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionsession.store(session, std::memory_order_release); - if (false == async_data_->is_promise_running.exchange(true, std::memory_order_acq_rel)) + async_data_->callback = std::move(callback); + + DispatchEvent(opentelemetry::ext::http::client::SessionState::Connecting); + + // The future is the one thing that stays unpublished until after the event, so a handler that + // calls FinishSession() from it returns instead of waiting on a transfer that has not been + // scheduled yet. + async_data_->result_promise = std::promise(); + async_data_->result_future = async_data_->result_promise.get_future(); + async_data_->is_promise_running.store(true, std::memory_order_release); + + // Cancelling from the event hands the session to the IO thread, which may already have torn + // the operation down and found no future to complete. Settle it here if so. + if (is_cleaned_.load(std::memory_order_acquire)) { - async_data_->result_promise = std::promise(); - async_data_->result_future = async_data_->result_promise.get_future(); + if (async_data_->is_promise_running.exchange(false, std::memory_order_acq_rel)) + { + async_data_->result_promise.set_value(last_curl_result_); + } + return CURLE_OK; + } + + if (WasAborted() || !session->GetHttpClient().ScheduleAddSession(session->GetSessionId())) + { + // Nothing is going to run this operation. It was cancelled while the event was dispatched, + // or its session was never registered because the URL did not parse. Finish it here rather + // than leave a future nobody can complete. + Cleanup(); } - async_data_->callback = std::move(callback); - session->GetHttpClient().ScheduleAddSession(session->GetSessionId()); return CURLE_OK; } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 90142962d6..949739aa04 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -689,6 +689,74 @@ TEST_F(BasicCurlHttpTests, RepeatedCallerThreadCancelsAreClean) EXPECT_GE(terminal_total, 20); } +// A handler is allowed to cancel from the events SendRequest dispatches, so the flags and the +// cancel route have to be published before the dispatch. Publish them after it and the cancel +// is thrown away, the request is neither sent nor completed, and FinishSession() waits on a +// promise nobody can fulfil. See #4390. +TEST_F(BasicCurlHttpTests, CancelFromCreatedCompletes) +{ + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + handler->cancel_target_ = session.get(); + handler->cancel_at_ = http_client::SessionState::Created; + + session->SendRequest(handler); + session->FinishSession(); + session_manager->FinishAllSessions(); + + EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); + EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); +} + +TEST_F(BasicCurlHttpTests, CancelFromConnectingCompletes) +{ + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + handler->cancel_target_ = session.get(); + handler->cancel_at_ = http_client::SessionState::Connecting; + + session->SendRequest(handler); + session->FinishSession(); + session_manager->FinishAllSessions(); + + EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); + EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); +} + +// CreateSession hands back an unregistered session when the URL does not parse, and +// CURLOPT_URL is not checked when it is set, so nothing on this path stops the operation +// reaching the same dead end with no handler involved at all. See #4393. +TEST_F(BasicCurlHttpTests, InvalidUrlCompletes) +{ + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:not-a-port"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + + session->SendRequest(handler); + session->FinishSession(); + session_manager->FinishAllSessions(); + + EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); + EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear(); From 55ff5231065c0659348b574111e3eee0027994b1 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 11 Aug 2026 15:58:35 +0000 Subject: [PATCH 02/23] [BUG] Publish the curl session state before the event that reports it DispatchEvent stored session_state_ after the handler had returned. A handler that cancels from the first event of a client that is already polling lets the background thread finish the operation and dispatch a state of its own, so both threads wrote that member, and the store left until after the handler returned overwrote the cancel with a state the operation had left. Found by @lalitb in review. The store moves ahead of the handler and the member becomes atomic. SessionState is a uint8_t enum and std::atomic of it is one byte with alignment one, so the layout of an installed type does not move. The new case warms the client with a completed request first, because the background thread is only spawned after SendAsync returns and without one there is nothing for the event to overlap. It is the only case in this file that reaches the is_cleaned_ recheck at the end of SendAsync. Its counters are relaxed deliberately, since an acquire or a release on them gives the two threads an ordering the code under test does not have and the racing store stops being reported. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 + .../http/client/curl/http_operation_curl.h | 7 +- .../http/client/curl/http_operation_curl.cc | 7 +- ext/test/http/curl_http_test.cc | 79 +++++++++++++++++++ 4 files changed, 92 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52e1017493..3607ebe7eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,9 @@ Increment the: * [BUG] Finish a curl operation that never gets scheduled, instead of leaving FinishSession blocked forever ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) +* [BUG] Store the curl session state before the event that reports it, so a + cancel dispatched by the IO thread is not overwritten or raced + ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) * [CODE HEALTH] Enable clang-tidy `modernize-deprecated-headers` and replace deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents 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..ef533cb93d 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 @@ -13,6 +13,7 @@ # include #endif +#include #include #include #include @@ -246,7 +247,7 @@ class HttpOperation */ opentelemetry::ext::http::client::SessionState GetSessionState() const noexcept { - return session_state_; + return session_state_.load(std::memory_order_acquire); } /** @@ -338,7 +339,9 @@ class HttpOperation const Headers &request_headers_; const opentelemetry::ext::http::client::Body &request_body_; size_t request_nwrite_{0}; - opentelemetry::ext::http::client::SessionState session_state_{ + // Written by whichever thread dispatches an event. A handler that cancels from one event + // lets the background thread dispatch another, so the two dispatches can overlap. + std::atomic session_state_{ opentelemetry::ext::http::client::SessionState::Created}; const opentelemetry::ext::http::client::Compression &compression_; diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index f876fd4e7f..bf7fb8b2fa 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -404,12 +404,15 @@ int HttpOperation::OnProgressCallback(void *clientp, void HttpOperation::DispatchEvent(opentelemetry::ext::http::client::SessionState type, const std::string &reason) { + // Published before the handler runs. A handler can cancel from here, which lets the background + // thread finish the operation and publish a state of its own, and a store left until after the + // handler returned would overwrite that one. + session_state_.store(type, std::memory_order_release); + if (event_handle_ != nullptr) { event_handle_->OnEvent(type, reason); } - - session_state_ = type; } HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 949739aa04..df8979bd6f 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -757,6 +757,85 @@ TEST_F(BasicCurlHttpTests, InvalidUrlCompletes) EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); } +// The counters say whether the IO thread reached this handler while the caller was still +// inside an event of its own. They are relaxed on purpose. An acquire or a release on them +// would give the two threads an ordering the code under test does not have, and a state store +// racing another would stop being reported. +class OverlappingCancelHandler : public TerminalCountingHandler +{ +public: + void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override + { + const int depth = inside_events_.fetch_add(1, std::memory_order_relaxed) + 1; + int highest = max_concurrent_events_.load(std::memory_order_relaxed); + while (depth > highest && + !max_concurrent_events_.compare_exchange_weak(highest, depth, std::memory_order_relaxed, + std::memory_order_relaxed)) + { + } + + TerminalCountingHandler::OnEvent(state, reason); + + if (state == cancel_at_) + { + // Cancelling wakes the IO thread, which finishes the operation and dispatches a Cancelled + // of its own. Stay in this event until that one arrives so the two really do overlap, and + // give up rather than hang if it never does. + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); + while (inside_events_.load(std::memory_order_relaxed) < 2 && + std::chrono::steady_clock::now() < deadline) + { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + } + + inside_events_.fetch_sub(1, std::memory_order_relaxed); + } + + std::atomic inside_events_{0}; + std::atomic max_concurrent_events_{0}; +}; + +// A client spawns its IO thread only after a request has been scheduled, so cancelling from +// the first event of the first request has nothing to overlap. On a client that is already +// polling, the IO thread finishes the operation while the handler is still inside that event, +// and the caller reaches the end of SendAsync with the operation already cleaned up. +TEST_F(BasicCurlHttpTests, CancelFromConnectingWhilePollingCompletes) +{ + received_requests_.clear(); + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + { + auto warm = session_manager->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_TRUE(warm_handler->got_response_.load(std::memory_order_acquire)); + } + + // Nothing listens on 19937, so this operation cannot answer on its own. + auto session = session_manager->CreateSession("http://127.0.0.1:19937"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + handler->cancel_target_ = session.get(); + handler->cancel_at_ = http_client::SessionState::Connecting; + + session->SendRequest(handler); + session->FinishSession(); + session_manager->FinishAllSessions(); + + EXPECT_EQ(2, handler->max_concurrent_events_.load(std::memory_order_acquire)); + EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); + EXPECT_EQ(1, handler->cancelled_from_callback_.load(std::memory_order_acquire)); + EXPECT_FALSE(session->IsSessionActive()); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear(); From f72fca0d574ea7c5daf09eeadd8d08f22c9d92e0 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:06:10 +0000 Subject: [PATCH 03/23] [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> --- .../http/client/curl/http_operation_curl.h | 3 +-- ext/src/http/client/curl/http_client_curl.cc | 6 ++--- .../http/client/curl/http_operation_curl.cc | 26 +++++++------------ 3 files changed, 13 insertions(+), 22 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 ef533cb93d..3205b6bcbc 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 @@ -339,8 +339,7 @@ class HttpOperation const Headers &request_headers_; const opentelemetry::ext::http::client::Body &request_body_; size_t request_nwrite_{0}; - // Written by whichever thread dispatches an event. A handler that cancels from one event - // lets the background thread dispatch another, so the two dispatches can overlap. + // Atomic because a handler that cancels from one event can overlap the next dispatch. std::atomic session_state_{ opentelemetry::ext::http::client::SessionState::Created}; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 77bc955193..9750724f82 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -644,10 +644,8 @@ bool HttpClient::ScheduleAddSession(uint64_t session_id) std::lock_guard sessions_lock{sessions_m_}; if (sessions_.end() == sessions_.find(session_id)) { - // Either the URL never parsed, so CreateSession handed the caller a session it did not - // register, or a handler took this one out while the first events were dispatched. - // Whatever removed it owns the teardown; adding it back would run an operation nobody - // is waiting on and leave the caller waiting on one nobody runs. + // Whatever removed the session owns its teardown. Adding it back would run an operation + // nobody waits on, and leave the caller waiting on one nobody runs. return false; } diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index bf7fb8b2fa..53c957908c 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -404,9 +404,8 @@ int HttpOperation::OnProgressCallback(void *clientp, void HttpOperation::DispatchEvent(opentelemetry::ext::http::client::SessionState type, const std::string &reason) { - // Published before the handler runs. A handler can cancel from here, which lets the background - // thread finish the operation and publish a state of its own, and a store left until after the - // handler returned would overwrite that one. + // Store before dispatching: a handler may cancel, and a later store would overwrite the state + // the background thread publishes. session_state_.store(type, std::memory_order_release); if (event_handle_ != nullptr) @@ -1455,11 +1454,9 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionresult_promise = std::promise(); async_data_->result_future = async_data_->result_promise.get_future(); async_data_->is_promise_running.store(true, std::memory_order_release); - // Cancelling from the event hands the session to the IO thread, which may already have torn - // the operation down and found no future to complete. Settle it here if so. + // A cancel from the event may have torn the operation down before the future existed. if (is_cleaned_.load(std::memory_order_acquire)) { if (async_data_->is_promise_running.exchange(false, std::memory_order_acq_rel)) @@ -1488,9 +1483,8 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionGetHttpClient().ScheduleAddSession(session->GetSessionId())) { - // Nothing is going to run this operation. It was cancelled while the event was dispatched, - // or its session was never registered because the URL did not parse. Finish it here rather - // than leave a future nobody can complete. + // Nothing will run this operation, so finish it here rather than leave an unfulfillable + // future. Cleanup(); } From 58cc4c1cc3d7a43ee626391b9afe73ce071b9983 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:36:59 +0000 Subject: [PATCH 04/23] [BUG] Give every session an id, and report what actually failed Four things from review. A session whose URL never parsed kept id 0. Pending removals are keyed by session id, and HttpCurlEasyResource's move assignment is a swap whose destructor frees nothing, so a second unparsable URL displaced the first handle into a temporary that nothing freed. Measured with LeakSanitizer: 5556 bytes in 5 allocations, an easy handle and four strings, 3 of 3. With an id, 0 bytes, 3 of 3. doAddSessions answered false when every add failed, although finishing the rejected sessions had just queued their removals. The loop's idle check runs doRemoveSessions before doAddSessions, so it could exit with that work still queued. It now reports the follow-up work. The failure was logged while sessions_m_ was held. The log handler is application code that can re-enter the client, so the rejection is collected under the lock and reported outside it, next to the finish that was already there for the same reason. A scheduling failure reached Cleanup as Cancelled, which is documented as a manual cancel, carrying a reason read from a curl result that is still CURLE_OK. It is now reported as a create failure that says what happened. The terminal assertions are exact rather than at least one. That matters: measured, cancelling once the transfer belongs to the IO thread produces two terminal events, which is #4360, and the old assertion absorbed it. The count is pinned so a change to it is visible. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 27 ++++++--- .../http/client/curl/http_operation_curl.cc | 12 +++- ext/test/http/curl_http_test.cc | 59 +++++++++++++++++-- 3 files changed, 83 insertions(+), 15 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 9750724f82..0fd09400a0 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -327,7 +327,12 @@ std::shared_ptr HttpClient::CreateSes const auto parsedUrl = common::UrlParser(std::string(url)); if (!parsedUrl.success_) { - return std::make_shared(*this); + // Unregistered, but given an id all the same. Pending removals are keyed by session id, so + // two sessions sharing one displace each other's easy handle and header list, which nothing + // then frees. + auto unregistered = std::make_shared(*this); + unregistered->SetId(++next_session_id_); + return unregistered; } auto session = std::make_shared(*this, parsedUrl.scheme_, parsedUrl.host_, parsedUrl.port_); @@ -737,7 +742,7 @@ bool HttpClient::doAddSessions() } bool has_data = false; - std::list> rejected_by_multi; + std::list, CURLMcode>> rejected_by_multi; { std::lock_guard lock_guard{sessions_m_}; @@ -764,10 +769,9 @@ bool HttpClient::doAddSessions() if (CURLM_OK != rc) { // Nothing will drive this transfer. Leaving it here is what makes a caller wait on a - // future nobody can complete, so hand it to the loop below to be finished. - OTEL_INTERNAL_LOG_ERROR( - "[HTTP Client Curl] curl_multi_add_handle failed: " << curl_multi_strerror(rc)); - rejected_by_multi.push_back(session->second); + // future nobody can complete, so hand it to the loop below to be finished. Reported + // there too: the log handler is application code that can re-enter this client. + rejected_by_multi.emplace_back(session->second, rc); continue; } @@ -777,12 +781,17 @@ bool HttpClient::doAddSessions() // Outside sessions_m_ on purpose. FinishOperation runs the caller's handler, and a handler // that cancels from it takes that lock again. See #4389. - for (auto &session : rejected_by_multi) + for (auto &rejected : rejected_by_multi) { - session->FinishOperation(); + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_add_handle failed: " + << curl_multi_strerror(rejected.second)); + rejected.first->FinishOperation(); } - return has_data; + // Finishing a rejected session queues its removal, and the loop's idle check has already run + // doRemoveSessions by the time it calls this. Answering false here lets the worker exit with + // that removal still queued. + return has_data || !rejected_by_multi.empty(); } bool HttpClient::doAbortSessions() diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 53c957908c..49d92ff48b 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1481,10 +1481,18 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionGetHttpClient().ScheduleAddSession(session->GetSessionId())) + if (WasAborted()) { // Nothing will run this operation, so finish it here rather than leave an unfulfillable - // future. + // future. Cleanup() reports the cancel, which is what happened. + Cleanup(); + } + else if (!session->GetHttpClient().ScheduleAddSession(session->GetSessionId())) + { + // The same, except nobody cancelled anything. Name the failure first: Cleanup() would report + // a manual cancel, with a reason read from a curl result that is still CURLE_OK. + DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, + "the session is not registered with this client"); Cleanup(); } diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index df8979bd6f..b5da8edc98 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -108,7 +108,13 @@ class TerminalCountingHandler : public CustomEventHandler void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override { - if (state == http_client::SessionState::Cancelled) + if (state == http_client::SessionState::CreateFailed) + { + terminal_count_.fetch_add(1, std::memory_order_release); + create_failed_.fetch_add(1, std::memory_order_release); + last_reason_empty_.store(reason.empty(), std::memory_order_release); + } + else if (state == http_client::SessionState::Cancelled) { terminal_count_.fetch_add(1, std::memory_order_release); // Cleanup dispatches its own Cancelled carrying a curl message, and GetCurlErrorMessage @@ -134,6 +140,8 @@ class TerminalCountingHandler : public CustomEventHandler std::thread::id cancelled_from_{}; std::atomic terminal_count_{0}; std::atomic cancelled_from_callback_{0}; + std::atomic create_failed_{0}; + std::atomic last_reason_empty_{true}; }; class GetEventHandler : public CustomEventHandler @@ -711,7 +719,7 @@ TEST_F(BasicCurlHttpTests, CancelFromCreatedCompletes) session_manager->FinishAllSessions(); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); - EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); + EXPECT_EQ(1, handler->terminal_count_.load(std::memory_order_acquire)); } TEST_F(BasicCurlHttpTests, CancelFromConnectingCompletes) @@ -732,12 +740,49 @@ TEST_F(BasicCurlHttpTests, CancelFromConnectingCompletes) session_manager->FinishAllSessions(); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); - EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); + // Two, and one is the goal rather than the behaviour: cancelling once the transfer has been + // handed to the IO thread produces a terminal event from Cleanup and another from the + // completion callback. That is #4360, and pinning the number here makes any change to it + // visible instead of letting an at-least-one assertion absorb it. + EXPECT_EQ(2, handler->terminal_count_.load(std::memory_order_acquire)); } // CreateSession hands back an unregistered session when the URL does not parse, and // CURLOPT_URL is not checked when it is set, so nothing on this path stops the operation // reaching the same dead end with no handler involved at all. See #4393. +// Pending removals are keyed by session id. Two sessions whose URL never parsed must not share +// one: the map's move assignment swaps the displaced easy handle and header list into a +// temporary whose destructor frees neither. +TEST_F(BasicCurlHttpTests, UnparsableUrlsReleaseTheirOwnResources) +{ + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto first = session_manager->CreateSession("http://127.0.0.1:not-a-port"); + auto second = session_manager->CreateSession("http://127.0.0.1:also-not-a-port"); + + const auto first_id = static_cast(first.get())->GetSessionId(); + const auto second_id = static_cast(second.get())->GetSessionId(); + EXPECT_NE(0U, first_id) << "an unregistered session kept the default id"; + EXPECT_NE(first_id, second_id) << "two unregistered sessions share a pending removal key"; + + auto first_request = first->CreateRequest(); + first_request->SetUri("get/"); + auto second_request = second->CreateRequest(); + second_request->SetUri("get/"); + + auto first_handler = std::make_shared(); + auto second_handler = std::make_shared(); + first->SendRequest(first_handler); + second->SendRequest(second_handler); + first->FinishSession(); + second->FinishSession(); + session_manager->FinishAllSessions(); + + EXPECT_EQ(1, first_handler->terminal_count_.load(std::memory_order_acquire)); + EXPECT_EQ(1, second_handler->terminal_count_.load(std::memory_order_acquire)); +} + TEST_F(BasicCurlHttpTests, InvalidUrlCompletes) { auto session_manager = std::make_shared()->Create(); @@ -754,7 +799,13 @@ TEST_F(BasicCurlHttpTests, InvalidUrlCompletes) session_manager->FinishAllSessions(); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); - EXPECT_GE(handler->terminal_count_.load(std::memory_order_acquire), 1); + // Exact, so a duplicate notification or a failure dressed as a manual cancel fails here + // rather than passing as at least one of something. + EXPECT_EQ(1, handler->terminal_count_.load(std::memory_order_acquire)); + EXPECT_EQ(1, handler->create_failed_.load(std::memory_order_acquire)) + << "an unregistered session was not reported as a failed create"; + EXPECT_FALSE(handler->last_reason_empty_.load(std::memory_order_acquire)) + << "the failure was reported without saying what failed"; } // The counters say whether the IO thread reached this handler while the caller was still From af89c4c683f70db3d5f060c2a0f5f9e2404bfd9a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 00:25:01 +0000 Subject: [PATCH 05/23] [BUG] Finish an unregistered operation before telling its handler The branch added for a session the client never registered reported the failure and then finished the operation. The handler is replaceable, and one that calls FinishSession() from that event waits on a promise that only the Cleanup() below the dispatch can fulfil, on the same stack. Measured: the case hung for the whole 60 second bound, twice, exit 124. With the order reversed it returns in under a second, 3 of 3. The terminal state goes in before Cleanup() so it still reports one event rather than a manual cancel, and the report now happens after the operation is done, which is the order the exporters already follow: retire, then call anything replaceable. The case stays, because the failure mode is a hang. Nothing else in the file would notice the order being put back. 32 tests pass, 3 of 3. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../http/client/curl/http_operation_curl.cc | 11 ++-- ext/test/http/curl_http_test.cc | 50 +++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 49d92ff48b..8a253475b8 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1489,11 +1489,16 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionGetHttpClient().ScheduleAddSession(session->GetSessionId())) { - // The same, except nobody cancelled anything. Name the failure first: Cleanup() would report - // a manual cancel, with a reason read from a curl result that is still CURLE_OK. + // The same, except nobody cancelled anything. The terminal state goes in first so Cleanup() + // does not report a manual cancel, with a reason read from a curl result that is still + // CURLE_OK, and the operation is finished before the handler hears about it: a handler that + // calls FinishSession() from this event would otherwise wait on the promise that the + // Cleanup() below it is the only thing able to fulfil. + session_state_.store(opentelemetry::ext::http::client::SessionState::CreateFailed, + std::memory_order_release); + Cleanup(); DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, "the session is not registered with this client"); - Cleanup(); } return CURLE_OK; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index b5da8edc98..31edbf823d 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -783,6 +783,56 @@ TEST_F(BasicCurlHttpTests, UnparsableUrlsReleaseTheirOwnResources) EXPECT_EQ(1, second_handler->terminal_count_.load(std::memory_order_acquire)); } +namespace +{ +// The event is dispatched after the operation is finished, so a handler is free to call +// FinishSession() from it. Reporting first instead leaves this waiting on a promise that only the +// Cleanup() below the dispatch can fulfil, which hangs rather than fails, so this case has to run. +class FinishFromEventHandler : public TerminalCountingHandler +{ +public: + void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override + { + TerminalCountingHandler::OnEvent(state, reason); + if (state == http_client::SessionState::CreateFailed && finish_target_ != nullptr) + { + auto *target = finish_target_; + finish_target_ = nullptr; + entered_.store(true, std::memory_order_release); + target->FinishSession(); + returned_.store(true, std::memory_order_release); + } + } + + http_client::Session *finish_target_ = nullptr; + std::atomic entered_{false}; + std::atomic returned_{false}; +}; +} // namespace + +TEST_F(BasicCurlHttpTests, FinishFromTheCreateFailedEventReturns) +{ + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:not-a-port"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + handler->finish_target_ = session.get(); + + session->SendRequest(handler); + + ASSERT_TRUE(handler->entered_.load(std::memory_order_acquire)) + << "the failure never reached the handler, so nothing was tested"; + EXPECT_TRUE(handler->returned_.load(std::memory_order_acquire)) + << "FinishSession from this event waited on a promise only its own caller can set"; + + session->FinishSession(); + session_manager->FinishAllSessions(); +} + TEST_F(BasicCurlHttpTests, InvalidUrlCompletes) { auto session_manager = std::make_shared()->Create(); From a6e867df6dbf1a6762a1708cb3234ff0b43d0a1b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 06:13:50 +0000 Subject: [PATCH 06/23] [TEST] Say which terminal outcome a cancel from Created actually gets The case counted terminal events and stopped there, so an honoured cancel and a request that was never registered looked the same to it. They are not the same, and what this path reports today is the second one. Created is dispatched from the constructor, before curl_operation_ holds the operation, so a cancel from that event reaches the Session but not the operation. Scheduling then finds no registration and reports a failed create. The caller asked to cancel and is told the create failed. Measured: zero Cancelled, one CreateFailed. An earlier measurement on this branch saw the opposite, before the unregistered path was reordered to finish before it reports, so the classification moved with that change and nothing here was watching. Pinned as it is rather than as it should be. Making it a cancel means moving the first events out of the constructor, which is the startup ordering the issue is about rather than something to add on the side. Pinning it means the day it changes is visible. The comment above the counting handler also still described the old ordering, where the event reached the handler before the state was stored. It stores first now. 32 tests pass, 3 of 3. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 31edbf823d..fc9c120cf4 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -95,7 +95,7 @@ class CustomEventHandler : public http_client::EventHandler // Counts the terminal notifications one request produces. Set cancel_at_response_ to cancel from // inside the Response event, which is the one moment both arms of the completion callback are -// eligible: DispatchEvent notifies the handler before it stores the new state, and the callback +// eligible: DispatchEvent stores the new state before it notifies the handler, and the callback // runs after both, so it sees an aborted operation that also has a response. class TerminalCountingHandler : public CustomEventHandler { @@ -117,6 +117,7 @@ class TerminalCountingHandler : public CustomEventHandler else if (state == http_client::SessionState::Cancelled) { terminal_count_.fetch_add(1, std::memory_order_release); + cancelled_.fetch_add(1, std::memory_order_release); // Cleanup dispatches its own Cancelled carrying a curl message, and GetCurlErrorMessage // never yields an empty one, so an empty reason is the completion callback and only it. if (reason.empty()) @@ -141,6 +142,7 @@ class TerminalCountingHandler : public CustomEventHandler std::atomic terminal_count_{0}; std::atomic cancelled_from_callback_{0}; std::atomic create_failed_{0}; + std::atomic cancelled_{0}; std::atomic last_reason_empty_{true}; }; @@ -719,7 +721,16 @@ TEST_F(BasicCurlHttpTests, CancelFromCreatedCompletes) session_manager->FinishAllSessions(); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); + // Exact, and the classification with it, because a count alone cannot tell an honoured cancel + // from a request that was never registered. What it reports today is the second one: Created is + // dispatched from the constructor, before curl_operation_ holds this operation, so the cancel + // reaches the Session but not the operation, and scheduling then finds no registration. The + // caller asked to cancel and is told the create failed. Moving the first events out of the + // constructor is what would make this a cancel, and that is the startup ordering #4390 is + // about rather than something to bolt on here. Pinned so the day it changes is visible. EXPECT_EQ(1, handler->terminal_count_.load(std::memory_order_acquire)); + EXPECT_EQ(0, handler->cancelled_.load(std::memory_order_acquire)); + EXPECT_EQ(1, handler->create_failed_.load(std::memory_order_acquire)); } TEST_F(BasicCurlHttpTests, CancelFromConnectingCompletes) From 329b0f8a293d5b157a1623726a3e4b0b8f3fb96a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 06:20:12 +0000 Subject: [PATCH 07/23] [TEST] Bound the curl cases, and check the layout claim instead of asserting it Two things this branch said but did not enforce. The cases here cover waits that are meant to end. When one stops ending it hangs rather than fails, and nothing bounded them, so a regression took the whole job with it instead of reporting. That happened three times while this branch was being written, each time as exit 124 with no case after it running. Every curl case now carries a CTest timeout of 120 seconds, read back from ctest --show-only=json-v1 as 120.0 on all 35. The slowest case that legitimately waits takes thirty. The description also claimed that std::atomic is one byte, aligned to one, and lock free, so the installed layout does not move. That was a measurement on one toolchain stated as a property of the language. The standard promises none of it. The size and alignment halves are now static_asserts, so a toolchain where they do not hold says so while building rather than silently changing an installed type. 32 tests pass. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_operation_curl.cc | 12 ++++++++++++ ext/test/http/CMakeLists.txt | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 8a253475b8..0a43607cb0 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -401,6 +401,18 @@ int HttpOperation::OnProgressCallback(void *clientp, } #endif +// The atomic member is only free of layout cost while it stays the size and alignment of the +// enum it replaced. The standard does not promise that, so it is checked here rather than +// asserted in prose: a toolchain where it does not hold changes an installed type and should say +// so at build time. +static_assert(sizeof(std::atomic) == + sizeof(opentelemetry::ext::http::client::SessionState), + "std::atomic grew, which changes the layout of HttpOperation"); +static_assert( + alignof(std::atomic) == + alignof(opentelemetry::ext::http::client::SessionState), + "std::atomic is more aligned, which changes the layout of HttpOperation"); + void HttpOperation::DispatchEvent(opentelemetry::ext::http::client::SessionState type, const std::string &reason) { diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index a1780586ae..133d03d86b 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -15,6 +15,11 @@ if(OTELCPP_WITH_HTTP_CLIENT_CURL) TARGET ${FILENAME} TEST_PREFIX ext.http.curl. TEST_LIST ${FILENAME}) + # These cover waits that are meant to end. When one stops ending it hangs + # rather than fails, and without a bound it takes the whole job with it + # instead of reporting. The slowest case that legitimately waits takes thirty + # seconds. + set_tests_properties(${${FILENAME}} PROPERTIES TIMEOUT 120) endif() set(SOCKET_TOOLS_FILENAME socket_tools_test) From e970fe49240d106ef955ff0c40fe13fef720231e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 07:14:07 +0000 Subject: [PATCH 08/23] [TEST] Say that the overlapping callbacks are a record, not a contract The case asserts that two callbacks are inside the handler at once, which is what this client does today. EventHandler does not say whether one request's callbacks can overlap, so a handler written against the interface is not obliged to be re-entrant, and an assertion that reads like a promise is the wrong thing to leave behind. The number stays, because reaching that overlap is the point of the case and a client that began serialising callbacks per operation should be noticed rather than absorbed. The comment now says which of the two it is. 32 tests pass. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index fc9c120cf4..28b69afab6 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -942,6 +942,12 @@ TEST_F(BasicCurlHttpTests, CancelFromConnectingWhilePollingCompletes) session->FinishSession(); session_manager->FinishAllSessions(); + // Two callbacks are in the handler at once here. That is what this client does today, not + // something EventHandler promises: the interface says nothing about whether one request's + // callbacks can overlap, so a handler written against it is not obliged to be re-entrant. The + // number is pinned because this case exists to reach that overlap, and a client that started + // serialising callbacks per operation would be an improvement worth noticing rather than a + // silent change. Read it as a record of the shape, not as a contract to preserve. EXPECT_EQ(2, handler->max_concurrent_events_.load(std::memory_order_acquire)); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); EXPECT_EQ(1, handler->cancelled_from_callback_.load(std::memory_order_acquire)); From 231f16d98fb1edf5187d822f2e2aad25bf4ba681 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 07:33:43 +0000 Subject: [PATCH 09/23] [BUG] Publish the operation once, and complete it only at the end Startup handed the operation out in pieces. The Session route went out before the callback, and the promise was created after the first event, so a cancel arriving in between could reach an operation whose callback was still empty. Whoever noticed that cleanup had started could then publish the completion, which meant FinishSession could return while the IO thread was still inside a user handler. The route is now the last thing stored. Nothing else can bring the IO thread to this operation: Abort only raises a flag, and the abort queue is reached through that same route, so by the time the operation is reachable the callback, the promise and the future all exist. Cleanup is then the only thing that ever fulfils the promise, at its tail, after the terminal event and the completion callback have run, and the recheck that used to fulfil it from the caller is gone. A promise that exists before the first event would make a handler calling FinishSession from that event wait on itself, which is what the late future used to avoid. A thread local scope answers it directly: every user callback runs inside one, and Finish returns early when the calling thread is already inside a callback for this operation. It returns before the finished flag, so a caller outside the callback still gets to wait, which the flag used to swallow. Measured with a handler that holds inside the terminal event for 200 ms. Before, on the runs where the IO thread delivered that event, Finish returned in 0 ms with the handler still running. After, it waits 400 ms, the length of the two held events, and the handler is never still running when it returns: 6 runs of 6. The case that came out of that probe is on the branch, and its comment says what it does not prove. Which thread delivers the terminal event is not something it can choose, and on the caller-thread schedule there is nothing for Finish to wait for, so reverting this change leaves it green 10 times in 10. The evidence above is the before and after, not that case. 33 tests pass, 3 of 3. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../http/client/curl/http_operation_curl.cc | 67 ++++++++++++---- ext/test/http/curl_http_test.cc | 77 +++++++++++++++++++ 2 files changed, 128 insertions(+), 16 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 0a43607cb0..99dfb2696b 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -413,6 +413,39 @@ static_assert( alignof(opentelemetry::ext::http::client::SessionState), "std::atomic is more aligned, which changes the layout of HttpOperation"); +namespace +{ +// Which operations the calling thread is currently inside a callback for. A handler is allowed to +// call FinishSession() on the request it is being told about, and that call must not wait for a +// completion only the thread it is running on can publish. +thread_local std::vector callbacks_in_progress; + +class CallbackScope +{ +public: + explicit CallbackScope(const HttpOperation *operation) noexcept + { + callbacks_in_progress.push_back(operation); + } + ~CallbackScope() { callbacks_in_progress.pop_back(); } + + CallbackScope(const CallbackScope &) = delete; + CallbackScope &operator=(const CallbackScope &) = delete; + + static bool InsideCallbackFor(const HttpOperation *operation) noexcept + { + for (const auto *entry : callbacks_in_progress) + { + if (entry == operation) + { + return true; + } + } + return false; + } +}; +} // namespace + void HttpOperation::DispatchEvent(opentelemetry::ext::http::client::SessionState type, const std::string &reason) { @@ -422,6 +455,7 @@ void HttpOperation::DispatchEvent(opentelemetry::ext::http::client::SessionState if (event_handle_ != nullptr) { + const CallbackScope scope{this}; event_handle_->OnEvent(type, reason); } } @@ -514,6 +548,14 @@ HttpOperation::~HttpOperation() void HttpOperation::Finish() { + // Called from inside a callback this operation dispatched, so the completion being waited for is + // the one this thread has not published yet. Returning before the flag below leaves the wait + // available to a caller that is not inside a callback. + if (CallbackScope::InsideCallbackFor(this)) + { + return; + } + if (is_finished_.exchange(true, std::memory_order_acq_rel)) { return; @@ -572,6 +614,7 @@ void HttpOperation::Cleanup() callback.swap(async_data_->callback); if (callback) { + const CallbackScope scope{this}; HttpOperationAccessor::SetThreadId(*async_data_, std::this_thread::get_id()); callback(*this); HttpOperationAccessor::SetThreadId(*async_data_, std::thread::id()); @@ -1472,26 +1515,18 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionsession.store(session, std::memory_order_release); - async_data_->callback = std::move(callback); - - DispatchEvent(opentelemetry::ext::http::client::SessionState::Connecting); - - // The future alone stays unpublished until after the event, so a handler calling - // FinishSession() from it returns instead of waiting on an unscheduled transfer. + async_data_->callback = std::move(callback); async_data_->result_promise = std::promise(); async_data_->result_future = async_data_->result_promise.get_future(); async_data_->is_promise_running.store(true, std::memory_order_release); - // A cancel from the event may have torn the operation down before the future existed. - if (is_cleaned_.load(std::memory_order_acquire)) - { - if (async_data_->is_promise_running.exchange(false, std::memory_order_acq_rel)) - { - async_data_->result_promise.set_value(last_curl_result_); - } - return CURLE_OK; - } + // Last, and the only thing that makes this operation reachable from the IO thread: an abort is + // queued through this route, so nothing can bring the operation there before the callback and + // the completion above exist. Cleanup is therefore the only thing that ever fulfils the promise, + // at its tail, once the terminal event and the completion callback have run. + async_data_->session.store(session, std::memory_order_release); + + DispatchEvent(opentelemetry::ext::http::client::SessionState::Connecting); if (WasAborted()) { diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 28b69afab6..537aa08b73 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -912,6 +912,83 @@ class OverlappingCancelHandler : public TerminalCountingHandler // the first event of the first request has nothing to overlap. On a client that is already // polling, the IO thread finishes the operation while the handler is still inside that event, // and the caller reaches the end of SendAsync with the operation already cleaned up. +namespace +{ +// Cancels from Connecting, then holds inside the terminal event for a bounded 200 ms. The bound is +// its own, so this can never block for good and a case built on it can only fail, not hang. +class LatchedCancelHandler : public TerminalCountingHandler +{ +public: + void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override + { + TerminalCountingHandler::OnEvent(state, reason); + if (state == http_client::SessionState::Cancelled) + { + entries_.fetch_add(1, std::memory_order_relaxed); + if (std::this_thread::get_id() == owner_) + { + on_owner_thread_.fetch_add(1, std::memory_order_relaxed); + } + inside_.fetch_add(1, std::memory_order_release); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + inside_.fetch_sub(1, std::memory_order_release); + } + } + + std::thread::id owner_{}; + std::atomic entries_{0}; + std::atomic on_owner_thread_{0}; + std::atomic inside_{0}; +}; +} // namespace + +// Holds that Finish does not return while a handler is still running. Which thread delivers the +// terminal event is not something this case can choose: when the caller thread delivers it there +// is nothing for Finish to wait for and the case only confirms that. Measured across separate +// runs, the IO thread takes it roughly one time in three, and that is the run that matters. So +// this asserts a true invariant on either schedule but does not on its own discriminate the +// ordering change it came from; the evidence for that is in the commit that made it. +TEST_F(BasicCurlHttpTests, FinishDoesNotReturnWhileAHandlerIsRunning) +{ + received_requests_.clear(); + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + { + auto warm = session_manager->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(); + } + + auto session = session_manager->CreateSession("http://127.0.0.1:19937"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + handler->owner_ = std::this_thread::get_id(); + handler->cancel_target_ = session.get(); + handler->cancel_at_ = http_client::SessionState::Connecting; + + session->SendRequest(handler); + + const auto start = std::chrono::steady_clock::now(); + session->FinishSession(); + const auto finished = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start) + .count(); + const int still_inside = handler->inside_.load(std::memory_order_acquire); + session_manager->FinishAllSessions(); + + EXPECT_EQ(0, still_inside) << "Finish returned after " << finished + << " ms with a handler still running"; + EXPECT_GE(handler->entries_.load(std::memory_order_relaxed), 1) + << "the terminal event never arrived, so nothing was tested"; +} + TEST_F(BasicCurlHttpTests, CancelFromConnectingWhilePollingCompletes) { received_requests_.clear(); From db30aa3d39eb1289a5136567970f01f5eb746b3c Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:56:26 +0000 Subject: [PATCH 10/23] [BUG] Say the same thing on both paths that end an unscheduled operation Of the three ways an operation ends up with nothing to run it, two told the handler CreateFailed and the third told it Cancelled. The enum documents that one as "(manually) cancelled", and both exporters print that word: the OTLP HTTP client logs "Session state: (manually) cancelled." and the Elasticsearch exporter logs "(manually) cancelled". So a handle libcurl refused to schedule reached the operator as a request somebody cancelled, and the reason libcurl gave for refusing it was not reported at all. Nothing behaves differently for it. Both states set need_stop in the OTLP handler and both end the wait in the Elasticsearch one, so what changes is the message, from a cancel that did not happen to a failed create carrying curl_multi_strerror. The ordering the registered path already used is now one method rather than three lines in two places: the state goes in ahead of the cleanup, which would otherwise report the cancel, and the cleanup goes in ahead of the event, so a handler calling FinishSession() from it is not waiting on the promise that cleanup is the only thing able to fulfil. That branch had no test. It has one now, and it needs no seam around libcurl: SendAsync does the whole async setup and Session::SendRequest is what starts the worker, so a case can build the operation, send it, put the client on a multi handle that refuses every add, and drive doAddSessions itself on one thread. Reverting the finish to the plain one it had before fails the case on both assertions. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 + .../http/client/curl/http_operation_curl.h | 9 ++ ext/src/http/client/curl/http_client_curl.cc | 13 ++- .../http/client/curl/http_operation_curl.cc | 28 ++++-- ext/test/http/curl_http_test.cc | 93 +++++++++++++++++++ 5 files changed, 133 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3607ebe7eb..60b4db3f99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,9 @@ Increment the: * [BUG] Finish a curl operation that never gets scheduled, instead of leaving FinishSession blocked forever ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) +* [BUG] Report a curl request the multi handle refused as a failed create, + rather than as a cancel nobody asked for + ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) * [BUG] Store the curl session state before the event that reports it, so a cancel dispatched by the IO thread is not overwritten or raced ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) 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 3205b6bcbc..9cf6178c17 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 @@ -230,6 +230,15 @@ class HttpOperation */ CURLcode SendAsync(Session *session, std::function callback = nullptr); + /** + * Finish an operation that nothing is going to run, and say why. + * + * The state goes in ahead of the cleanup, which would otherwise report a cancel nobody asked + * for, and the cleanup goes in ahead of the event, so a handler that calls FinishSession() + * from it is not waiting on the promise that this cleanup is the only thing able to fulfil. + */ + void FinishUnscheduled(const char *reason); + inline void SendSync() { Send(); } /** diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 0fd09400a0..b19d69a910 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -783,9 +783,16 @@ bool HttpClient::doAddSessions() // that cancels from it takes that lock again. See #4389. for (auto &rejected : rejected_by_multi) { - OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_add_handle failed: " - << curl_multi_strerror(rejected.second)); - rejected.first->FinishOperation(); + const char *reason = curl_multi_strerror(rejected.second); + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_add_handle failed: " << reason); + + // Told the same way as a session this client never registered, because it is the same thing + // from the handler's side: nothing is going to run this request. + auto &operation = rejected.first->GetOperation(); + if (operation) + { + operation->FinishUnscheduled(reason); + } } // Finishing a rejected session queues its removal, and the loop's idle check has already run diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 99dfb2696b..fa17226b9f 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1536,16 +1536,8 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functionGetHttpClient().ScheduleAddSession(session->GetSessionId())) { - // The same, except nobody cancelled anything. The terminal state goes in first so Cleanup() - // does not report a manual cancel, with a reason read from a curl result that is still - // CURLE_OK, and the operation is finished before the handler hears about it: a handler that - // calls FinishSession() from this event would otherwise wait on the promise that the - // Cleanup() below it is the only thing able to fulfil. - session_state_.store(opentelemetry::ext::http::client::SessionState::CreateFailed, - std::memory_order_release); - Cleanup(); - DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, - "the session is not registered with this client"); + // The same, except nobody cancelled anything. + FinishUnscheduled("the session is not registered with this client"); } return CURLE_OK; @@ -1605,6 +1597,22 @@ void HttpOperation::Abort() } } +void HttpOperation::FinishUnscheduled(const char *reason) +{ + // Ahead of the cleanup: Cleanup() reports a manual cancel for any state short of a terminal + // one, with a reason read from a curl result that is still CURLE_OK, and nobody cancelled + // this. The enum documents Cancelled as manually cancelled and both exporters print that word. + session_state_.store(opentelemetry::ext::http::client::SessionState::CreateFailed, + std::memory_order_release); + + // Ahead of the event, so a handler calling FinishSession() from it is not waiting on the + // promise that this cleanup is the only thing able to fulfil. + Cleanup(); + + DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, + nullptr != reason ? reason : ""); +} + void HttpOperation::PerformCurlMessage(CURLcode code) { ++retry_attempts_; diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 537aa08b73..a962bc1f48 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -56,6 +57,19 @@ class HttpClientTestPeer { public: static void ResetMultiHandle(HttpClient &client) { client.resetMultiHandle(); } + + static bool AddSessions(HttpClient &client) { return client.doAddSessions(); } + + static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + + // A multi handle that failed to initialize refuses every add, which is the state the case + // below needs and the one thing no transfer can be arranged into. + static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) + { + CURLM *previous = client.multi_handle_; + client.multi_handle_ = replacement; + return previous; + } }; } // namespace curl } // namespace client @@ -639,6 +653,85 @@ TEST_F(BasicCurlHttpTests, ResetMultiHandleWithASessionDoesNotDeadlock) client->FinishAllSessions(); } +class RecordingHandler : public CustomEventHandler +{ +public: + void OnResponse(http_client::Response & /* response */) noexcept override + { + got_response_.store(true, std::memory_order_release); + } + + void OnEvent(http_client::SessionState state, nostd::string_view /* reason */) noexcept override + { + std::lock_guard lock_guard{states_m_}; + states_.push_back(state); + } + + std::vector States() + { + std::lock_guard lock_guard{states_m_}; + return states_; + } + +private: + std::mutex states_m_; + std::vector states_; +}; + +// The third way an operation ends up with nothing to run it. SendAsync does the whole async +// setup and Session::SendRequest is what spawns the worker, so the add runs here on one thread +// against a multi handle that refuses it, which is what a multi handle that failed to initialize +// does to every add. +TEST_F(BasicCurlHttpTests, ASessionTheMultiHandleRefusesIsFinished) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + auto curl_session = std::static_pointer_cast(session); + + auto handler = std::make_shared(); + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::RetryPolicy no_retry{}; + + // Named, and outliving the operation: it keeps a reference to the ssl options, the headers, + // the body and this rather than a copy of any of them. + http_client::Compression compression = http_client::Compression::kNone; + + curl_session->GetOperation().reset(new curl::HttpOperation( + http_client::Method::Get, "http://127.0.0.1:19000/get/", no_ssl, handler.get(), headers, body, + compression, false, curl::kDefaultHttpConnTimeout, false, false, no_retry)); + + std::atomic completed{0}; + ASSERT_EQ(CURLE_OK, curl_session->GetOperation()->SendAsync( + curl_session.get(), [&completed](curl::HttpOperation & /* operation */) { + completed.fetch_add(1, std::memory_order_release); + })); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + const bool has_data = http_client::curl::HttpClientTestPeer::AddSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + // The removal the finish queues is the reason this has to answer true: the worker checks it + // after it has already run doRemoveSessions for this round, and nothing else would drain it. + EXPECT_TRUE(has_data); + EXPECT_EQ(1, completed.load(std::memory_order_acquire)); + + // Told the same way as a session this client never registered. Not Cancelled: the enum calls + // that one manually cancelled and both exporters print that word, and nobody cancelled this. + const auto states = handler->States(); + ASSERT_FALSE(states.empty()); + EXPECT_EQ(http_client::SessionState::CreateFailed, states.back()); + for (const auto state : states) + { + EXPECT_NE(http_client::SessionState::Cancelled, state); + } + + http_client::curl::HttpClientTestPeer::RemoveSessions(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 a3701f20167767727fc735e241bb053c2d670f32 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:45:55 +0000 Subject: [PATCH 11/23] [TEST] Wait on the count that only goes up, and say when the wait gives up The overlapping cancel case holds the calling thread inside the first event until the background thread has entered one of its own, so the two really do overlap. It waited on inside_events_, which is raised on the way into an event and lowered on the way out, and the event the background thread dispatches there is a few atomics long. Sampling that every millisecond almost never catches it at two, so the bound was spent in full on every run: 30005, 30509, 30006 and 30015 ms across four runs that all passed, because the count the assertion reads is written by the thread that creates the overlap and never goes down. Waiting on that count instead leaves as soon as the overlap has happened. The case goes from 30006 ms to 511 ms, ten runs out of ten, and the whole binary from about 53 seconds to 23.5. It also makes the case detect what it exists for. Against a faithful revert of what this branch changed about session_state_, a plain member again and the store back after the handler, ThreadSanitizer reports the race five runs out of five with the shorter wait and none at all with the longer one, which spends thirty seconds and a great many reads of that same address between the two writes. With the change in place it reports none either way. A bound that does expire now means the overlap really did not happen, so it is recorded and checked rather than left to be read as the client having dispatched one event. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index a962bc1f48..9fe9bd30e0 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -986,12 +986,24 @@ class OverlappingCancelHandler : public TerminalCountingHandler // Cancelling wakes the IO thread, which finishes the operation and dispatches a Cancelled // of its own. Stay in this event until that one arrives so the two really do overlap, and // give up rather than hang if it never does. + // + // On the count that only goes up. inside_events_ is lowered again on the way out of that + // event, which is a few atomics long, so sampling it every millisecond almost never catches + // it and the bound below is spent in full even though the overlap did happen. const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); - while (inside_events_.load(std::memory_order_relaxed) < 2 && + while (max_concurrent_events_.load(std::memory_order_relaxed) < 2 && std::chrono::steady_clock::now() < deadline) { std::this_thread::sleep_for(std::chrono::milliseconds(1)); } + + if (max_concurrent_events_.load(std::memory_order_relaxed) < 2) + { + // Only a run where the overlap never happened reaches this, and it is worth saying so: + // the count checked at the end would otherwise read as the client dispatching one event + // rather than as the other thread never turning up. + overlap_timed_out_.store(true, std::memory_order_release); + } } inside_events_.fetch_sub(1, std::memory_order_relaxed); @@ -999,6 +1011,7 @@ class OverlappingCancelHandler : public TerminalCountingHandler std::atomic inside_events_{0}; std::atomic max_concurrent_events_{0}; + std::atomic overlap_timed_out_{false}; }; // A client spawns its IO thread only after a request has been scheduled, so cancelling from @@ -1118,6 +1131,7 @@ TEST_F(BasicCurlHttpTests, CancelFromConnectingWhilePollingCompletes) // number is pinned because this case exists to reach that overlap, and a client that started // serialising callbacks per operation would be an improvement worth noticing rather than a // silent change. Read it as a record of the shape, not as a contract to preserve. + EXPECT_FALSE(handler->overlap_timed_out_.load(std::memory_order_acquire)); EXPECT_EQ(2, handler->max_concurrent_events_.load(std::memory_order_acquire)); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); EXPECT_EQ(1, handler->cancelled_from_callback_.load(std::memory_order_acquire)); From d08837b72bdbcf4785e807d25ff0f209442ccd61 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:39:17 +0000 Subject: [PATCH 12/23] the includes the case needs, and none it does not A probe left cstdio behind when its printf went, and the lambda handed to SendAsync needs functional. Both are what include-what-you-use asks for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 9fe9bd30e0..3d2951f3f6 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -16,8 +16,8 @@ #include #include #include -#include #include +#include #include #include #include From f53d6b9087d00f8439c49d0c69185065896c2ed1 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:57:11 +0000 Subject: [PATCH 13/23] [TEST] A session a reset took before its id was queued still finishes resetMultiHandle keeps the sessions whose ids are already in pending_to_add_session_ids_ and takes the rest. A request that has not reached ScheduleAddSession yet is one of the rest: the caller is between CreateSession, which registered the session, and SendAsync, which is what queues the id. So the reset cancels a session whose operation is about to be given a promise, and on main the id then goes into the queue whatever became of the session, where doAddSessions finds nothing to add and moves on. The promise is never fulfilled and FinishSession never returns. This branch already refuses that id, and this is the case for it. Nothing is sent before the reset, so there is no IO thread and the caller stands exactly where the interleaving puts it, in program order rather than in a window. That also keeps it away from the multi handle: taking one from a running thread is not something libcurl allows, and a case that did it could not say whether what it saw was the client or itself. Against a faithful revert of what this branch changed about ScheduleAddSession, the id going in whatever became of the session, it hangs: three runs out of three reach the 90 second bound with no case finished. It passes in 503 ms with the change, three out of three. All 35 cases in the binary pass, in 24.5 seconds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 34 +++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 3d2951f3f6..ffd693180a 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1368,6 +1368,40 @@ TEST_F(BasicCurlHttpTests, FinishInAsyncCallback) } } +TEST_F(BasicCurlHttpTests, ASessionResetTookBeforeItWasQueuedIsFinished) +{ + 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("get/"); + + // The interleaving, in program order, which is what makes it a case rather than a window. + // A reset keeps the sessions whose ids are already in pending_to_add_session_ids_ and takes + // the rest, and a request that has not reached ScheduleAddSession yet is one of the rest: + // the caller is between CreateSession, which registered it, and SendAsync, which is what + // queues the id. Nothing here is sent, so the IO thread does not exist and this thread is + // standing exactly where it would be standing. + http_client::curl::HttpClientTestPeer::ResetMultiHandle(*concrete); + + auto handler = std::make_shared(); + session->SendRequest(handler); + + // Nothing is going to run this operation: the session it names is not registered any more, + // and adding the id back would leave the caller waiting on a transfer nobody arranged. So + // what has to happen is that it is finished. Returning from here is the assertion, and + // without it this hangs rather than fails. + session->FinishSession(); + + const auto states = handler->States(); + ASSERT_FALSE(states.empty()); + EXPECT_EQ(http_client::SessionState::CreateFailed, states.back()); + + client->FinishAllSessions(); +} + TEST_F(BasicCurlHttpTests, ElegantQuitQuick) { auto http_client = std::make_shared()->Create(); From c074a443a67449776499f81a17c94a21f3b91bb5 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:09:37 +0000 Subject: [PATCH 14/23] Take the include include-what-you-use asks for The case reads CURLM, and curlver.h is not where that comes from. 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 ffd693180a..df7baf5522 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #include #include "gtest/gtest.h" From df6f7304e22ec32805eaa973da478ab37cd7ace5 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:09:37 +0000 Subject: [PATCH 15/23] Include curl.h once, not twice The copy inside the retry guard was there for the case that needs gmock. The one added at the top for include-what-you-use covers every build, so the guarded one is a duplicate, and the abiv2-preview job says so while abiv1 does not. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/curl_http_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index df7baf5522..cc7df8972a 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -6,7 +6,6 @@ #include "gtest/gtest.h" #ifdef ENABLE_OTLP_RETRY_PREVIEW -# include # include "gmock/gmock.h" #endif // ENABLE_OTLP_RETRY_PREVIEW From 9ae97cc95531685658171ae5ceab2bee725b3d2a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 17:49:15 +0000 Subject: [PATCH 16/23] [BUG] Tell the handler before letting go of what keeps it alive FinishUnscheduled() cleaned up and then dispatched the terminal event. The operation holds the caller's handler as a bare pointer, and the only strong reference to it is the one Session::SendRequest captured in the completion callback, so Cleanup() taking that callback and letting it go is what frees the handler. The event that followed read it. AddressSanitizer reports a heap-use-after-free at the OnEvent call, reached from FinishUnscheduled, on the path where curl_multi_add_handle refuses a session. Nothing in the suite caught it because every case that reaches this path either keeps its own reference to the handler or hands SendAsync a completion that captures none, so the ownership that breaks was never assembled. The event now goes first. A handler that calls FinishSession() from it is inside a callback for this operation, so Finish() returns instead of waiting on a promise this thread has not published, which is what made the other order necessary before CallbackScope existed. DispatchEvent stores the state ahead of the handler, so the cleanup that follows still sees a terminal state and still does not report a manual cancel for something nobody cancelled. Cleaning up last also means a Finish() on another thread waits for the terminal event and the completion callback rather than for the promise alone. The new case is the only one in the file that leaves the completion callback as the sole owner of the handler, which is what every caller of Session::SendRequest does. It records its own event against its own destruction into state the case owns, so the order outlives the handler and the case fails without a sanitizer as well as with one. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../http/client/curl/http_operation_curl.cc | 23 +++-- ext/test/http/curl_http_test.cc | 95 +++++++++++++++++++ 2 files changed, 108 insertions(+), 10 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index fa17226b9f..2cbeba5357 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1599,18 +1599,21 @@ void HttpOperation::Abort() void HttpOperation::FinishUnscheduled(const char *reason) { - // Ahead of the cleanup: Cleanup() reports a manual cancel for any state short of a terminal - // one, with a reason read from a curl result that is still CURLE_OK, and nobody cancelled - // this. The enum documents Cancelled as manually cancelled and both exporters print that word. - session_state_.store(opentelemetry::ext::http::client::SessionState::CreateFailed, - std::memory_order_release); - - // Ahead of the event, so a handler calling FinishSession() from it is not waiting on the - // promise that this cleanup is the only thing able to fulfil. - Cleanup(); - + // The event first, because the operation holds the handler as a bare pointer and the only + // strong reference to it is the one the completion callback captured. Cleanup() takes that + // callback and lets it go, so an event dispatched afterwards can be talking to a handler + // nothing owns any more. A handler calling FinishSession() from here is inside a callback for + // this operation, so Finish() returns rather than waiting on a promise this thread has not + // published yet, which is what used to make the other order necessary. + // + // DispatchEvent stores the state before it calls the handler, so the cleanup below sees a + // terminal state and does not report a manual cancel for something nobody cancelled. DispatchEvent(opentelemetry::ext::http::client::SessionState::CreateFailed, nullptr != reason ? reason : ""); + + // Last, so that a Finish() on another thread waits for the terminal event and the completion + // callback rather than for the promise alone. + Cleanup(); } void HttpOperation::PerformCurlMessage(CURLcode code) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index cc7df8972a..256138aa14 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -678,6 +678,101 @@ class RecordingHandler : public CustomEventHandler std::vector states_; }; +// Owned by the case rather than by the handler, so the order survives the handler being +// destroyed. That is what lets the case below tell "the event ran while the handler was alive" +// from "the event ran on memory nothing owned any more" without depending on a sanitizer, +// though AddressSanitizer reports the second one outright. +struct LifetimeProbe +{ + std::atomic next{0}; + std::atomic event_at{-1}; + std::atomic destroyed_at{-1}; +}; + +class ProbedHandler : public CustomEventHandler +{ +public: + explicit ProbedHandler(std::shared_ptr probe) : probe_{std::move(probe)} {} + + ~ProbedHandler() override + { + probe_->destroyed_at.store(probe_->next.fetch_add(1, std::memory_order_acq_rel), + std::memory_order_release); + } + + ProbedHandler(const ProbedHandler &) = delete; + ProbedHandler(ProbedHandler &&) = delete; + ProbedHandler &operator=(const ProbedHandler &) = delete; + ProbedHandler &operator=(ProbedHandler &&) = delete; + + void OnResponse(http_client::Response & /* response */) noexcept override {} + + void OnEvent(http_client::SessionState /* state */, + nostd::string_view /* reason */) noexcept override + { + probe_->event_at.store(probe_->next.fetch_add(1, std::memory_order_acq_rel), + std::memory_order_release); + } + +private: + std::shared_ptr probe_; +}; + +// The operation holds the handler as a bare pointer, and what keeps the caller's handler alive +// past SendRequest is the shared_ptr the completion callback captured. Cleanup() takes that +// callback and lets it go, so anything dispatched after Cleanup() is talking to whatever is left +// of the handler. This case is the only one in the file that leaves the completion callback as +// the sole owner, which is exactly what Session::SendRequest arranges for every real caller. +TEST_F(BasicCurlHttpTests, AnUnscheduledSessionTellsAHandlerNothingElseHolds) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + auto curl_session = std::static_pointer_cast(session); + + auto probe = std::make_shared(); + auto handler = std::make_shared(probe); + + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::RetryPolicy no_retry{}; + http_client::Compression compression = http_client::Compression::kNone; + + curl_session->GetOperation().reset(new curl::HttpOperation( + http_client::Method::Get, "http://127.0.0.1:19000/get/", no_ssl, handler.get(), headers, body, + compression, false, curl::kDefaultHttpConnTimeout, false, false, no_retry)); + + std::atomic completed{0}; + ASSERT_EQ(CURLE_OK, + curl_session->GetOperation()->SendAsync( + curl_session.get(), [handler, &completed](curl::HttpOperation & /* operation */) { + completed.fetch_add(1, std::memory_order_release); + })); + + // Deliberately the last reference the case holds, the same as a caller that hands its handler + // to SendRequest and keeps nothing of its own. + handler.reset(); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + const bool has_data = http_client::curl::HttpClientTestPeer::AddSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_TRUE(has_data); + EXPECT_EQ(1, completed.load(std::memory_order_acquire)); + + const int event_at = probe->event_at.load(std::memory_order_acquire); + const int destroyed_at = probe->destroyed_at.load(std::memory_order_acquire); + ASSERT_GE(event_at, 0) << "the handler was never told that nothing would run its request"; + ASSERT_GE(destroyed_at, 0) << "the handler outlived the operation, so this case is not holding " + << "the ownership it says it is"; + EXPECT_LT(event_at, destroyed_at) + << "the terminal event reached the handler after the last reference to it had gone"; + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); +} + // The third way an operation ends up with nothing to run it. SendAsync does the whole async // setup and Session::SendRequest is what spawns the worker, so the add runs here on one thread // against a multi handle that refuses it, which is what a multi handle that failed to initialize From 2ebb518f7a647592a06fada10ec191dc7647d51c Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:03:20 +0000 Subject: [PATCH 17/23] [CHORE] Enter a callback scope without allocating, and say what the order is CallbackScope kept its entries in a thread_local vector, so entering one could reallocate, and it did that from a noexcept constructor where a bad_alloc calls std::terminate instead of reaching whoever asked for the request. The scopes are already stack objects living exactly as long as the entry needs to, so they can hold the links themselves: each remembers the one it displaced and puts it back on the way out. Nested dispatches, reentrancy from a different operation and other threads all behave as before, with no allocation on any event path. The header still described FinishUnscheduled as cleaning up before dispatching. It does the opposite now, for the handler's sake, so the comment said the wrong thing about the code underneath it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../http/client/curl/http_operation_curl.h | 8 +++--- .../http/client/curl/http_operation_curl.cc | 25 ++++++++++++++----- 2 files changed, 24 insertions(+), 9 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 9cf6178c17..c7fd8a1eec 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 @@ -233,9 +233,11 @@ class HttpOperation /** * Finish an operation that nothing is going to run, and say why. * - * The state goes in ahead of the cleanup, which would otherwise report a cancel nobody asked - * for, and the cleanup goes in ahead of the event, so a handler that calls FinishSession() - * from it is not waiting on the promise that this cleanup is the only thing able to fulfil. + * The event goes in ahead of the cleanup, because the only strong reference to the caller's + * handler is the one the completion callback holds and the cleanup is what lets that go. The + * event carries the terminal state with it, so the cleanup still does not report a cancel + * nobody asked for, and cleaning up last is what makes a Finish() on another thread wait for + * the event rather than for the promise alone. */ void FinishUnscheduled(const char *reason); diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 2cbeba5357..203ffc8147 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -418,32 +418,45 @@ namespace // Which operations the calling thread is currently inside a callback for. A handler is allowed to // call FinishSession() on the request it is being told about, and that call must not wait for a // completion only the thread it is running on can publish. -thread_local std::vector callbacks_in_progress; - +// A stack of scopes linked through the scopes themselves, so entering one allocates nothing. Each +// lives on the stack frame that dispatches the callback, which is exactly as long as the entry +// needs to be there. A container here would put a heap allocation on every event, and would put +// it inside a noexcept constructor, where running out of memory calls std::terminate rather than +// reaching whoever asked for the request. class CallbackScope { public: explicit CallbackScope(const HttpOperation *operation) noexcept + : operation_{operation}, previous_{current_} { - callbacks_in_progress.push_back(operation); + current_ = this; } - ~CallbackScope() { callbacks_in_progress.pop_back(); } + + ~CallbackScope() { current_ = previous_; } CallbackScope(const CallbackScope &) = delete; CallbackScope &operator=(const CallbackScope &) = delete; static bool InsideCallbackFor(const HttpOperation *operation) noexcept { - for (const auto *entry : callbacks_in_progress) + for (const CallbackScope *scope = current_; nullptr != scope; scope = scope->previous_) { - if (entry == operation) + if (scope->operation_ == operation) { return true; } } return false; } + +private: + const HttpOperation *operation_; + const CallbackScope *previous_; + + static thread_local const CallbackScope *current_; }; + +thread_local const CallbackScope *CallbackScope::current_ = nullptr; } // namespace void HttpOperation::DispatchEvent(opentelemetry::ext::http::client::SessionState type, From 7b1e06f825da401c3617e6de7cdb38563ebab7f0 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:15:25 +0000 Subject: [PATCH 18/23] [BUG] Ask the callback scope which thread is in a callback, not an unsynchronized member AsyncData::callback_thread was a plain std::thread::id written on the thread running a callback and read by Finish() and by the destructor on whichever thread called them. The accessors put a standalone atomic_thread_fence on either side of a plain load and store, which does not order anything: fences synchronize through atomic operations on a common object, and there was none, so the two conflicting accesses had no happens-before between them and the program had a data race. ThreadSanitizer stayed quiet because both accessors carried OPENTELEMETRY_SANITIZER_NO_THREAD, and the fences were compiled out under that same sanitizer, so what it was told to ignore was a bare unsynchronized access. CallbackScope already answers the question the member existed to answer, and answers it without shared state, since the scope lives on the stack frame that dispatches the callback. Cleanup() already entered one around the completion callback, right beside the two writes this removes, and Finish() already consulted it and returned before ever reaching the member, which made the second check unreachable as anything but true. The destructor now asks the same way, which also covers a handler destroying its operation from inside an event rather than only from inside the completion. FinishUnscheduled goes private with the client as a friend. It forces a terminal state, cleans up, dispatches, fulfils the promise and hands the easy resource back, and the client is the only thing that ever discovers a request nothing will run. This header is not installed by CMake but Bazel exposes it, so a public method here is API surface. include-what-you-use asked for to be dropped from both files once the member was gone, and is clean on all three of its presets. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../http/client/curl/http_operation_curl.h | 30 ++++++------ .../http/client/curl/http_operation_curl.cc | 46 ++++--------------- 2 files changed, 25 insertions(+), 51 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 c7fd8a1eec..d73f325fee 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 @@ -20,7 +20,6 @@ #include #include #include -#include #include #ifdef _WIN32 # include @@ -230,17 +229,6 @@ class HttpOperation */ CURLcode SendAsync(Session *session, std::function callback = nullptr); - /** - * Finish an operation that nothing is going to run, and say why. - * - * The event goes in ahead of the cleanup, because the only strong reference to the caller's - * handler is the one the completion callback holds and the cleanup is what lets that go. The - * event carries the terminal state with it, so the cleanup still does not report a cancel - * nobody asked for, and cleaning up last is what makes a Finish() on another thread wait for - * the event rather than for the promise alone. - */ - void FinishUnscheduled(const char *reason); - inline void SendSync() { Send(); } /** @@ -302,6 +290,22 @@ class HttpOperation inline CURL *GetCurlEasyHandle() noexcept { return curl_resource_.easy_handle; } private: + // The client is what discovers that nothing will run a request, so it is what gets to say so. + friend class HttpClient; + + /** + * Finish an operation that nothing is going to run, and say why. Not for callers: it forces a + * terminal state, cleans up, dispatches the terminal event, fulfils the promise and hands the + * easy resource back, none of which is safe to ask for from outside the client. + * + * The event goes in ahead of the cleanup, because the only strong reference to the caller's + * handler is the one the completion callback holds and the cleanup is what lets that go. The + * event carries the terminal state with it, so the cleanup still does not report a cancel + * nobody asked for, and cleaning up last is what makes a Finish() on another thread wait for + * the event rather than for the promise alone. + */ + void FinishUnscheduled(const char *reason); + CURLcode SetCurlPtrOption(CURLoption option, void *value); CURLcode SetCurlStrOption(CURLoption option, const char *str) @@ -378,13 +382,11 @@ class HttpOperation // Read by Abort() on whichever thread cancels, cleared by Cleanup() on the IO thread. std::atomic session{nullptr}; // Owner Session - std::thread::id callback_thread; std::function callback; std::atomic is_promise_running{false}; std::promise result_promise; std::future result_future; }; - friend class HttpOperationAccessor; std::unique_ptr async_data_; }; } // 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 203ffc8147..196fb0453b 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -20,7 +20,6 @@ #include #include #include -#include #include #include @@ -98,28 +97,6 @@ namespace client namespace curl { -class HttpOperationAccessor -{ -public: - OPENTELEMETRY_SANITIZER_NO_THREAD static std::thread::id GetThreadId( - const HttpOperation::AsyncData &async_data) - { -#if !(defined(OPENTELEMETRY_HAVE_THREAD_SANITIZER) && OPENTELEMETRY_HAVE_THREAD_SANITIZER) - std::atomic_thread_fence(std::memory_order_acquire); -#endif - return async_data.callback_thread; - } - - OPENTELEMETRY_SANITIZER_NO_THREAD static void SetThreadId(HttpOperation::AsyncData &async_data, - std::thread::id thread_id) - { - async_data.callback_thread = thread_id; -#if !(defined(OPENTELEMETRY_HAVE_THREAD_SANITIZER) && OPENTELEMETRY_HAVE_THREAD_SANITIZER) - std::atomic_thread_fence(std::memory_order_release); -#endif - } -}; - size_t HttpOperation::WriteMemoryCallback(void *contents, size_t size, size_t nmemb, void *userp) { HttpOperation *self = reinterpret_cast(userp); @@ -542,13 +519,14 @@ HttpOperation::~HttpOperation() case opentelemetry::ext::http::client::SessionState::Connecting: case opentelemetry::ext::http::client::SessionState::Connected: case opentelemetry::ext::http::client::SessionState::Sending: { - if (async_data_ && async_data_->result_future.valid()) + // Not while inside a callback this operation dispatched: a handler that destroys the + // operation it is being told about would be waiting for a completion that only the thread + // running the handler can publish. + if (async_data_ && async_data_->result_future.valid() && + !CallbackScope::InsideCallbackFor(this)) { - if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id()) - { - async_data_->result_future.wait(); - last_curl_result_ = async_data_->result_future.get(); - } + async_data_->result_future.wait(); + last_curl_result_ = async_data_->result_future.get(); } break; } @@ -576,12 +554,8 @@ void HttpOperation::Finish() if (async_data_ && async_data_->result_future.valid()) { - // We should not wait in callback from Cleanup() - if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id()) - { - async_data_->result_future.wait(); - last_curl_result_ = async_data_->result_future.get(); - } + async_data_->result_future.wait(); + last_curl_result_ = async_data_->result_future.get(); } } @@ -628,9 +602,7 @@ void HttpOperation::Cleanup() if (callback) { const CallbackScope scope{this}; - HttpOperationAccessor::SetThreadId(*async_data_, std::this_thread::get_id()); callback(*this); - HttpOperationAccessor::SetThreadId(*async_data_, std::thread::id()); } // Set value to promise to continue Finish() From 14879a1f6e499294de5c12cf8b27af28c6f9e2c8 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:01:50 +0000 Subject: [PATCH 19/23] [TEST] Hold both halves of finishing from an event, and say the deadlock is closed Two cases, each shown to fail without the change it covers. FinishSessionFromAnEventTheIoThreadDelivers is #4402: a handler calls FinishSession() from ConnectFailed, which the IO thread delivers during a transfer, and the call has to come back. Remove the callback-scope check from Finish() and it hangs, reported as the case's own message rather than as a stuck binary, because it waits on a condition to a deadline instead of sleeping for a fixed time. FinishFromTheCreateFailedEventReturns covers only the event this client generates for an operation nothing will run, which is dispatched from whichever thread found it, so it never exercised the schedule the issue is about. FinishFromAnotherThreadWaitsForTheTerminalEvent is the other direction: an outside Finish() must wait for the terminal event and the completion callback, not just for the promise. The handler parks inside the event and a third thread watches the finisher stay in, since the thread dispatching the event is the one parked and cannot also let itself out. Put the cleanup back ahead of the event and the finisher returns while the handler is still running, which the case reports. The changelog now says the deadlock is closed. The mechanism was already general: the callback scope wraps every dispatch and Finish() consults it, so the handler is covered on the IO thread's events too, and narrowing it again would only put the deadlock back. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 4 + ext/test/http/curl_http_test.cc | 148 +++++++++++++++++++++++++++++++- 2 files changed, 151 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60b4db3f99..0b4c07f21f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,6 +119,10 @@ Increment the: * [BUG] Store the curl session state before the event that reports it, so a cancel dispatched by the IO thread is not overwritten or raced ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) +* [BUG] Let a handler finish the request it is being told about from any event, + including the ones the IO thread delivers, instead of waiting on a completion + only that thread goes on to publish + ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) * [CODE HEALTH] Enable clang-tidy `modernize-deprecated-headers` and replace deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 256138aa14..417af47a1c 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -993,7 +993,7 @@ class FinishFromEventHandler : public TerminalCountingHandler void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override { TerminalCountingHandler::OnEvent(state, reason); - if (state == http_client::SessionState::CreateFailed && finish_target_ != nullptr) + if (state == finish_at_ && finish_target_ != nullptr) { auto *target = finish_target_; finish_target_ = nullptr; @@ -1004,6 +1004,7 @@ class FinishFromEventHandler : public TerminalCountingHandler } http_client::Session *finish_target_ = nullptr; + http_client::SessionState finish_at_ = http_client::SessionState::CreateFailed; std::atomic entered_{false}; std::atomic returned_{false}; }; @@ -1190,6 +1191,151 @@ TEST_F(BasicCurlHttpTests, FinishDoesNotReturnWhileAHandlerIsRunning) << "the terminal event never arrived, so nothing was tested"; } +namespace +{ +// Stays inside the terminal event until the case lets it out, so another thread's FinishSession() +// has something to be held up by. +class HeldTerminalHandler : public CustomEventHandler +{ +public: + std::atomic inside_{false}; + std::atomic release_{false}; + + void OnResponse(http_client::Response & /* response */) noexcept override {} + + void OnEvent(http_client::SessionState state, nostd::string_view /* reason */) noexcept override + { + if (http_client::SessionState::CreateFailed != state) + { + return; + } + inside_.store(true, std::memory_order_release); + while (!release_.load(std::memory_order_acquire)) + { + std::this_thread::yield(); + } + inside_.store(false, std::memory_order_release); + } +}; + +// Polls a condition to a deadline rather than sleeping for a fixed time, so a pass costs only as +// long as the thing being waited for takes and a failure is a bounded wait rather than a hang. +template +bool WaitFor(Predicate ready, std::chrono::milliseconds budget) +{ + const auto deadline = std::chrono::steady_clock::now() + budget; + while (std::chrono::steady_clock::now() < deadline) + { + if (ready()) + { + return true; + } + std::this_thread::yield(); + } + return ready(); +} +} // namespace + +// A handler is allowed to finish the request it is being told about. On the IO thread's events +// that used to be a deadlock, because Finish() waited on a promise only the thread running the +// handler goes on to fulfil. Fixes #4402. +TEST_F(BasicCurlHttpTests, FinishSessionFromAnEventTheIoThreadDelivers) +{ + auto session_manager = std::make_shared()->Create(); + ASSERT_TRUE(session_manager != nullptr); + + auto session = session_manager->CreateSession("http://127.0.0.1:19937"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + + auto handler = std::make_shared(); + handler->finish_target_ = session.get(); + handler->finish_at_ = http_client::SessionState::ConnectFailed; + + // The handler has to be the first caller: a FinishSession() from here would take is_finished_ + // and every later one would return without ever reaching the wait. + session->SendRequest(handler); + + const bool entered = + WaitFor([&handler]() { return handler->entered_.load(std::memory_order_acquire); }, + std::chrono::seconds(30)); + ASSERT_TRUE(entered) << "the connect never failed, so no handler ran and nothing was tested"; + + const bool returned = + WaitFor([&handler]() { return handler->returned_.load(std::memory_order_acquire); }, + std::chrono::seconds(30)); + EXPECT_TRUE(returned) << "FinishSession() called from the event did not return"; + + session_manager->FinishAllSessions(); +} + +// The other half of the same ordering: an outside Finish() has to wait for the terminal event and +// the completion callback, not just for the promise. The handler holds the event open and the +// case watches the other thread stay inside FinishSession() until it lets go. +TEST_F(BasicCurlHttpTests, FinishFromAnotherThreadWaitsForTheTerminalEvent) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + auto curl_session = std::static_pointer_cast(session); + + auto handler = std::make_shared(); + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::RetryPolicy no_retry{}; + http_client::Compression compression = http_client::Compression::kNone; + + curl_session->GetOperation().reset(new curl::HttpOperation( + http_client::Method::Get, "http://127.0.0.1:19000/get/", no_ssl, handler.get(), headers, body, + compression, false, curl::kDefaultHttpConnTimeout, false, false, no_retry)); + + ASSERT_EQ(CURLE_OK, curl_session->GetOperation()->SendAsync( + curl_session.get(), [](curl::HttpOperation & /* operation */) {})); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + + std::atomic finish_returned{false}; + std::thread finisher([&curl_session, &finish_returned]() { + curl_session->FinishSession(); + finish_returned.store(true, std::memory_order_release); + }); + + // This thread is the one that dispatches the terminal event, so it is the one that parks inside + // the handler. Whoever lets it out has to be somebody else, and that somebody is also the only + // one placed to watch the finisher while the event is still running. + std::atomic observed_held{false}; + std::atomic returned_early{false}; + std::thread watcher([&handler, &finish_returned, &observed_held, &returned_early]() { + if (WaitFor([&handler]() { return handler->inside_.load(std::memory_order_acquire); }, + std::chrono::seconds(30))) + { + observed_held.store(true, std::memory_order_release); + returned_early.store( + WaitFor([&finish_returned]() { return finish_returned.load(std::memory_order_acquire); }, + std::chrono::milliseconds(200)), + std::memory_order_release); + } + handler->release_.store(true, std::memory_order_release); + }); + + http_client::curl::HttpClientTestPeer::AddSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + watcher.join(); + finisher.join(); + + ASSERT_TRUE(observed_held.load(std::memory_order_acquire)) + << "the terminal event never reached the handler, so nothing was tested"; + EXPECT_FALSE(returned_early.load(std::memory_order_acquire)) + << "FinishSession() on another thread returned while the handler was still inside the " + << "terminal event"; + EXPECT_TRUE(finish_returned.load(std::memory_order_acquire)); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); +} + TEST_F(BasicCurlHttpTests, CancelFromConnectingWhilePollingCompletes) { received_requests_.clear(); From a91857c8608fdcd266d77527f63dc614a939111e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:09:03 +0000 Subject: [PATCH 20/23] [TEST] Say which refusal a case means, rather than borrowing libcurl's The cases about a session the multi handle will not take used to swap the client's multi handle for a null one and call doAddSessions. That works, but it works by handing libcurl a handle its own documentation says not to use after curl_multi_init returns null, so the rejection under test was libcurl's defence against invalid input rather than a rejection it promises. All the manual says is that a non-zero return means the add failed. doAddSessions now goes through a member the client initialises to curl_multi_add_handle, and a case that wants a refusal names the code it wants back. The three cases that forced one keep a real multi handle and ask for CURLM_BAD_EASY_HANDLE, which is a code libcurl does document for an easy handle a multi already holds, and the seam is one indirect call on a path that already takes a lock and a map lookup. The swap is still there for the state it actually describes, a client whose multi handle could not be created at all. 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 | 2 +- ext/test/http/curl_http_test.cc | 22 ++++++++++++++----- 3 files changed, 23 insertions(+), 7 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 b0465b6dfc..85299be684 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 @@ -368,6 +368,12 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::mutex multi_handle_m_; CURLM *multi_handle_; + + // How a session is handed to the multi handle. Only a test replaces it, so that a case about + // what happens to a refused session can name the code libcurl returns instead of arranging a + // multi handle libcurl documents as unusable and relying on what it does with one. + CURLMcode (*add_handle_)(CURLM *, CURL *) = &curl_multi_add_handle; + std::atomic next_session_id_{0}; uint64_t max_sessions_per_connection_; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index b19d69a910..1161732d26 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::doAddSessions() continue; } - const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle); + const CURLMcode rc = add_handle_(multi_handle_, easy_handle); if (CURLM_OK != rc) { // Nothing will drive this transfer. Leaving it here is what makes a caller wait on a diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 417af47a1c..15c9143c71 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -62,6 +62,16 @@ class HttpClientTestPeer static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + // Refuses every add with a code the case chooses. A case about what happens to a refused + // session then says which refusal it means, instead of arranging a multi handle libcurl + // documents as unusable and depending on what libcurl does with one. + static void RefuseAdds(HttpClient &client) + { + client.add_handle_ = [](CURLM *, CURL *) { return CURLM_BAD_EASY_HANDLE; }; + } + + static void AllowAdds(HttpClient &client) { client.add_handle_ = &curl_multi_add_handle; } + // A multi handle that failed to initialize refuses every add, which is the state the case // below needs and the one thing no transfer can be arranged into. static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) @@ -755,9 +765,9 @@ TEST_F(BasicCurlHttpTests, AnUnscheduledSessionTellsAHandlerNothingElseHolds) // to SendRequest and keeps nothing of its own. handler.reset(); - CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RefuseAdds(client); const bool has_data = http_client::curl::HttpClientTestPeer::AddSessions(client); - http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + http_client::curl::HttpClientTestPeer::AllowAdds(client); EXPECT_TRUE(has_data); EXPECT_EQ(1, completed.load(std::memory_order_acquire)); @@ -805,9 +815,9 @@ TEST_F(BasicCurlHttpTests, ASessionTheMultiHandleRefusesIsFinished) completed.fetch_add(1, std::memory_order_release); })); - CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RefuseAdds(client); const bool has_data = http_client::curl::HttpClientTestPeer::AddSessions(client); - http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + http_client::curl::HttpClientTestPeer::AllowAdds(client); // The removal the finish queues is the reason this has to answer true: the worker checks it // after it has already run doRemoveSessions for this round, and nothing else would drain it. @@ -1294,7 +1304,7 @@ TEST_F(BasicCurlHttpTests, FinishFromAnotherThreadWaitsForTheTerminalEvent) ASSERT_EQ(CURLE_OK, curl_session->GetOperation()->SendAsync( curl_session.get(), [](curl::HttpOperation & /* operation */) {})); - CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RefuseAdds(client); std::atomic finish_returned{false}; std::thread finisher([&curl_session, &finish_returned]() { @@ -1321,7 +1331,7 @@ TEST_F(BasicCurlHttpTests, FinishFromAnotherThreadWaitsForTheTerminalEvent) }); http_client::curl::HttpClientTestPeer::AddSessions(client); - http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + http_client::curl::HttpClientTestPeer::AllowAdds(client); watcher.join(); finisher.join(); From f317b9096bdffe0aea7899609448aab493a15d6a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:12:32 +0000 Subject: [PATCH 21/23] [CHORE] Give the callback scope the members clang-tidy asks a scope guard for The move operations are deleted alongside the copies, since a scope that moved would leave the thread's stack pointing at an object that is no longer the innermost one, and previous_ takes its value where it is declared rather than in the constructor list. Both are what the checks name, and the file now carries one warning fewer than main rather than one more. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_operation_curl.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 196fb0453b..d035ae9298 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -403,8 +403,7 @@ namespace class CallbackScope { public: - explicit CallbackScope(const HttpOperation *operation) noexcept - : operation_{operation}, previous_{current_} + explicit CallbackScope(const HttpOperation *operation) noexcept : operation_{operation} { current_ = this; } @@ -412,7 +411,9 @@ class CallbackScope ~CallbackScope() { current_ = previous_; } CallbackScope(const CallbackScope &) = delete; + CallbackScope(CallbackScope &&) = delete; CallbackScope &operator=(const CallbackScope &) = delete; + CallbackScope &operator=(CallbackScope &&) = delete; static bool InsideCallbackFor(const HttpOperation *operation) noexcept { @@ -427,10 +428,11 @@ class CallbackScope } private: - const HttpOperation *operation_; - const CallbackScope *previous_; - static thread_local const CallbackScope *current_; + + const HttpOperation *operation_; + // Read before the constructor body replaces it, which is what makes the stack a stack. + const CallbackScope *previous_ = current_; }; thread_local const CallbackScope *CallbackScope::current_ = nullptr; From 828b32224ef68834dee23cee91d6b2cc0006c148 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:38:10 +0000 Subject: [PATCH 22/23] [CHORE] Say in the changelog that the unsynchronized callback marker is gone AsyncData::callback_thread is on main today, read and written from two threads with nothing ordering them and with both accessors marked no-thread-sanitizer, so removing it is a fix a user of the released client gets rather than something internal to this branch. The use after free this branch also fixes is not listed, because the code it lives in does not exist outside this branch and a changelog entry would claim a repair to something that never shipped. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b4c07f21f..085c8ff3e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,10 @@ Increment the: including the ones the IO thread delivers, instead of waiting on a completion only that thread goes on to publish ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) +* [BUG] Decide whether a curl thread is inside a callback without an + unsynchronized member, which was read and written from two threads behind a + sanitizer suppression + ([#4395](https://github.com/open-telemetry/opentelemetry-cpp/pull/4395)) * [CODE HEALTH] Enable clang-tidy `modernize-deprecated-headers` and replace deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents From 6bb00bb671fd2758ec87f956635a52a329c5e8ad Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 31 Aug 2026 10:36:05 +0000 Subject: [PATCH 23/23] [BUG] Settle a refused session before reporting it doAddSessions logged the refusal, then read GetOperation(), then finished it. The log goes through the global handler, which is replaceable application code and is not inside a callback scope for that operation, so a handler answering with Session::FinishSession() reached HttpOperation::Finish(). That is not InsideCallbackFor(this), so it waits on a promise only the FinishUnscheduled below it fulfils, on the same thread. Reading the operation after the log was the other half. A handler calling SendRequest() from there swaps the operation, and the loop then finishes the new one and leaves the refused one unsettled. FinishUnscheduled dispatches its event before it cleans up, and a handler calling FinishSession() from that event is inside a callback for the operation, so Finish() returns instead of waiting. Reporting after it is what puts the log handler inside the same protection. ARejectedSessionIsSettledBeforeItIsReported installs a global log handler that records whether the terminal event has arrived by the time the refusal is logged, and restores the previous handler and level through a scope guard. It passes with this change and fails at the ordering assertion with the report moved back above the settlement. The first version of that assertion asked only whether the handler had seen any event. That is true in both orders, because SendAsync dispatches Created during setup, and the case passed with the defect put back. It asks for CreateFailed now. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/src/http/client/curl/http_client_curl.cc | 6 +- ext/test/http/curl_http_test.cc | 114 ++++++++++++++++++- 2 files changed, 118 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 1161732d26..f4ff263832 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -784,7 +784,6 @@ bool HttpClient::doAddSessions() for (auto &rejected : rejected_by_multi) { const char *reason = curl_multi_strerror(rejected.second); - OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_add_handle failed: " << reason); // Told the same way as a session this client never registered, because it is the same thing // from the handler's side: nothing is going to run this request. @@ -793,6 +792,11 @@ bool HttpClient::doAddSessions() { operation->FinishUnscheduled(reason); } + + // Settled first. A log handler is replaceable application code, and one that calls + // FinishSession() from here would otherwise wait on a promise only the line above fulfils, + // on this thread. + OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_multi_add_handle failed: " << reason); } // Finishing a rejected session queues its removal, and the loop's idle check has already run diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 15c9143c71..965b107b56 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -13,13 +13,13 @@ # include #endif // ENABLE_OTLP_COMPRESSION_PREVIEW +#include #include #include #include #include #include #include -#include #include #include #include @@ -33,13 +33,16 @@ #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}; namespace curl = opentelemetry::ext::http::client::curl; namespace http_client = opentelemetry::ext::http::client; +namespace sdk_common = opentelemetry::sdk::common; namespace nostd = opentelemetry::nostd; OPENTELEMETRY_BEGIN_NAMESPACE @@ -783,6 +786,115 @@ TEST_F(BasicCurlHttpTests, AnUnscheduledSessionTellsAHandlerNothingElseHolds) http_client::curl::HttpClientTestPeer::RemoveSessions(client); } +// Restores the global log handler and level whatever the case does, since both are process wide +// and gtest runs one process per case only under CTest. +class ScopedLogHandler +{ +public: + explicit ScopedLogHandler(const nostd::shared_ptr &handler) + : previous_handler_(sdk_common::internal_log::GlobalLogHandler::GetLogHandler()), + previous_level_(sdk_common::internal_log::GlobalLogHandler::GetLogLevel()) + { + sdk_common::internal_log::GlobalLogHandler::SetLogHandler(handler); + + // Error rather than Debug: it is the level the refusal is written at, and anything wider + // hands this observer unrelated lines from the same path. + sdk_common::internal_log::GlobalLogHandler::SetLogLevel( + sdk_common::internal_log::LogLevel::Error); + } + + ~ScopedLogHandler() + { + sdk_common::internal_log::GlobalLogHandler::SetLogHandler(previous_handler_); + sdk_common::internal_log::GlobalLogHandler::SetLogLevel(previous_level_); + } + + ScopedLogHandler(const ScopedLogHandler &) = delete; + ScopedLogHandler(ScopedLogHandler &&) = delete; + ScopedLogHandler &operator=(const ScopedLogHandler &) = delete; + ScopedLogHandler &operator=(ScopedLogHandler &&) = delete; + +private: + nostd::shared_ptr previous_handler_; + sdk_common::internal_log::LogLevel previous_level_; +}; + +// Runs whatever the case gives it, on the thread that wrote the log line. +class ObservingLogHandler : public sdk_common::internal_log::LogHandler +{ +public: + explicit ObservingLogHandler(std::function observe) : observe_(std::move(observe)) {} + + void Handle(sdk_common::internal_log::LogLevel /* level */, + const char * /* file */, + int /* line */, + const char * /* msg */, + const sdk_common::AttributeMap & /* attributes */) noexcept override + { + observe_(); + } + +private: + std::function observe_; +}; + +// The refusal is reported through the global log handler, which is application code that can call +// back into this client. Reporting before the operation is settled leaves a handler that answers +// with FinishSession() waiting on a promise only this thread can fulfil. +TEST_F(BasicCurlHttpTests, ARejectedSessionIsSettledBeforeItIsReported) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + auto curl_session = std::static_pointer_cast(session); + + auto handler = std::make_shared(); + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::RetryPolicy no_retry{}; + http_client::Compression compression = http_client::Compression::kNone; + + curl_session->GetOperation().reset(new curl::HttpOperation( + http_client::Method::Get, "http://127.0.0.1:19000/get/", no_ssl, handler.get(), headers, body, + compression, false, curl::kDefaultHttpConnTimeout, false, false, no_retry)); + + ASSERT_EQ(CURLE_OK, curl_session->GetOperation()->SendAsync( + curl_session.get(), [](curl::HttpOperation & /* operation */) {})); + + std::atomic log_calls{0}; + std::atomic settled_when_logged{0}; + auto observer = nostd::shared_ptr( + new ObservingLogHandler([&handler, &log_calls, &settled_when_logged]() { + log_calls.fetch_add(1, std::memory_order_release); + + // The terminal event specifically. Created arrives during setup, so "the handler has + // seen something" is true in either order. + const std::vector states = handler->States(); + if (std::find(states.begin(), states.end(), http_client::SessionState::CreateFailed) != + states.end()) + { + settled_when_logged.fetch_add(1, std::memory_order_release); + } + })); + + { + ScopedLogHandler scoped{observer}; + http_client::curl::HttpClientTestPeer::RefuseAdds(client); + http_client::curl::HttpClientTestPeer::AddSessions(client); + http_client::curl::HttpClientTestPeer::AllowAdds(client); + } + + ASSERT_EQ(1, log_calls.load(std::memory_order_acquire)) + << "the refusal was not reported once, so this case is not observing what it says it is"; + EXPECT_EQ(1, settled_when_logged.load(std::memory_order_acquire)) + << "the refusal was reported while the operation was still unsettled, so a log handler " + << "calling FinishSession() from here would wait on a promise only this thread fulfils"; + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); +} + // The third way an operation ends up with nothing to run it. SendAsync does the whole async // setup and Session::SendRequest is what spawns the worker, so the add runs here on one thread // against a multi handle that refuses it, which is what a multi handle that failed to initialize