[BUG] HttpOperation::Abort writes to the curl easy handle from the cancelling thread

[thc1006]

Splitting this out of #4369, which is about the test that happened to expose it. dbarker asked for a separate issue to track the client side and the discussion of how to fix it.

What happens

HttpOperation::Abort() writes to the curl easy handle from whichever thread cancels:

void HttpOperation::Abort()
{
  is_aborted_.store(true, std::memory_order_release);
  if (curl_resource_.easy_handle != nullptr)
  {
    // Enable progress callback to abort from polling thread
    curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_NOPROGRESS, 0L);
    ...

The background thread is driving that same handle through the multi handle at the time. libcurl's rule is that an easy handle must not be used from two threads at once, and TSAN saw exactly that pairing:

main thread   Session::CancelSession -> HttpOperation::Abort -> curl_easy_setopt
thread T17    curl_multi_perform -> Curl_connect -> resolve_server -> Curl_resolv

Full log in #4369.

Reach

Session::CancelSession() is public, and HttpClient::CancelAllSessions() calls it for every session, which is what OtlpHttpClient::Shutdown() does. So an application that shuts down while a request is in flight is in this, not only a test. It is intermittent: the window is however long the IO thread spends inside the handle, so the odds go up when the cancel lands early, during name resolution or connect.

A possible fix

OnProgressCallback reads nothing but is_aborted_, which is atomic and is already set on the line above, and the callback is registered at setup through CURLOPT_XFERINFOFUNCTION and CURLOPT_XFERINFODATA. CURLOPT_NOPROGRESS is the only piece still left at its default, which is why Abort() has to reach for the handle at all. Turning it on at setup would leave Abort() with the flag and the scheduled teardown:

void HttpOperation::Abort()
{
  is_aborted_.store(true, std::memory_order_release);
  if (async_data_ && nullptr != async_data_->session)
  {
    async_data_->session->GetHttpClient().ScheduleAbortSession(
        async_data_->session->GetSessionId());
  }
}

The curl_resource_.easy_handle read goes too, since reading that member from the cancelling thread is the same kind of sharing.

The cost is a progress callback that runs during every transfer and does one atomic load. I have this built: 22 of 22 in curl_http_test, 19.1 s against 18.1 s on main so no slowdown, clean under OTELCPP_MAINTAINER_MODE=ON, and a probe confirms the callback really does fire, 459 times across the suite, so CURLOPT_NOPROGRESS = 0 at setup is taking effect.

I have not sent it, because it is a behaviour change in a shared client and worth agreeing on first. Happy to open it if this shape looks right.

What I could not do

Reproduce it. Twelve TSAN runs on the box, including one pointed at a slow-resolving host to widen the window, all clean. The evidence is the CI log and the code path, so whatever fix lands wants checking against the Bazel TSAN config rather than a local one.

[owent]

curl_multi_wakeup() is only available since curl 7.68. In ScheduleAbortSession, we use it to wake up the curl IO thread so that curl_multi_remove_handle() can be called to cancel the in-flight request promptly.

For curl versions before 7.68, curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_NOPROGRESS, 0L) is an alternative way to stop the data transfer as soon as possible — otherwise the thread may block for a long time waiting for curl_multi_perform(...) to return.

In some previously reported bugs, we found that when gracefully shutting down the whole system, it could block and take a long time to finish. With curl 7.68 or later, this situation is hard to reproduce.

Looking at curl's source code:

static CURLcode setopt_long(struct Curl_easy *data, CURLoption option,
                            long arg)
{
  // ...
  case CURLOPT_NOPROGRESS:
    /*
     * Shut off the internal supported progress meter
     */
    data->progress.hide = enabled;
    break;
  // ...
}

This write causes TSAN to report a data race. However, it should be safe in practice, because we have already ensured that the easy handle is still valid at this point. Do you have any suggestions on how to deal with this issue?

[thc1006]

Short answer: set CURLOPT_NOPROGRESS in SendAsync(), beside CURLOPT_PRIVATE, rather than in Abort(), and the cancelling thread stops touching the handle at all. #4392 does that. It goes there rather than in Setup() because Setup() serves the synchronous path too, and nothing there can be cancelled.

It keeps the pre-7.68 lever rather than putting it behind a version guard, which was my first instinct until I read your note. The wakeup only gets the IO thread out of curl_multi_poll; the callback covers the case where it's stuck inside curl_multi_perform. Those aren't the same case, and third_party_minimum saying 7.81 doesn't make the second one go away.

Two things I didn't expect came out of building it.

The setopt wasn't the race I could reproduce. A single caller-thread cancel gave nothing across twenty TSAN runs on main. Looping the cancel twenty times per run fired on five runs out of five, and not where I was looking:

Write of size 8 by thread T3:
  HttpOperation::Cleanup()   ext/src/http/client/curl/http_operation_curl.cc:552
Previous read of size 8 by main thread:
  HttpOperation::Abort()     ext/src/http/client/curl/http_operation_curl.cc:1516
  Session::CancelSession()   ext/src/http/client/curl/http_client_curl.cc:249

async_data_->session is a plain Session *, read by Abort() and cleared by Cleanup() on the IO thread. It's atomic in the PR. Nothing worse is hiding under it: Abort() only ever runs from Session::CancelSession(), so the pointer is always the session whose own method is executing, and callers hold that by shared_ptr.

Arming the callback exposes its return value. OnProgressCallback() returns CURL_PROGRESSFUNC_CONTINUE when nothing has been aborted, and that branch is dead today, because NOPROGRESS only reaches 0 after is_aborted_ is already up and the callback returns early. Arm the option up front and it becomes the normal path. CURL_PROGRESSFUNC_CONTINUE doesn't mean carry on quietly, it asks libcurl to run its built-in progress meter as well, and that meter writes to stderr.

With the option armed and nothing differing but the return value, curl_http_test writes 10,305 bytes of curl progress bars to stderr against 114 bytes. Moving the option on its own would have been a bad trade. It needs return 0 alongside it.

On safe in practice, I'd rather not have to decide. With the option set at setup, nothing writes to the handle off the IO thread any more, so whether that one write happened to be harmless stops being a question anyone has to answer later.

Three other things turned up on the way, filed separately to keep this one small: #4389, the IO thread deadlocking on sessions_m_ while recovering from a multi error, reproduced with a forced branch and a core; #4390, a cancel from a Connecting event getting erased so FinishSession() never returns, reproduced through the public API alone; #4391, the handle being reset and its header list freed before the removal from the multi handle.

One more I didn't file. AsyncData::callback_thread is a plain std::thread::id, written on the IO thread in Cleanup() and read by whoever calls Finish(), with OPENTELEMETRY_SANITIZER_NO_THREAD on both accessors and a pair of standalone fences that don't have an atomic object to pair with. A torn read in the direction that compares equal would skip the future wait. Say the word and I'll write it up.