[BUG] Cancel a curl session without writing to the easy handle

[thc1006]

Fixes #4375.

Abort() ran curl_easy_setopt on whichever thread called CancelSession(), while the IO thread was driving the same easy handle through curl_multi_perform. That is the pairing TSAN caught in #4369, and libcurl's rule is that one easy handle must not be used by two threads at the same time.

Where the option is set now

CURLOPT_NOPROGRESS was the only option still left at its default, which is the whole reason Abort() had to reach for the handle. SendAsync() sets it now, right beside CURLOPT_PRIVATE, on the thread that owns the handle and before the handle is scheduled. Cancelling is left with the atomic flag the progress callback already reads plus the abort it schedules.

It goes there rather than in Setup() because Setup() serves the synchronous path too, and nothing on that path can be cancelled: Abort() runs only from Session::CancelSession(), and HttpClientSync never makes a session. Arming the callback there would have bought nothing.

@owent, this keeps the fallback you described. ScheduleAbortSession() already calls wakeupBackgroundThread(), so on 7.68 and later curl_multi_wakeup() gets the IO thread out of curl_multi_poll on its own. What the progress callback still buys is the other case, where the IO thread is inside curl_multi_perform rather than waiting in the poll, and arming it up front costs one atomic load per progress tick instead of a cross thread write to the handle. The callback fires 376 times across curl_http_test, so the option is taking effect, and the cost does not show up: over five runs of the same 22 cases, main came in between 17.6 s and 18.6 s and this branch between 16.1 s and 18.6 s, which is the same spread.

The race that actually reproduces

There is a second one in the same call, and it turned out to be the one I could trigger. AsyncData::session is a plain Session * that Abort() reads at :1516 while Cleanup() writes nullptr to it at :552 on the IO thread. It is std::atomic<Session *> now.

There is no lifetime question sitting behind the pointer value here: Abort() has exactly one caller, Session::CancelSession() at http_client_curl.cc:249, so the pointer is always the session whose own method is running, and callers hold that by shared_ptr.

Against unmodified main, with only the new test case added, TSAN reports on every run:

WARNING: ThreadSanitizer: data race
  Write of size 8 by thread T3:
    #0 HttpOperation::Cleanup() ext/src/http/client/curl/http_operation_curl.cc:552
    #1 HttpOperation::PerformCurlMessage(CURLcode) ext/src/http/client/curl/http_operation_curl.cc:1629
  Previous read of size 8 by main thread:
    #0 HttpOperation::Abort() ext/src/http/client/curl/http_operation_curl.cc:1516
    #1 Session::CancelSession() ext/src/http/client/curl/http_client_curl.cc:249

curl_http_test --gtest_filter='*RepeatedCallerThreadCancels*', 5 runs	TSAN warnings
main at f6e4818 plus the new case	5
this branch	0

The return value had to change with it

OnProgressCallback() returned CURL_PROGRESSFUNC_CONTINUE when the request had not been aborted. That branch was unreachable while CURLOPT_NOPROGRESS was only cleared inside Abort(), since by then is_aborted_ is already up and the callback returns early. Arming the option up front makes it the normal path, and it does not mean what the name suggests:

If your callback function returns CURL_PROGRESSFUNC_CONTINUE it makes libcurl to continue executing the default progress function.

lib/progress.c falls straight through to progress_meter(), which writes to data->set.err, and that is stderr unless CURLOPT_STDERR says otherwise:

      if(rc != CURL_PROGRESSFUNC_CONTINUE) {
        if(rc) {
          failf(data, "Callback aborted");
          return CURLE_ABORTED_BY_CALLBACK;
        }
        return CURLE_OK;
      }
    ...
    if(showprogress)
      progress_meter(data);

With the option armed and nothing differing but the return value:

non-aborted return	curl_http_test stderr	progress meter headers
CURL_PROGRESSFUNC_CONTINUE	10,305 bytes	31
0	114 bytes, one expected log line	0

So without this half, the fix would have printed curl progress bars to stderr for every export that ran longer than a second. A standalone libcurl program built twice from one source, differing only in that return, gives the same answer: 711 bytes of meter against none.

