[BUG] CleanupSession reads Session::curl_operation_ while SendRequest is still assigning it #4438

Line numbers, re-resolved 19 October 2026. Everything quoted below is at 60c3d11e, 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
http_client_curl.cc:203	the curl_operation_.reset(...) write	228
http_client_curl.cc:409	the read in CleanupSession	CleanupSession is at 404 and the read the stack names, session->GetOperation(), is at 429
http_client_curl.cc:251	Session::CancelSession	270

The body also says the three functions involved are byte identical between main and the branch the core was taken on. That stopped being true with #4431, merged 24 September, which gave SendRequest a send_started_.exchange guard and rewrote the compression path. The race is unchanged: the write at 228 takes no lock, and the read at 429 is under session_ids_m_.

Cancelling a session from one thread while another thread is still inside Session::SendRequest() races on Session::curl_operation_. ThreadSanitizer reports it as an unsynchronized read against a write, and the read arrives through the background thread's own route into the operation rather than through the one the client publishes for that purpose.

Describe your environment

main at 60c3d11, Linux 6.12 x86_64, gcc 14.2, libcurl 8.14.1, CMake preset all-options-abiv1, -fsanitize=thread. The three functions involved are byte identical between main and the branch the reproduction was built on, verified by hashing each function body.

Steps to reproduce

Warm a client so its IO thread is already running, then start SendRequest() and CancelSession() on the same session from two threads:

auto session = session_manager->CreateSession("http://127.0.0.1:19000");
auto request = session->CreateRequest();
request->SetUri("get/");
auto handler = std::make_shared<GetEventHandler>();

std::atomic<bool> go{false};
std::thread canceller([&session, &go]() {
  while (!go.load(std::memory_order_acquire)) { std::this_thread::yield(); }
  session->CancelSession();
});

go.store(true, std::memory_order_release);
session->SendRequest(handler);
canceller.join();

Two hundred iterations under ThreadSanitizer produced two reports and exit code 66.

What is the expected behavior?

CancelSession() is a public API on a shared session and the client already uses atomics and a background thread to support cancelling from a thread other than the one that sent the request. Calling it while another thread is inside SendRequest() should either be well defined or be documented as forbidden. Nothing in HttpClient or Session says a caller has to wait for SendRequest() to return first.

What is the actual behavior?

WARNING: ThreadSanitizer: data race

  Write of size 8 by main thread:
    #2 Session::SendRequest(std::shared_ptr<EventHandler>) http_client_curl.cc:203

  Previous read of size 8 by thread T3 (mutexes: write M0):
    #2 std::unique_ptr<HttpOperation>::operator bool() const
    #3 HttpClient::CleanupSession(unsigned long) http_client_curl.cc:409
    #4 Session::CancelSession() http_client_curl.cc:251

Line 203 is curl_operation_.reset(new HttpOperation(...)). SendRequest() writes that member under no lock at all, while CleanupSession() reads it under session_ids_m_, so the mutex the reader holds protects nothing here.

Why the published route does not cover this

SendAsync() publishes async_data_->session last, after the callback, the promise and the future exist, so that an abort queued through that route cannot reach an operation whose async state is incomplete. That ordering is sound, but it is not the only way in.

CleanupSession() never consults it. It decides from the session alone:

else if (session->IsSessionActive() && session->GetOperation())
{
  pending_to_abort_sessions_[session_id] = std::move(session);
  need_wakeup_background_thread          = true;
}

IsSessionActive() is set on the first line of SendRequest() and GetOperation() is non-null from the assignment at line 203, so both are already true well before SendAsync() has built anything. Session::CancelSession() takes that route unconditionally:

bool Session::CancelSession() noexcept
{
  if (curl_operation_) { curl_operation_->Abort(); }
  http_client_.CleanupSession(session_id_);
  return true;
}

Abort() may find no session route to queue against and do nothing, and CleanupSession() then hands the session to the IO thread anyway. doAbortSessions() accepts it on the same terms, if (session.second->GetOperation()), and calls Session::FinishOperation(), which is HttpOperation::Cleanup().

What can follow from it

The reported race is on the unique_ptr itself, which is the part a sanitizer can see without the timing having to be perfect. The window it opens is wider than that one member. Between line 203 and the end of SendAsync() the caller thread runs Setup() and then stores is_finished_, is_aborted_ and is_cleaned_ back to false and assigns async_data_->callback, the promise and the future. If the IO thread is inside Cleanup() for the same operation during that stretch, then the cancel can be erased by the later is_aborted_.store(false), the completion callback can be assigned and swapped out concurrently, and both threads can be touching the same easy handle, which libcurl does not allow.

Those follow-on effects are analysis rather than something I have caught in a report; the data race above is measured.

Additional context

I found this while working on #4390. #4395, the pull request against it, narrows one window by publishing the session route after the callback, and it does not touch this one, since SendRequest() is unchanged there. Related but distinct: #4402 is a self-deadlock from a handler, #4408 is the session_state_ write, and #4396 was a session reused while its operation was still running, fixed by #4431. None of them is this.

Two shapes of fix seem plausible and I do not have a preference yet. The startup could be restructured so the operation is fully prepared before it is published into the Session, which would also stop the Created event being dispatched before the session holds the operation. Or a publication flag could be added, released after the callback, promise, future and route are all in place, with CleanupSession() declining to hand an unpublished operation to the IO thread and leaving SendAsync() to finish it on the caller thread. I am happy to send a PR for whichever the maintainers prefer.