[BUG] The curl client writes HttpOperation::session_state_ from two threads without synchronization

[thc1006]

Describe your environment main at f1e1c9c, Linux, gcc 14.2, libcurl 8.14.1, CMake build with -fsanitize=thread. The race is in the source rather than in a build option, so the Bazel --config=tsan toolchain sees the same thing.

Steps to reproduce

HttpOperation::DispatchEvent stores the new state after the handler has returned, at http_operation_curl.cc:415:

void HttpOperation::DispatchEvent(SessionState type, const std::string &reason)
{
  if (event_handle_ != nullptr)
  {
    event_handle_->OnEvent(type, reason);
  }

  session_state_ = type;
}

session_state_ is a plain SessionState member (http_operation_curl.h:341, and SessionState is a std::uint8_t enum). Two threads reach that store for one operation whenever a handler cancels from an early event on a client that is already polling:

1. Send a request on a fresh client and let it finish. MaybeSpawnBackgroundThread() runs after SendAsync returns, so this first request is what puts an IO thread there at all.
2. Create a second session and call CancelSession() from its Connecting event, without leaving the handler yet.
3. The cancel wakes the IO thread, which runs doAbortSessions (http_client_curl.cc:777) into Session::FinishOperation (:269) into HttpOperation::Cleanup. Cleanup finds a non terminal state and dispatches a Cancelled of its own at http_operation_curl.cc:536, which stores session_state_ on the IO thread.
4. The handler returns and DispatchEvent stores session_state_ = Connecting on the caller thread.

I held the handler open for 400 ms so the overlap is wide enough to be deterministic rather than something I got lucky on. Three TSAN runs out of three report it, and five out of five reach the overlap on the behavioral build.

What is the expected behavior?

Either one thread owns the store, or the store is synchronized. And a state the operation has already left should not replace a terminal one.

What is the actual behavior?

TSAN reports a data race on the store. Trimmed to the two stacks:

WARNING: ThreadSanitizer: data race
  Write of size 1 by main thread:
    #0 HttpOperation::DispatchEvent(...) http_operation_curl.cc:415
    #1 HttpOperation::SendAsync(...) http_operation_curl.cc:1455
    #2 Session::SendRequest(...) http_client_curl.cc:209

  Previous write of size 1 by thread T2:
    #0 HttpOperation::DispatchEvent(...) http_operation_curl.cc:415
    #1 HttpOperation::Cleanup() http_operation_curl.cc:536
    #2 Session::FinishOperation() http_client_curl.cc:269
    #3 HttpClient::doAbortSessions() http_client_curl.cc:777

Past the undefined behavior, the later store wins, so GetSessionState() ends at Connecting for an operation that was cancelled. Two places read that member and act on it: Cleanup at :529 decides from it whether to dispatch at all, and the completion callback in Session::SendRequest compares it against Response to decide whether to deliver a response.

Additional context

Nothing in ext/test/http/curl_http_test.cc reaches this today, which is why Bazel tsan config is green. Every case that cancels from an early event uses a freshly created client, and there the IO thread does not exist while the event runs, so there is only one writer. That is measured rather than assumed: the same probe without the first request reports one thread inside the handler, and with it, two.

A warning for whoever writes the regression test. My first attempt counted concurrent handler entries with acq_rel atomics and TSAN went quiet. The IO thread's release RMW and the caller's later RMW on the same atomic synchronize with each other, which orders the two stores and hides the race the counters exist to catch. They have to be relaxed. I checked that with a deliberate unsynchronized counter on the same line as a control: the acq_rel build reported neither, the relaxed build reported both.

On main this scenario reports two races and then hangs. #4395 removes the hang and the other one, a write to async_data_->session in SendAsync racing the read of it in Cleanup, and leaves this one. So this is not something #4395 introduces, and it is not something #4395 fixes either.

Two shapes for a fix, and the choice does not look like mine to make:

• Make session_state_ a std::atomic. Removes the undefined behavior, leaves the later writer winning, so a cancelled operation can still report Connecting. It also changes the layout of a type in an installed header.
• Store before dispatching and refuse to leave a terminal state. That fixes the reported value too, but it changes what a handler sees when it asks GetSessionState() from inside its own event, which today is the state before the one it is being told about.

Scheduling note, same as on #4360. #4392 and #4395 are open against these two files. Tell me which shape you want and I will fit it around whatever has landed rather than sending a change that collides with them. It also holds up the regression test for the polling client half of #4390, because that test turns Bazel tsan config red on this race.

[thc1006]

One correction to the above, and this now has a fix in flight.

I wrote that making session_state_ a std::atomic "changes the layout of a type in an installed header". It does not. SessionState is a std::uint8_t enum, and std::atomic of it is one byte, aligned to one, and lock free, so neither the member nor the class changes size. HttpOperation already deletes its copy and move, so an atomic member costs nothing there either. I should have measured that before writing it down.

The other half of what I wrote also needs an update. @lalitb had already asked for this on #4395, at http_operation_curl.cc:1466 on 11 August, before I opened this. So it belongs there rather than in a change of its own, and #4395 now carries it.

Both shapes went in, because they fix different halves. Storing the state before the handler runs is what fixes the reported value: the store is then ordered ahead of the cancel, which goes through sessions_m_ and session_ids_m_, and the background thread takes the latter before it can reach Cleanup(). The atomic covers the paths with no such handoff, where a third thread cancels rather than the handler.

Nothing else in the client reads GetSessionState() from inside its own event, and every internal reader takes it after a dispatch has returned, where both orders give the same value, so moving the store does not change any single threaded path.

CancelFromConnectingWhilePollingCompletes on that branch is what measures it: five runs out of five report this race without the change, none with it.