Skip to content

Commit 7714cb9

Browse files
committed
[BUG] Record what the multi handle accepted instead of reading it back
curl_multi_remove_handle reports whether that call succeeded, not whether the multi handle was holding the easy handle. libcurl 8.10 and 8.11 refuse a handle the multi handle does not hold, and a multi handle that failed to initialize refuses every one, so reading the refusal as ownership dropped the record: the easy handle, the header list it points at and the session it names went with it, while the message loop still reads that session back out of CURLOPT_PRIVATE. The client now records an easy handle when curl_multi_add_handle accepts it, asks for no removal of one that was never added, and keeps the whole record, session included, when libcurl will not give a handle back. Kept handles are freed once curl_multi_cleanup has detached them. The queue holds one record per easy handle rather than one per session, so a session that starts another request no longer displaces the handle its previous request left behind, and the destructor releases what the background thread did not get to. The retry path goes through the same accounting, and reports a re-arm that libcurl would not accept rather than dropping the session from the queue in silence. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent dd2ca47 commit 7714cb9

4 files changed

Lines changed: 349 additions & 43 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ Increment the:
5757
`stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents
5858
([#4349](https://github.com/open-telemetry/opentelemetry-cpp/pull/4349))
5959
* [BUG] Remove a curl easy handle from the multi handle before freeing that
60-
handle and the header list it points at
60+
handle and the header list it points at, and keep both when libcurl will not
61+
take the handle back
6162
[#4391](https://github.com/open-telemetry/opentelemetry-cpp/issues/4391)
6263

6364
* [CONFIGURATION] Add SDK component builder interfaces to the registry

‎ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,9 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient
366366
bool doRetrySessions(bool report_all);
367367
void resetMultiHandle();
368368

369+
bool detachHandle(CURL *easy_handle);
370+
void releaseQuarantinedHandles();
371+
369372
std::mutex multi_handle_m_;
370373
CURLM *multi_handle_;
371374
std::atomic<uint64_t> next_session_id_{0};
@@ -376,8 +379,32 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient
376379
std::unordered_map<uint64_t, std::shared_ptr<Session>> sessions_;
377380
std::unordered_set<uint64_t> pending_to_add_session_ids_;
378381
std::unordered_map<uint64_t, std::shared_ptr<Session>> pending_to_abort_sessions_;
379-
std::unordered_map<uint64_t, HttpCurlEasyResource> pending_to_remove_session_handles_;
380-
std::list<std::shared_ptr<Session>> pending_to_remove_sessions_;
382+
// One easy handle on its way back to libcurl. There is one of these per handle rather than
383+
// per session, because a session that starts another request hands over a second handle while
384+
// the first is still queued, and one entry per session would drop the first one, leaving its
385+
// easy handle and header list with no owner. The session is only filled in when the handle
386+
// has to be kept, and names what the handle still points at.
387+
struct PendingCurlRemoval
388+
{
389+
uint64_t session_id;
390+
HttpCurlEasyResource resource;
391+
std::shared_ptr<Session> owner;
392+
};
393+
394+
std::list<PendingCurlRemoval> pending_to_remove_session_handles_;
395+
std::unordered_map<uint64_t, std::shared_ptr<Session>> pending_to_remove_sessions_;
396+
397+
// Which easy handles the multi handle holds, recorded when curl_multi_add_handle accepts one.
398+
// What curl_multi_remove_handle returns cannot answer that question afterwards: it reports
399+
// whether the removal succeeded, and libcurl 8.10 and 8.11 reject a handle the multi handle
400+
// does not hold where every other version accepts it. Background thread only.
401+
std::unordered_set<CURL *> attached_handles_;
402+
403+
// Handles libcurl would not give back. A transfer may still be running on one, and its
404+
// CURLOPT_PRIVATE names the session, so freeing either would leave libcurl and this client
405+
// reading storage that has been released. Both are held until curl_multi_cleanup detaches
406+
// the handle, which is where they are freed.
407+
std::list<PendingCurlRemoval> quarantined_handles_;
381408
std::deque<std::shared_ptr<Session>> pending_to_retry_sessions_;
382409

383410
std::mutex background_thread_m_;

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

Lines changed: 158 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,26 @@ namespace client
4747
namespace curl
4848
{
4949

50+
namespace
51+
{
52+
// The easy handle goes first. libcurl does not copy the header list, so it is in use for as long
53+
// as anything is still transferring on the handle that points at it.
54+
void ReleaseCurlResource(HttpCurlEasyResource &resource) noexcept
55+
{
56+
if (nullptr != resource.easy_handle)
57+
{
58+
curl_easy_cleanup(resource.easy_handle);
59+
resource.easy_handle = nullptr;
60+
}
61+
62+
if (nullptr != resource.headers_chunk)
63+
{
64+
curl_slist_free_all(resource.headers_chunk);
65+
resource.headers_chunk = nullptr;
66+
}
67+
}
68+
} // namespace
69+
5070
HttpCurlGlobalInitializer::HttpCurlGlobalInitializer()
5171
{
5272
curl_global_init(CURL_GLOBAL_ALL);
@@ -318,7 +338,31 @@ HttpClient::~HttpClient()
318338
{
319339
std::lock_guard<std::mutex> lock_guard{multi_handle_m_};
320340
curl_multi_cleanup(multi_handle_);
341+
342+
// Nothing is attached to a multi handle that no longer exists, and clearing the pointer
343+
// keeps anything released below from reaching for the one just destroyed.
344+
attached_handles_.clear();
345+
multi_handle_ = nullptr;
346+
}
347+
348+
// The background thread has stopped, so nothing else is coming back for what it left behind.
349+
// Sessions go first, because releasing one can hand over one last easy handle, and they are
350+
// released outside the lock because that hand over takes it.
351+
releaseQuarantinedHandles();
352+
{
353+
std::unordered_map<uint64_t, std::shared_ptr<Session>> pending_to_remove_sessions;
354+
{
355+
std::lock_guard<std::recursive_mutex> session_id_lock_guard{session_ids_m_};
356+
pending_to_remove_sessions_.swap(pending_to_remove_sessions);
357+
}
358+
}
359+
360+
std::lock_guard<std::recursive_mutex> session_id_lock_guard{session_ids_m_};
361+
for (auto &pending : pending_to_remove_session_handles_)
362+
{
363+
ReleaseCurlResource(pending.resource);
321364
}
365+
pending_to_remove_session_handles_.clear();
322366
}
323367

324368
std::shared_ptr<opentelemetry::ext::http::client::Session> HttpClient::CreateSession(
@@ -396,10 +440,19 @@ void HttpClient::CleanupSession(uint64_t session_id)
396440

397441
if (session)
398442
{
399-
if (pending_to_remove_session_handles_.end() !=
400-
pending_to_remove_session_handles_.find(session_id))
443+
bool has_pending_handle = false;
444+
for (const auto &pending : pending_to_remove_session_handles_)
401445
{
402-
pending_to_remove_sessions_.emplace_back(std::move(session));
446+
if (pending.session_id == session_id)
447+
{
448+
has_pending_handle = true;
449+
break;
450+
}
451+
}
452+
453+
if (has_pending_handle)
454+
{
455+
pending_to_remove_sessions_.emplace(session_id, std::move(session));
403456
}
404457
else if (session->IsSessionActive() && session->GetOperation())
405458
{
@@ -643,7 +696,6 @@ void HttpClient::ScheduleAddSession(uint64_t session_id)
643696
{
644697
std::lock_guard<std::recursive_mutex> lock_guard{session_ids_m_};
645698
pending_to_add_session_ids_.insert(session_id);
646-
pending_to_remove_session_handles_.erase(session_id);
647699
pending_to_abort_sessions_.erase(session_id);
648700
}
649701

@@ -678,7 +730,8 @@ void HttpClient::ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource
678730
{
679731
std::lock_guard<std::recursive_mutex> lock_guard{session_ids_m_};
680732
pending_to_add_session_ids_.erase(session_id);
681-
pending_to_remove_session_handles_[session_id] = std::move(resource);
733+
pending_to_remove_session_handles_.push_back(
734+
PendingCurlRemoval{session_id, std::move(resource), nullptr});
682735
}
683736

684737
wakeupBackgroundThread();
@@ -749,7 +802,18 @@ bool HttpClient::doAddSessions()
749802
continue;
750803
}
751804

752-
curl_multi_add_handle(multi_handle_, easy_handle);
805+
const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle);
806+
if (CURLM_OK != rc)
807+
{
808+
// The request never starts. Leaving the handle unrecorded is what stops the release path
809+
// from asking the multi handle to give back something it was never given.
810+
OTEL_INTERNAL_LOG_ERROR(
811+
"[HTTP Client Curl] curl_multi_add_handle failed, this request will not be sent: "
812+
<< curl_multi_strerror(rc));
813+
continue;
814+
}
815+
816+
attached_handles_.insert(easy_handle);
753817
has_data = true;
754818
}
755819

@@ -781,10 +845,10 @@ bool HttpClient::doAbortSessions()
781845
// FinishOperation queued the easy handle for removal, and that handle still holds
782846
// CURLOPT_PRIVATE and the callback data pointing into this session and its operation.
783847
// doRemoveSessions cannot find the session to hold, because aborting took it out of
784-
// sessions_, so hand it the owner directly: it keeps one until the handle is freed.
848+
// sessions_, so hand it over under the id it will look the handle up by.
785849
{
786850
std::lock_guard<std::recursive_mutex> session_id_lock_guard{session_ids_m_};
787-
pending_to_remove_sessions_.emplace_back(std::move(session.second));
851+
pending_to_remove_sessions_.emplace(session.first, std::move(session.second));
788852
}
789853
}
790854
return has_data;
@@ -796,8 +860,8 @@ bool HttpClient::doRemoveSessions()
796860
bool should_continue{false};
797861
do
798862
{
799-
std::unordered_map<uint64_t, HttpCurlEasyResource> pending_to_remove_session_handles;
800-
std::list<std::shared_ptr<Session>> pending_to_remove_sessions;
863+
std::list<PendingCurlRemoval> pending_to_remove_session_handles;
864+
std::unordered_map<uint64_t, std::shared_ptr<Session>> pending_to_remove_sessions;
801865
{
802866
std::lock_guard<std::recursive_mutex> session_id_lock_guard{session_ids_m_};
803867
pending_to_remove_session_handles_.swap(pending_to_remove_session_handles);
@@ -809,51 +873,46 @@ bool HttpClient::doRemoveSessions()
809873
std::lock_guard<std::mutex> session_lock_guard{sessions_m_};
810874
for (auto &removing_handle : pending_to_remove_session_handles)
811875
{
812-
auto session = sessions_.find(removing_handle.first);
876+
auto session = sessions_.find(removing_handle.session_id);
813877
if (session != sessions_.end())
814878
{
815-
pending_to_remove_sessions.emplace_back(std::move(session->second));
879+
pending_to_remove_sessions.emplace(session->first, std::move(session->second));
816880
sessions_.erase(session);
817881
}
818882
}
819883
}
820884

821885
for (auto &removing_handle : pending_to_remove_session_handles)
822886
{
823-
auto &resource = removing_handle.second;
887+
auto &resource = removing_handle.resource;
824888
if (nullptr == resource.easy_handle)
825889
{
826890
continue;
827891
}
828892

829-
// Remove before cleanup: libcurl forbids freeing an easy handle, or the header list it
830-
// points at, while the multi handle still owns it. A handle never added reports CURLM_OK.
831-
const CURLMcode rc = curl_multi_remove_handle(multi_handle_, resource.easy_handle);
832-
if (CURLM_OK != rc)
893+
// Give the handle back first. libcurl does not copy the header list, so it stays in use
894+
// for as long as the multi handle has a transfer running on this handle.
895+
if (!detachHandle(resource.easy_handle))
833896
{
834-
// The multi handle may still own it. Leaking one easy handle is the better half of
835-
// this trade: freeing it here would corrupt whatever still holds it.
836-
OTEL_INTERNAL_LOG_ERROR(
837-
"[HTTP Client Curl] curl_multi_remove_handle failed, leaving "
838-
"the handle alone: "
839-
<< curl_multi_strerror(rc));
840-
continue;
841-
}
897+
// Still held, so a transfer may still be running on it and its CURLOPT_PRIVATE still
898+
// names this session. Keep the whole record instead of losing track of the handle.
899+
auto owner = pending_to_remove_sessions.find(removing_handle.session_id);
900+
if (owner != pending_to_remove_sessions.end())
901+
{
902+
removing_handle.owner = owner->second;
903+
}
842904

843-
if (nullptr != resource.headers_chunk)
844-
{
845-
curl_slist_free_all(resource.headers_chunk);
846-
resource.headers_chunk = nullptr;
905+
quarantined_handles_.push_back(std::move(removing_handle));
906+
continue;
847907
}
848908

849-
curl_easy_cleanup(resource.easy_handle);
850-
resource.easy_handle = nullptr;
909+
ReleaseCurlResource(resource);
851910
}
852911

853912
for (auto &removing_session : pending_to_remove_sessions)
854913
{
855914
// This operation may add more pending_to_remove_session_handles
856-
removing_session->FinishOperation();
915+
removing_session.second->FinishOperation();
857916
}
858917

859918
should_continue =
@@ -890,8 +949,25 @@ bool HttpClient::doRetrySessions(bool report_all)
890949
else if (operation->NextRetryTime() < now)
891950
{
892951
auto easy_handle = operation->GetCurlEasyHandle();
893-
curl_multi_remove_handle(multi_handle_, easy_handle);
894-
curl_multi_add_handle(multi_handle_, easy_handle);
952+
953+
// If the multi handle will not give it back, the add below is answered with
954+
// CURLM_ADDED_ALREADY and the transfer carries on where it was.
955+
detachHandle(easy_handle);
956+
957+
const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle);
958+
if (CURLM_OK == rc)
959+
{
960+
attached_handles_.insert(easy_handle);
961+
}
962+
else
963+
{
964+
// The session leaves the retry queue either way, so say so rather than let a request
965+
// that was never re-armed look like one that was.
966+
OTEL_INTERNAL_LOG_ERROR(
967+
"[HTTP Client Curl] curl_multi_add_handle failed, this retry will not be sent: "
968+
<< curl_multi_strerror(rc));
969+
}
970+
895971
retry_it = pending_to_retry_sessions_.erase(retry_it);
896972
has_data = true;
897973
}
@@ -938,12 +1014,55 @@ void HttpClient::resetMultiHandle()
9381014

9391015
doRemoveSessions();
9401016

941-
// We will modify the multi_handle_, so we need to lock it
942-
std::lock_guard<std::mutex> lock_guard{multi_handle_m_};
943-
curl_multi_cleanup(multi_handle_);
1017+
{
1018+
// We will modify the multi_handle_, so we need to lock it
1019+
std::lock_guard<std::mutex> lock_guard{multi_handle_m_};
1020+
curl_multi_cleanup(multi_handle_);
9441021

945-
// Create a another multi handle to continue pending sessions
946-
multi_handle_ = curl_multi_init();
1022+
// The cleanup detached everything the old handle held, so nothing is attached any more and
1023+
// nothing may be offered back to the handle created below.
1024+
attached_handles_.clear();
1025+
1026+
// Create a another multi handle to continue pending sessions
1027+
multi_handle_ = curl_multi_init();
1028+
}
1029+
1030+
releaseQuarantinedHandles();
1031+
}
1032+
1033+
bool HttpClient::detachHandle(CURL *easy_handle)
1034+
{
1035+
if (attached_handles_.end() == attached_handles_.find(easy_handle))
1036+
{
1037+
// The multi handle was never given this one, so it has nothing to give back. Asking libcurl
1038+
// is not a way to find that out: 8.10 and 8.11 reject a handle the multi handle does not
1039+
// hold, and a multi handle that failed to initialize rejects every handle.
1040+
return true;
1041+
}
1042+
1043+
const CURLMcode rc = curl_multi_remove_handle(multi_handle_, easy_handle);
1044+
if (CURLM_OK != rc)
1045+
{
1046+
OTEL_INTERNAL_LOG_ERROR(
1047+
"[HTTP Client Curl] curl_multi_remove_handle failed, the handle stays "
1048+
"with the multi handle: "
1049+
<< curl_multi_strerror(rc));
1050+
return false;
1051+
}
1052+
1053+
attached_handles_.erase(easy_handle);
1054+
return true;
1055+
}
1056+
1057+
void HttpClient::releaseQuarantinedHandles()
1058+
{
1059+
std::list<PendingCurlRemoval> quarantined_handles;
1060+
quarantined_handles.swap(quarantined_handles_);
1061+
1062+
for (auto &quarantined : quarantined_handles)
1063+
{
1064+
ReleaseCurlResource(quarantined.resource);
1065+
}
9471066
}
9481067

9491068
} // namespace curl

0 commit comments

Comments
 (0)