From cd09784c582fd043a62c8a6d5f5d473cf7167cec Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:22:30 +0000 Subject: [PATCH 1/3] [BUG] Scope the session lock to the snapshot in resetMultiHandle resetMultiHandle held sessions_m_ for the whole function, then called CancelSession and doRemoveSessions, both of which take it again on the same thread. It is a plain std::mutex, so the IO thread stopped there and never came back, on the path that recovers from a curl_multi_perform error. The lock only ever guarded the snapshot, so it is scoped to that. Making the mutex recursive would have worked and been wrong: FinishOperation runs the caller's handler through Cleanup, and sessions_m_ was never meant to cover user code. A test peer reaches resetMultiHandle directly, since curl_multi_perform cannot be made to fail from a test and a production failpoint is not worth carrying. One registered session is enough to reach both re-lock sites. Fixes #4389. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/client/curl/http_client_curl.h | 2 ++ ext/src/http/client/curl/http_client_curl.cc | 6 +++- ext/test/http/curl_http_test.cc | 36 +++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) 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 3539297145..9a09fac9aa 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 @@ -357,6 +357,8 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient void WaitBackgroundThreadExit(); private: + friend class HttpClientTestPeer; + void wakeupBackgroundThread(); bool doAddSessions(); bool doAbortSessions(); diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index dcacc66151..76f67fcd90 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -884,8 +884,12 @@ bool HttpClient::doRetrySessions(bool /* report_all */) void HttpClient::resetMultiHandle() { std::list> sessions; - std::lock_guard session_lock_guard{sessions_m_}; { + // Only the snapshot needs these. CancelSession and doRemoveSessions below both take + // sessions_m_ again, and it is not recursive, so holding it across them stops the IO + // thread here for good. Cleanup also runs the caller's handler, which this lock was + // never meant to cover. + std::lock_guard session_lock_guard{sessions_m_}; std::lock_guard session_id_lock_guard{session_ids_m_}; for (auto &session : sessions_) { diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index e1b180e8fe..be1d1f7344 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -40,6 +40,28 @@ namespace curl = opentelemetry::ext::http::client::curl; namespace http_client = opentelemetry::ext::http::client; namespace nostd = opentelemetry::nostd; +OPENTELEMETRY_BEGIN_NAMESPACE +namespace ext +{ +namespace http +{ +namespace client +{ +namespace curl +{ +// resetMultiHandle only runs when curl_multi_perform fails, which a test cannot provoke, so +// the case below reaches it directly. See #4389. +class HttpClientTestPeer +{ +public: + static void ResetMultiHandle(HttpClient &client) { client.resetMultiHandle(); } +}; +} // namespace curl +} // namespace client +} // namespace http +} // namespace ext +OPENTELEMETRY_END_NAMESPACE + namespace { @@ -593,6 +615,20 @@ TEST_F(BasicCurlHttpTests, RetryJitterIsNotSharedAcrossThreads) drawing_second.join(); } +// resetMultiHandle used to hold sessions_m_ across CancelSession and doRemoveSessions, which +// take it again on the same thread. One registered session is enough to reach both. +TEST_F(BasicCurlHttpTests, ResetMultiHandleWithASessionDoesNotDeadlock) +{ + auto client = std::make_shared(); + + auto session = client->CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + + http_client::curl::HttpClientTestPeer::ResetMultiHandle(*client); + + client->FinishAllSessions(); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear(); From 3a0fae3e80573264d613e43839ca0612c599d24a Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 10 Aug 2026 09:25:18 +0000 Subject: [PATCH 2/3] [BUG] Scope the session lock to the snapshot in resetMultiHandle Add the CHANGELOG entry now that the pull request has a number. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bca7b4b1c..5825967d67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,9 @@ Increment the: * [BUG] Draw the curl retry jitter from a per thread generator instead of one shared across HTTP client threads ([#4399](https://github.com/open-telemetry/opentelemetry-cpp/pull/4399)) +* [BUG] Stop the curl IO thread deadlocking on itself while recovering from a + multi handle error + ([#4394](https://github.com/open-telemetry/opentelemetry-cpp/pull/4394)) * [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 b32c8cf737bb1c4c06d7d0f20a0ca3e6656e3bc3 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:41:21 +0000 Subject: [PATCH 3/3] [BUG] Scope the session lock to the snapshot in resetMultiHandle Include opentelemetry/version.h directly in the curl test. The test peer this change adds is wrapped in OPENTELEMETRY_BEGIN_NAMESPACE, and the file had been picking that macro up transitively, which include-what-you-use rejects. 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 be1d1f7344..4f7cc006ec 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -33,6 +33,7 @@ #include "opentelemetry/ext/http/server/http_server.h" #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/version.h" constexpr int HTTP_PORT{19000};