diff --git a/CHANGELOG.md b/CHANGELOG.md index fc64aaef2..0f7d302b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,10 @@ Increment the: deprecated C headers (`stdint.h`, `stddef.h`, `stdlib.h`, `string.h`, `stdio.h`, `ctype.h`, `limits.h`, `assert.h`) with their C++ equivalents ([#4349](https://github.com/open-telemetry/opentelemetry-cpp/pull/4349)) +* [BUG] Remove a curl easy handle from the multi handle before freeing that + handle and the header list it points at, and keep both when libcurl will not + take the handle back + [#4391](https://github.com/open-telemetry/opentelemetry-cpp/issues/4391) * [CONFIGURATION] Add SDK component builder interfaces to the registry [#4358](https://github.com/open-telemetry/opentelemetry-cpp/issues/4358) 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 9a09fac9a..6837ca147 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 @@ -366,6 +366,9 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doRetrySessions(bool report_all); void resetMultiHandle(); + bool detachHandle(CURL *easy_handle); + void releaseQuarantinedHandles(); + std::mutex multi_handle_m_; CURLM *multi_handle_; std::atomic next_session_id_{0}; @@ -376,8 +379,33 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unordered_map> sessions_; std::unordered_set pending_to_add_session_ids_; std::unordered_map> pending_to_abort_sessions_; - std::unordered_map pending_to_remove_session_handles_; - std::list> pending_to_remove_sessions_; + // One easy handle on its way back to libcurl, with the session it still names through + // CURLOPT_PRIVATE. The session is taken when the record is made, so that nothing has to find + // it again later against a sessions_ the calling thread is free to change in between. + // + // There is one of these per handle rather than per session, because a session that starts + // another request hands over a second handle while the first is still queued, and one entry + // per session would drop the first one, leaving its easy handle and header list with no owner. + struct PendingCurlRemoval + { + uint64_t session_id; + HttpCurlEasyResource resource; + std::shared_ptr owner; + }; + + std::list pending_to_remove_session_handles_; + + // Which easy handles the multi handle holds, recorded when curl_multi_add_handle accepts one. + // What curl_multi_remove_handle returns cannot answer that question afterwards: it reports + // whether the removal succeeded, and libcurl 8.10 and 8.11 reject a handle the multi handle + // does not hold where every other version accepts it. Background thread only. + std::unordered_set attached_handles_; + + // Handles libcurl would not give back. A transfer may still be running on one, and its + // CURLOPT_PRIVATE names the session, so freeing either would leave libcurl and this client + // reading storage that has been released. Both are held until curl_multi_cleanup detaches + // the handle, which is where they are freed. + std::list quarantined_handles_; std::deque> pending_to_retry_sessions_; std::mutex background_thread_m_; 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 c57309ccd..7448b7107 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 @@ -285,7 +285,12 @@ class HttpOperation * * @param code CURLcode */ - void PerformCurlMessage(CURLcode code); + /** + * Read one completed curl message. + * @return true when the operation has been rewound for another attempt, and the caller is to + * take it on. False when the message was not this operation's to read, or it is finished. + */ + bool PerformCurlMessage(CURLcode code); inline CURL *GetCurlEasyHandle() noexcept { return curl_resource_.easy_handle; } diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 76f67fcd9..cd27b6f61 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 @@ -47,6 +47,35 @@ namespace client namespace curl { +namespace +{ +// Only ever called with the handle detached, either because it has been given back or because +// the multi handle that held it has been cleaned up. +// +// The reset clears the callbacks and the pointers they are given, so that freeing the handle +// cannot reach the operation or the event handler the caller has let go of by then. The header +// list goes next, because libcurl does not copy it and the reset is what stops it being read. +void ReleaseCurlResource(HttpCurlEasyResource &resource) noexcept +{ + if (nullptr != resource.easy_handle) + { + curl_easy_reset(resource.easy_handle); + } + + if (nullptr != resource.headers_chunk) + { + curl_slist_free_all(resource.headers_chunk); + resource.headers_chunk = nullptr; + } + + if (nullptr != resource.easy_handle) + { + curl_easy_cleanup(resource.easy_handle); + resource.easy_handle = nullptr; + } +} +} // namespace + HttpCurlGlobalInitializer::HttpCurlGlobalInitializer() { curl_global_init(CURL_GLOBAL_ALL); @@ -318,6 +347,36 @@ HttpClient::~HttpClient() { std::lock_guard lock_guard{multi_handle_m_}; curl_multi_cleanup(multi_handle_); + + // Nothing is attached to a multi handle that no longer exists, and clearing the pointer + // keeps anything released below from reaching for the one just destroyed. + attached_handles_.clear(); + multi_handle_ = nullptr; + } + + // The background thread has stopped, so nothing else is coming back for what it left behind. + // A batch at a time and with no lock held: a record owns the session it names, and letting one + // go runs a destructor that comes back through ScheduleRemoveSession. That can only queue what + // a session still held, and no more sessions are made here, so this ends. + releaseQuarantinedHandles(); + + while (true) + { + std::list pending_to_remove_session_handles; + { + std::lock_guard session_id_lock_guard{session_ids_m_}; + pending_to_remove_session_handles_.swap(pending_to_remove_session_handles); + } + + if (pending_to_remove_session_handles.empty()) + { + break; + } + + for (auto &pending : pending_to_remove_session_handles) + { + ReleaseCurlResource(pending.resource); + } } } @@ -394,19 +453,13 @@ void HttpClient::CleanupSession(uint64_t session_id) std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.erase(session_id); - if (session) + // A handle of this session that is queued for removal already holds the session, so there + // is nothing to decide about one here. + if (session && session->IsSessionActive() && session->GetOperation()) { - if (pending_to_remove_session_handles_.end() != - pending_to_remove_session_handles_.find(session_id)) - { - pending_to_remove_sessions_.emplace_back(std::move(session)); - } - else if (session->IsSessionActive() && session->GetOperation()) - { - // If this session is already running, give it to the background thread for cleanup. - pending_to_abort_sessions_[session_id] = std::move(session); - need_wakeup_background_thread = true; - } + // If this session is already running, give it to the background thread for cleanup. + pending_to_abort_sessions_[session_id] = std::move(session); + need_wakeup_background_thread = true; } } @@ -524,9 +577,11 @@ bool HttpClient::MaybeSpawnBackgroundThread() { // Session can not be destroyed when calling PerformCurlMessage auto hold_session = session->shared_from_this(); - operation->PerformCurlMessage(result); - if (operation->IsRetryable()) + // Reading the message and taking it on for another attempt are one decision. + // Asking again afterwards reads whatever the last message left behind, which for + // an operation that has since been cleaned up is a status from before that. + if (operation->PerformCurlMessage(result)) { self->pending_to_retry_sessions_.push_back(hold_session); } @@ -643,7 +698,6 @@ void HttpClient::ScheduleAddSession(uint64_t session_id) { std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.insert(session_id); - pending_to_remove_session_handles_.erase(session_id); pending_to_abort_sessions_.erase(session_id); } @@ -676,9 +730,23 @@ void HttpClient::ScheduleAbortSession(uint64_t session_id) void HttpClient::ScheduleRemoveSession(uint64_t session_id, HttpCurlEasyResource &&resource) { { - std::lock_guard lock_guard{session_ids_m_}; + // The session is taken here, in the same breath as the record. Looking it up when the record + // is drained instead leaves a gap the calling thread runs through: it returns from + // FinishSession, CleanupSession takes the session out of sessions_, and by then this queue + // has been swapped away, so neither side would be holding a session the handle still names. + std::lock_guard session_lock_guard{sessions_m_}; + std::lock_guard session_id_lock_guard{session_ids_m_}; + + std::shared_ptr owner; + auto session = sessions_.find(session_id); + if (session != sessions_.end()) + { + owner = session->second; + } + pending_to_add_session_ids_.erase(session_id); - pending_to_remove_session_handles_[session_id] = std::move(resource); + pending_to_remove_session_handles_.push_back( + PendingCurlRemoval{session_id, std::move(resource), std::move(owner)}); } wakeupBackgroundThread(); @@ -749,7 +817,18 @@ bool HttpClient::doAddSessions() continue; } - curl_multi_add_handle(multi_handle_, easy_handle); + const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle); + if (CURLM_OK != rc) + { + // The request never starts. Leaving the handle unrecorded is what stops the release path + // from asking the multi handle to give back something it was never given. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_add_handle failed, this request will not be sent: " + << curl_multi_strerror(rc)); + continue; + } + + attached_handles_.insert(easy_handle); has_data = true; } @@ -765,7 +844,7 @@ bool HttpClient::doAbortSessions() } bool has_data = false; - for (const auto &session : pending_to_abort_sessions) + for (auto &session : pending_to_abort_sessions) { if (!session.second) { @@ -777,6 +856,20 @@ bool HttpClient::doAbortSessions() session.second->FinishOperation(); has_data = true; } + + // Aborting took this session out of sessions_ before FinishOperation queued its easy + // handle, so the record could not take the session for itself and is given it here. Nothing + // drains the queue in between: both run on this thread, one after the other. + { + std::lock_guard session_id_lock_guard{session_ids_m_}; + for (auto &pending : pending_to_remove_session_handles_) + { + if (pending.session_id == session.first && !pending.owner) + { + pending.owner = session.second; + } + } + } } return has_data; } @@ -787,12 +880,11 @@ bool HttpClient::doRemoveSessions() bool should_continue{false}; do { - std::unordered_map pending_to_remove_session_handles; - std::list> pending_to_remove_sessions; + std::list pending_to_remove_session_handles; + std::unordered_map> pending_to_remove_sessions; { std::lock_guard session_id_lock_guard{session_ids_m_}; pending_to_remove_session_handles_.swap(pending_to_remove_session_handles); - pending_to_remove_sessions_.swap(pending_to_remove_sessions); } { // If user callback do not call CancelSession or FinishSession, We still need to remove it @@ -800,10 +892,10 @@ bool HttpClient::doRemoveSessions() std::lock_guard session_lock_guard{sessions_m_}; for (auto &removing_handle : pending_to_remove_session_handles) { - auto session = sessions_.find(removing_handle.first); + auto session = sessions_.find(removing_handle.session_id); if (session != sessions_.end()) { - pending_to_remove_sessions.emplace_back(std::move(session->second)); + pending_to_remove_sessions.emplace(session->first, std::move(session->second)); sessions_.erase(session); } } @@ -811,19 +903,30 @@ bool HttpClient::doRemoveSessions() for (auto &removing_handle : pending_to_remove_session_handles) { - if (nullptr != removing_handle.second.headers_chunk) + auto &resource = removing_handle.resource; + if (nullptr == resource.easy_handle) + { + continue; + } + + // Give the handle back first. libcurl does not copy the header list, so it stays in use + // for as long as the multi handle has a transfer running on this handle. + if (!detachHandle(resource.easy_handle)) { - curl_slist_free_all(removing_handle.second.headers_chunk); + // Still held, so a transfer may still be running on it and its CURLOPT_PRIVATE still + // names this session. The record carries that session already, so keeping the record + // keeps both. + quarantined_handles_.push_back(std::move(removing_handle)); + continue; } - curl_multi_remove_handle(multi_handle_, removing_handle.second.easy_handle); - curl_easy_cleanup(removing_handle.second.easy_handle); + ReleaseCurlResource(resource); } for (auto &removing_session : pending_to_remove_sessions) { // This operation may add more pending_to_remove_session_handles - removing_session->FinishOperation(); + removing_session.second->FinishOperation(); } should_continue = @@ -860,8 +963,25 @@ bool HttpClient::doRetrySessions(bool report_all) else if (operation->NextRetryTime() < now) { auto easy_handle = operation->GetCurlEasyHandle(); - curl_multi_remove_handle(multi_handle_, easy_handle); - curl_multi_add_handle(multi_handle_, easy_handle); + + // If the multi handle will not give it back, the add below is answered with + // CURLM_ADDED_ALREADY and the transfer carries on where it was. + detachHandle(easy_handle); + + const CURLMcode rc = curl_multi_add_handle(multi_handle_, easy_handle); + if (CURLM_OK == rc) + { + attached_handles_.insert(easy_handle); + } + else + { + // The session leaves the retry queue either way, so say so rather than let a request + // that was never re-armed look like one that was. + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_add_handle failed, this retry will not be sent: " + << curl_multi_strerror(rc)); + } + retry_it = pending_to_retry_sessions_.erase(retry_it); has_data = true; } @@ -908,12 +1028,55 @@ void HttpClient::resetMultiHandle() doRemoveSessions(); - // We will modify the multi_handle_, so we need to lock it - std::lock_guard lock_guard{multi_handle_m_}; - curl_multi_cleanup(multi_handle_); + { + // We will modify the multi_handle_, so we need to lock it + std::lock_guard lock_guard{multi_handle_m_}; + curl_multi_cleanup(multi_handle_); + + // The cleanup detached everything the old handle held, so nothing is attached any more and + // nothing may be offered back to the handle created below. + attached_handles_.clear(); + + // Create a another multi handle to continue pending sessions + multi_handle_ = curl_multi_init(); + } + + releaseQuarantinedHandles(); +} + +bool HttpClient::detachHandle(CURL *easy_handle) +{ + if (attached_handles_.end() == attached_handles_.find(easy_handle)) + { + // The multi handle was never given this one, so it has nothing to give back. Asking libcurl + // is not a way to find that out: 8.10 and 8.11 reject a handle the multi handle does not + // hold, and a multi handle that failed to initialize rejects every handle. + return true; + } + + const CURLMcode rc = curl_multi_remove_handle(multi_handle_, easy_handle); + if (CURLM_OK != rc) + { + OTEL_INTERNAL_LOG_ERROR( + "[HTTP Client Curl] curl_multi_remove_handle failed, the handle stays " + "with the multi handle: " + << curl_multi_strerror(rc)); + return false; + } - // Create a another multi handle to continue pending sessions - multi_handle_ = curl_multi_init(); + attached_handles_.erase(easy_handle); + return true; +} + +void HttpClient::releaseQuarantinedHandles() +{ + std::list quarantined_handles; + quarantined_handles.swap(quarantined_handles_); + + for (auto &quarantined : quarantined_handles) + { + ReleaseCurlResource(quarantined.resource); + } } } // 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 0f1bda403..8e2614c5f 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -542,15 +542,11 @@ void HttpOperation::Cleanup() // Only cleanup async once even in recursive calls if (async_data_) { - // Just reset and move easy_handle to owner if in async mode + // Hand the easy handle over untouched. It may still be attached to the multi handle, and the + // thread inside curl_multi_perform owns it until the IO thread removes it. Session *session = async_data_->session.exchange(nullptr, std::memory_order_acq_rel); if (session != nullptr) { - if (curl_resource_.easy_handle != nullptr) - { - curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, NULL); - curl_easy_reset(curl_resource_.easy_handle); - } session->GetHttpClient().ScheduleRemoveSession(session->GetSessionId(), std::move(curl_resource_)); } @@ -1523,8 +1519,17 @@ void HttpOperation::Abort() } } -void HttpOperation::PerformCurlMessage(CURLcode code) +bool HttpOperation::PerformCurlMessage(CURLcode code) { + if (is_cleaned_.load(std::memory_order_acquire)) + { + // Some thread has entered Cleanup for this operation, which is as much as the flag says: it + // is taken before the terminal event, the hand over of the easy handle and the completion + // callback. Either way this message is not this operation's to read, and it is not to be + // taken on for another attempt on the strength of what an earlier message left behind. + return false; + } + ++retry_attempts_; last_attempt_time_ = std::chrono::system_clock::now(); last_curl_result_ = code; @@ -1629,7 +1634,10 @@ void HttpOperation::PerformCurlMessage(CURLcode code) { // Cleanup and unbind easy handle from multi handle, and finish callback Cleanup(); + return false; } + + return true; } } // namespace curl diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 90142962d..545fb0b4c 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1,11 +1,11 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #include #include "gtest/gtest.h" #ifdef ENABLE_OTLP_RETRY_PREVIEW -# include # include "gmock/gmock.h" #endif // ENABLE_OTLP_RETRY_PREVIEW @@ -17,12 +17,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include @@ -56,6 +58,54 @@ class HttpClientTestPeer { public: static void ResetMultiHandle(HttpClient &client) { client.resetMultiHandle(); } + + static bool RemoveSessions(HttpClient &client) { return client.doRemoveSessions(); } + + static bool AbortSessions(HttpClient &client) { return client.doAbortSessions(); } + + // What the multi handle holds is recorded when curl_multi_add_handle accepts a handle. The + // cases below have no transfer to accept, so they say what it holds directly. + static void NoteAttached(HttpClient &client, CURL *easy_handle) + { + client.attached_handles_.insert(easy_handle); + } + + static std::size_t AttachedCount(const HttpClient &client) + { + return client.attached_handles_.size(); + } + + static std::size_t PendingRemovalCount(const HttpClient &client) + { + return client.pending_to_remove_session_handles_.size(); + } + + static std::size_t PendingRemovalOwnerCount(const HttpClient &client) + { + std::size_t owned = 0; + for (const auto &pending : client.pending_to_remove_session_handles_) + { + if (pending.owner) + { + ++owned; + } + } + return owned; + } + + static std::size_t QuarantinedCount(const HttpClient &client) + { + return client.quarantined_handles_.size(); + } + + // A multi handle that failed to initialize refuses every removal, which is how the cases below + // reach the path where libcurl will not take a handle back. + static CURLM *ExchangeMultiHandle(HttpClient &client, CURLM *replacement) + { + CURLM *previous = client.multi_handle_; + client.multi_handle_ = replacement; + return previous; + } }; } // namespace curl } // namespace client @@ -108,6 +158,14 @@ class TerminalCountingHandler : public CustomEventHandler void OnEvent(http_client::SessionState state, nostd::string_view reason) noexcept override { + // Counted on its own, and before the chain below rather than inside it: a case can cancel + // from one of these, and two others pin terminal_count_ to an exact value. + if (state == http_client::SessionState::ConnectFailed || + state == http_client::SessionState::SendFailed) + { + failed_count_.fetch_add(1, std::memory_order_release); + } + if (state == http_client::SessionState::Cancelled) { terminal_count_.fetch_add(1, std::memory_order_release); @@ -133,6 +191,7 @@ class TerminalCountingHandler : public CustomEventHandler http_client::SessionState cancel_at_ = http_client::SessionState::Response; std::thread::id cancelled_from_{}; std::atomic terminal_count_{0}; + std::atomic failed_count_{0}; std::atomic cancelled_from_callback_{0}; }; @@ -442,6 +501,32 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations) delete handler; } +// A CA certificate supplied as a string goes to curl as a blob rather than a path. curl copies it +// and does not read it until a handshake, so a plain request reaches the option and no certificate +// has to be valid for this. +TEST_F(BasicCurlHttpTests, ACaCertificateStringIsPassedAsABlob) +{ + RetryEventHandler handler; + http_client::HttpSslOptions ssl_options; + ssl_options.use_ssl = true; + ssl_options.ssl_ca_cert_string = + "-----BEGIN CERTIFICATE-----\nnot a certificate\n-----END CERTIFICATE-----\n"; + http_client::Body body; + http_client::Headers headers; + + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy; + + // Every argument is named. The defaulted ones would be temporaries, and the operation keeps + // references to them past the end of this expression. + curl::HttpOperation operation(http_client::Method::Get, "http://127.0.0.1:19000/get/", + ssl_options, &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_EQ(200, operation.GetResponseCode()); +} + #ifdef ENABLE_OTLP_RETRY_PREVIEW TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) { @@ -461,6 +546,32 @@ TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) ASSERT_TRUE(operation.IsRetryable()); } +// Reading a message and taking the operation on for another attempt are one decision. The status +// an operation reports does not change when it is cleaned up, so a caller that asks about it +// separately would put a session whose easy handle is on its way out back in the retry queue. +TEST_F(BasicCurlHttpTests, ACleanedOperationDoesNotAskToBeRetried) +{ + RetryEventHandler handler; + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy = {5, std::chrono::duration{1.0f}, + std::chrono::duration{5.0f}, 1.5f}; + + curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, + &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_TRUE(operation.IsRetryable()); + + operation.Cleanup(); + + EXPECT_TRUE(operation.IsRetryable()); + EXPECT_FALSE(operation.PerformCurlMessage(CURLE_OK)); +} + TEST_F(BasicCurlHttpTests, RetryPolicyDisabled) { RetryEventHandler handler; @@ -629,6 +740,226 @@ TEST_F(BasicCurlHttpTests, ResetMultiHandleWithASessionDoesNotDeadlock) client->FinishAllSessions(); } +// The record queued for removal is the only thing naming the easy handle, the header list it +// points at and the session, so a removal libcurl refuses has to keep all three. The message +// loop reads that session back out of CURLOPT_PRIVATE, which makes letting go of it as much a +// live pointer left behind as a handle. +TEST_F(BasicCurlHttpTests, ARefusedRemovalKeepsTheHandleAndTheSessionItNames) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + const auto session_id = std::static_pointer_cast(session)->GetSessionId(); + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: keep"); + ASSERT_TRUE(headers_chunk != nullptr); + + http_client::curl::HttpClientTestPeer::NoteAttached(client, easy_handle); + client.ScheduleRemoveSession(session_id, {easy_handle, headers_chunk}); + + // From here the client holds the only reference, so what survives the removal is what the + // removal chose to keep. + std::weak_ptr watch = session; + session.reset(); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); + EXPECT_FALSE(watch.expired()); +} + +// A queued handle still names its session through CURLOPT_PRIVATE, and the message loop reads +// that back. The session is taken when the record is made rather than found when the record is +// drained, because between those two the calling thread returns from FinishSession and takes the +// session out of sessions_, and by then the queue has been swapped away. +TEST_F(BasicCurlHttpTests, AQueuedHandleTakesTheSessionItNamesWithIt) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + const auto session_id = std::static_pointer_cast(session)->GetSessionId(); + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: owner"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(session_id, {easy_handle, headers_chunk}); + + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalOwnerCount(client)); + + // What the calling thread does next takes nothing away, because there is nothing left for it + // to decide. + std::weak_ptr watch = session; + session.reset(); + client.CleanupSession(session_id); + + EXPECT_FALSE(watch.expired()); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + EXPECT_TRUE(watch.expired()); +} + +// Scheduling an abort takes the session out of sessions_, so a handle queued after that finds +// nothing to take and the abort hands the session over itself. Both steps are the background +// thread's, one after the other, and no request is sent here so there is no background thread to +// run them out of order. +TEST_F(BasicCurlHttpTests, AnAbortedSessionIsGivenToTheHandleItQueued) +{ + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000"); + ASSERT_TRUE(session != nullptr); + const auto session_id = std::static_pointer_cast(session)->GetSessionId(); + + client.ScheduleAbortSession(session_id); + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: aborted"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(session_id, {easy_handle, headers_chunk}); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalOwnerCount(client)); + + http_client::curl::HttpClientTestPeer::AbortSessions(client); + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalOwnerCount(client)); + + std::weak_ptr watch = session; + session.reset(); + EXPECT_FALSE(watch.expired()); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + EXPECT_TRUE(watch.expired()); +} + +// A multi handle that was never given a handle has nothing to give back, and asking it is not a +// way to find that out: libcurl 8.10 and 8.11 refuse, and a multi handle that failed to +// initialize refuses on every version. Reading either refusal as ownership strands the handle. +TEST_F(BasicCurlHttpTests, AHandleTheMultiHandleNeverTookIsFreedNotStranded) +{ + curl::HttpClient client; + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: free"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(4405U, {easy_handle, headers_chunk}); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); +} + +// A handle handed over after the background thread has stopped, or before it ever started, has +// nobody left to drain it, so the client frees it on the way out. Nothing here can watch that +// happen, which leaves this case checking that the record reaches the queue and the sanitizer +// builds checking what becomes of it. +TEST_F(BasicCurlHttpTests, AHandleQueuedWithNoOneLeftToDrainItIsFreedWithTheClient) +{ + curl::HttpClient client; + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: undrained"); + ASSERT_TRUE(headers_chunk != nullptr); + + client.ScheduleRemoveSession(4405U, {easy_handle, headers_chunk}); + + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); +} + +// curl_multi_cleanup detaches whatever the old multi handle held, so a handle recorded against +// it is not attached to anything once it has been replaced, and nothing may be handed back to +// the replacement. Emptying the ledger in the same step is what says so. The replacement is made +// to refuse every removal here, so an offer to it would leave the handle kept back instead of +// freed. +TEST_F(BasicCurlHttpTests, AHandleFromTheReplacedMultiHandleIsNotOfferedToItsSuccessor) +{ + curl::HttpClient client; + + CURL *easy_handle = curl_easy_init(); + ASSERT_TRUE(easy_handle != nullptr); + curl_slist *headers_chunk = curl_slist_append(nullptr, "X-Test: generation"); + ASSERT_TRUE(headers_chunk != nullptr); + + http_client::curl::HttpClientTestPeer::NoteAttached(client, easy_handle); + EXPECT_EQ(1U, http_client::curl::HttpClientTestPeer::AttachedCount(client)); + + http_client::curl::HttpClientTestPeer::ResetMultiHandle(client); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::AttachedCount(client)); + + client.ScheduleRemoveSession(4405U, {easy_handle, headers_chunk}); + + CURLM *multi_handle = http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, nullptr); + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + http_client::curl::HttpClientTestPeer::ExchangeMultiHandle(client, multi_handle); + + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); +} + +// One record per easy handle. A session that starts another request hands over a second handle +// while the first is still queued, and keying the queue by session would lose the first one. +TEST_F(BasicCurlHttpTests, ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst) +{ + curl::HttpClient client; + + CURL *first = curl_easy_init(); + ASSERT_TRUE(first != nullptr); + CURL *second = curl_easy_init(); + ASSERT_TRUE(second != nullptr); + curl_slist *first_headers = curl_slist_append(nullptr, "X-Test: first"); + ASSERT_TRUE(first_headers != nullptr); + curl_slist *second_headers = curl_slist_append(nullptr, "X-Test: second"); + ASSERT_TRUE(second_headers != nullptr); + + client.ScheduleRemoveSession(4405U, {first, first_headers}); + client.ScheduleAddSession(4405U); + client.ScheduleRemoveSession(4405U, {second, second_headers}); + + EXPECT_EQ(2U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); + + http_client::curl::HttpClientTestPeer::RemoveSessions(client); + + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::PendingRemovalCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(client)); +} + +// The same ledger after a real request. The background thread is the only one that writes it and +// it only stops once nothing is running, so what a case can look at is the state it leaves +// behind: an entry left there would send a later handle allocated at the same address through a +// removal it never needed, and nothing should still be waiting to be given back. +TEST_F(BasicCurlHttpTests, TheAttachmentLedgerIsEmptyOnceARequestFinishes) +{ + received_requests_.clear(); + curl::HttpClient client; + + auto session = client.CreateSession("http://127.0.0.1:19000/get/"); + auto request = session->CreateRequest(); + request->SetUri("get/"); + auto handler = std::make_shared(); + + session->SendRequest(handler); + ASSERT_TRUE(waitForRequests(30, 1)); + session->FinishSession(); + client.WaitBackgroundThreadExit(); + + EXPECT_TRUE(handler->got_response_.load(std::memory_order_acquire)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::AttachedCount(client)); + EXPECT_EQ(0U, http_client::curl::HttpClientTestPeer::QuarantinedCount(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. @@ -663,8 +994,6 @@ TEST_F(BasicCurlHttpTests, ACancelFromTheCallerThreadReportsCancelled) // thread sanitizer this reports against the unfixed client on every run. TEST_F(BasicCurlHttpTests, RepeatedCallerThreadCancelsAreClean) { - int terminal_total = 0; - for (int i = 0; i < 20; ++i) { auto session_manager = std::make_shared()->Create(); @@ -681,12 +1010,15 @@ TEST_F(BasicCurlHttpTests, RepeatedCallerThreadCancelsAreClean) session_manager->FinishAllSessions(); EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire)); - terminal_total += handler->terminal_count_.load(std::memory_order_acquire); - } - // A lower bound, not a count: #4360 tracks the same cancel arriving twice, and how many - // arrive is not what this case decides. - EXPECT_GE(terminal_total, 20); + // Whichever side wins, the attempt ends somewhere the handler is told about: the cancel + // lands first, or the connection to a closed port fails first and the cancel is then + // correctly a no-op. How many cancels arrive is not what this case decides, #4360 tracks + // that, and neither is which of the two wins. + EXPECT_GT(handler->terminal_count_.load(std::memory_order_acquire) + + handler->failed_count_.load(std::memory_order_acquire), + 0); + } } TEST_F(BasicCurlHttpTests, SendGetRequestSync)