[BUG] FinishSession() from an OnEvent handler deadlocks the curl client's background thread #4402

Line numbers, re-resolved 19 October 2026. Everything quoted below is at f6e48180, which the environment section names. Against main at cdd17eb1 the file has moved, so here is where each cited line is now.

cited	what it is	on cdd17eb1
line 506	the is_finished_.exchange in Finish()	523
line 514	the thread-marker compare in Finish()	531
http_operation_curl.cc:516, line 516	the result_future wait in Finish()	534, and it is .get() now rather than .wait()
line 536	the Cancelled dispatch in Cleanup()	553
lines 566 and 568	the marker set and clear in Cleanup()	581 and 583
line 574	result_promise.set_value	589
http_operation_curl.cc:412	the OnEvent call in DispatchEvent	429, with DispatchEvent itself at 424
http_client_curl.cc:259	Session::FinishSession	280
curl_http_test.cc:158	FinishInCallbackHandler	the class is at 231

One claim in the body is wrong and not just stale. It says the only change to this file since the core was taken is #4399 in NextRetryTime. There are 10 commits on http_operation_curl.cc and 7 on http_client_curl.cc since f6e48180, and two of them are in this path: #4392 and #4431. The defect itself is unchanged: DispatchEvent still calls OnEvent outside any marker, and Finish() still waits.

Describe your environment

main at 768d04de, Debian, gcc 14.2.0, libcurl 8.14.1, plain CMake build with the defaults. No sanitizer needed, an ordinary build reproduces it every time. I haven't tried other platforms, but nothing in the mechanism looks platform specific.

Steps to reproduce

Drop this into ext/test/http/curl_http_test.cc. CustomEventHandler is the helper already in that file. Nothing listens on 19937, so the connection fails and the background thread dispatches ConnectFailed.

class FinishFromOnEventHandler : public CustomEventHandler
{
public:
  http_client::Session *session_ = nullptr;
  std::atomic<bool> entered_{false};
  std::atomic<bool> returned_{false};

  void OnResponse(http_client::Response &) noexcept override {}

  void OnEvent(http_client::SessionState state, nostd::string_view) noexcept override
  {
    if (state == http_client::SessionState::ConnectFailed && session_ != nullptr &&
        !entered_.exchange(true))
    {
      session_->FinishSession();
      returned_.store(true, std::memory_order_release);
    }
  }
};

TEST_F(BasicCurlHttpTests, FinishSessionFromOnEvent)
{
  auto session_manager = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
  auto session = session_manager->CreateSession("http://127.0.0.1:19937");
  auto request = session->CreateRequest();
  request->SetUri("get/");

  auto handler      = std::make_shared<FinishFromOnEventHandler>();
  handler->session_ = session.get();

  session->SendRequest(handler);
  std::this_thread::sleep_for(std::chrono::seconds(3));

  EXPECT_TRUE(handler->entered_.load(std::memory_order_acquire)) << "handler never ran";
  EXPECT_TRUE(handler->returned_.load(std::memory_order_acquire))
      << "FinishSession from OnEvent did not return";
}

The handler has to be the first caller of FinishSession(). If the test body calls it first, Finish() early-returns at the is_finished_.exchange on line 506 and never reaches the wait below, which is what sent me down the wrong path the first time I looked at this.

What is the expected behavior?

FinishSession() returns, the same as it does when a handler calls it from OnResponse. That path is deliberately supported: Cleanup() marks the calling thread on lines 566 and 568 around callback(*this), and Finish() consults that marker on line 514 before it waits. BasicCurlHttpTests.FinishInAsyncCallback covers it, through FinishInCallbackHandler, which calls session_->FinishSession() from OnResponse on line 158 of the same test file.

What is the actual behavior?

It never returns, and it takes the client's background thread with it.

curl_http_test.cc: Failure
FinishSession from OnEvent did not return

The process then has to be killed. Under timeout 90 the run exits 124.

Additional context

Backtrace of the stuck thread, from a core taken on f6e48180. The only change to this file since then is #4399 in NextRetryTime, which sits well below every line referenced here.

#5  std::__future_base::_State_baseV2::wait ()
#6  std::__basic_future<CURLcode>::wait ()
#7  HttpOperation::Finish ()                        http_operation_curl.cc:516
#8  Session::FinishSession ()                       http_client_curl.cc:259
#9  FinishFromOnEventHandler::OnEvent ()
#10 HttpOperation::DispatchEvent ()                 http_operation_curl.cc:412
#11 HttpOperation::PerformCurlMessage (code=CURLE_COULDNT_CONNECT)
#13 HttpClient::MaybeSpawnBackgroundThread()::<lambda()>

Finish() waits on result_future on line 516. The one place that promise is satisfied is line 574, at the tail of Cleanup(), and the same background thread only reaches Cleanup() after PerformCurlMessage returns. So the thread is blocked waiting for something further down its own call stack. No race is involved, it deadlocks on every run.

What decides whether Finish() waits is that thread marker, and the marker only covers callback(*this) inside Cleanup(). DispatchEvent on line 412 calls OnEvent with no marker set, so every OnEvent the background thread dispatches is exposed: Connected, Sending, Response, ConnectFailed, SendFailed and Cancelled. Only Created and the first Connecting are dispatched on the caller's own thread and come back normally.

Five of those dispatches sit inside WriteMemoryCallback, WriteVectorHeaderCallback, WriteVectorBodyCallback, ReadMemoryCallback and PreRequestCallback, so in those cases the thread parks inside curl_multi_perform while libcurl still owns the easy handle.

One state is split down the middle, and that is what makes this awkward to discover from the outside. On an aborted request Cancelled reaches the handler twice: once from Cleanup() line 536 through DispatchEvent, carrying libcurl's error string as the reason, and once from the completion lambda in Session::SendRequest, carrying an empty reason. The second one runs inside the marker and returns fine. The first one deadlocks. Same state, different reason string, opposite outcome. (The double delivery itself is #4360, not this.)

background_thread_ is a single std::unique_ptr<std::thread> per HttpClient and MaybeSpawnBackgroundThread reuses the running one, so once it is parked nothing else on that client makes progress either. For an exporter that means the block is not limited to the request whose handler called in.

The public header documents session->FinishSession() from the caller and says nothing either way about handlers, so I don't think this is a documented restriction users are ignoring. The library already supports the same call from the adjacent callback, and that's the part that makes the asymmetry look unintended rather than deliberate.

I haven't sent a patch because I'd be guessing at the contract. Widening the marker to cover DispatchEvent is the small change, but it's one slot on the operation and DispatchEvent runs on both threads, so it would need save and restore rather than clear to default. The other reading is that Finish() should never wait at all when it's already on the thread that drives the transfer. Say which shape you'd want and I'll put it together.