diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f427215d1..865a2f680b 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 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..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}; @@ -40,6 +41,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 +616,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();