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

[thc1006]

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.

[thc1006]

Four notes on the description, two of them corrections to it.

Cleanup() isn't reached after PerformCurlMessage returns. It's called from the tail of PerformCurlMessage itself, at line 1630, under if (!is_retryable || retry_after_exceeds_max_delay). That makes the picture tighter than I drew it rather than looser: the thread blocks near the top of a function waiting for a set_value that the bottom of the same function would have done. Same conclusion, wrong sentence.

"Only Created and the first Connecting are dispatched on the caller's own thread" is wrong too. SendAsync dispatches ConnectFailed on the caller thread at line 1450, on the Setup() failure path:

  CURLcode code     = Setup();
  last_curl_result_ = code;
  if (code != CURLE_OK)
  {
    const char *message = GetCurlErrorMessage(code);
    DispatchEvent(opentelemetry::ext::http::client::SessionState::ConnectFailed, message);
    return code;
  }

So ConnectFailed reaches a handler from either thread depending on how far the request got, and only the one from the background thread hangs. That's a worse property than the tidy split I described, not a better one: the same handler, on the same state, either returns or deadlocks depending on whether the local setup failed or the connection did.

I should have checked whether my own open PRs already cover this before filing, so I've done it now. It still reproduces with #4392 applied and with #4395 applied, exit 124 at a 90 second timeout and entered=1 returned=0 in both cases. Neither of them goes near the thread marker.

Last one, since they sit next to each other: this isn't #4390. That one is a cancel that gets dropped, so the operation is never finished at all. This one is a FinishSession() that does arrive and then blocks the thread that would have finished it. Different cause, different fix, and #4390's fix doesn't move this.