Tests

RepeatedCallerThreadCancelsAreClean cancels from the caller thread while the IO thread owns the handle, twenty times. Nothing listens on 19937, so each attempt fails to connect and the IO thread reaches Cleanup() while the caller is still inside CancelSession(), which is the overlap the race needs. It passes on main too, since a data race is not a functional failure, and it is there for the sanitizer builds.

The two existing cancel cases keep asserting they cancel from the IO thread, but the comment explaining why has to go: it said cancelling reaches curl_easy_setopt and therefore has to run on the owning thread, and after this that is no longer true.

What this does not fix

Three things I ran into on the way and filed rather than folding in here: #4389, where the IO thread deadlocks on sessions_m_ recovering from a curl_multi_perform error, #4390, where cancelling from a Created or Connecting event loses the cancel and leaves the caller blocked forever, and #4391, where the handle is reset and its header list freed before the removal from the multi handle. All three are on this file or its neighbour and none of them changes what is here.

#4360 is also unaffected: a cancel that the progress callback turns into CURLE_ABORTED_BY_CALLBACK still produces ConnectFailed or SendFailed followed by Cancelled. That was already the case before this change, since Abort() armed the same callback, so the event sequence is the same as it was.

Checks

24 of 24 in curl_http_test, clean under OTELCPP_MAINTAINER_MODE=ON, clean under clang-format 18.1.8. The whole suite under TSAN is 24 of 24 with zero warnings.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.38%. Comparing base (1279a7a) to head (1b8c4ea).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4392      +/-   ##
==========================================
+ Coverage   82.35%   82.38%   +0.04%     
==========================================
  Files         502      502              
  Lines       19877    19883       +6     
==========================================
+ Hits        16368    16379      +11     
+ Misses       3509     3504       -5     

Files with missing lines	Coverage Δ
...lemetry/ext/http/client/curl/http_operation_curl.h	91.31% <ø> (ø)
ext/src/http/client/curl/http_operation_curl.cc	60.60% <100.00%> (+0.76%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lalitb]

LGTM. Nice work finding and fixing this race. Thanks!

[marcalff]

@thc1006

Please resolve merge conflicts, will merge after that.

[thc1006]

Rebased onto main and it's mergeable again.

Both conflicts were with #4394, which landed in between. The test file one keeps both cases, ResetMultiHandleWithASessionDoesNotDeadlock from #4394 and the two cancel cases from here, and the CHANGELOG keeps both entries. I checked afterwards that #4394's own change survived rather than assuming it had: friend class HttpClientTestPeer is still in the header, and sessions_m_ is still scoped to the snapshot inside resetMultiHandle rather than held across CancelSession and doRemoveSessions.

26 of 26 in curl_http_test, clang-format 18.1.8 clean, and include-what-you-use 0.26 with the repository mapping file reports nothing on the three files this touches.

The two commits also became one. The second only corrected the description of the first, so keeping both would have put the same subject line on the branch twice.

[thc1006]

Conflicts resolved, the branch is on current main.

The one red job, CMake -> exporter proto (Build as DLL), is not from this branch. Step 5, the environment setup, failed while bootstrapping vcpkg:

Downloading https://github.com/microsoft/vcpkg-tool/releases/download/2025-09-03/vcpkg.exe -> D:\a\opentelemetry-cpp\opentelemetry-cpp\tools\vcpkg\vcpkg.exe...
While calling Windows API function WinHttpReceiveResponse got error 0x00002EE2:
The operation timed out
D:\a\opentelemetry-cpp\opentelemetry-cpp\tools\vcpkg\scripts\bootstrap.ps1 : Downloading vcpkg.exe failed.

The three steps after it, run cmake test (DLL build), run cmake cxx20 test (DLL build) and run otprotocol test (DLL build), are all skipped, so nothing here was compiled in that job and the log holds no compiler output at all. The rebase touched ext/test/http/curl_http_test.cc and CHANGELOG.md and nothing else.

Re-running it needs admin rights on the repository, so that one is yours rather than mine.