[BUG] Scope the session lock to the snapshot in resetMultiHandle

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

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();

ext/src/http/client/curl/http_client_curl.cc

@@ -884,8 +884,12 @@ bool HttpClient::doRetrySessions(bool /* report_all */)
 void HttpClient::resetMultiHandle()
 {
   std::list<std::shared_ptr<Session>> sessions;
-  std::lock_guard<std::mutex> 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<std::mutex> session_lock_guard{sessions_m_};
     std::lock_guard<std::recursive_mutex> session_id_lock_guard{session_ids_m_};
     for (auto &session : sessions_)
     {

ext/test/http/curl_http_test.cc

@@ -33,13 +33,36 @@
 #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};
 
 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<http_client::curl::HttpClient>();
+
+  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();