open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] Cancelling from a Created or Connecting event loses the cancel and leaves the operation unfinished

Open
#4,390 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
Dominant language
C++
Stars
1.4k
Forks
632
Avg merge
1d 13h
Merged PRs (30d)
75

Description

Describe your environment main at f6e48180, Ubuntu 24.04, gcc 14, libcurl 8.14.1, CMake. The ordering below has no platform conditionals in it.

HttpOperation::SendAsync() hands the first event to the user before the operation is ready to be cancelled, and a cancel that lands in that window is thrown away. The request is then neither sent nor completed, and the caller blocks when it tries to finish the session.

  curl_easy_setopt(curl_resource_.easy_handle, CURLOPT_PRIVATE, session);

  DispatchEvent(...SessionState::Connecting);                    // :1454  user handler runs here
  is_finished_.store(false, std::memory_order_release);
  is_aborted_.store(false, std::memory_order_release);           // :1456  clears the cancel
  is_cleaned_.store(false, std::memory_order_release);

  async_data_->session = session;                                // :1459  route published here
  ...
  async_data_->result_promise = std::promise<CURLcode>();        // :1462  promise created here
  ...
  session->GetHttpClient().ScheduleAddSession(...);              // :1467

Say the handler calls Session::CancelSession() from that Connecting event, which the public API allows. Abort() at :1509 raises is_aborted_, then checks async_data_->session at :1516, which is still the nullptr written at :1440, so no abort is scheduled. CleanupSession() at :379 does run, and it removes the session from sessions_.

SendAsync then resumes and undoes the rest of it. Line :1456 puts is_aborted_ back to false. ScheduleAddSession at :641 erases the pending abort at :647. When the IO thread reaches doAddSessions, the session is no longer in sessions_, so the lookup at :735 misses and it continues at :738 without ever adding the handle to the multi handle.

What is left is a live promise with nobody to fulfil it. HttpOperation::Finish() waits on it at :516, and ~HttpOperation waits on it at :491 when the state is Connecting.

Steps to reproduce Add this to ext/test/http/curl_http_test.cc on unmodified main. It uses TerminalCountingHandler, which is already there:

TEST_F(BasicCurlHttpTests, CancelFromConnecting)
{
  auto session_manager = std::make_shared<http_client::curl::HttpCurlClientFactory>()->Create();
  auto session = session_manager->CreateSession("http://127.0.0.1:19000");
  auto request = session->CreateRequest();
  request->SetUri("get/");

  auto handler            = std::make_shared<TerminalCountingHandler>();
  handler->cancel_target_ = session.get();
  handler->cancel_at_     = http_client::SessionState::Connecting;

  session->SendRequest(handler);
  session->FinishSession();
  session_manager->FinishAllSessions();
}

SendRequest returns and FinishSession never does. Under a 60 second timeout the binary exits 124 with zero cases finished. A core taken at 12 seconds:

#5  std::__future_base::_State_baseV2::wait (...) at /usr/include/c++/14/future:360
#7  HttpOperation::Finish (...) at ext/src/http/client/curl/http_operation_curl.cc:516
#8  Session::FinishSession (...) at ext/src/http/client/curl/http_client_curl.cc:259

What is the expected behavior? A cancel from any event either takes effect or does nothing, and either way the operation reaches a terminal state and releases whoever is waiting on it.

What is the actual behavior? The cancel is erased, the request is never sent, and the session cannot be finished or destroyed without blocking forever.

Additional context Created has the same shape one step earlier. The constructor dispatches it at :475, but Session::curl_operation_ is only assigned when curl_operation_.reset(...) at http_client_curl.cc:203 returns, so a handler cancelling from Created cannot reach an operation at all.

The fix I would reach for is to publish the promise, the callback and the session route before the first DispatchEvent, and to make add versus abort a single decision that abort always wins, rather than two flags that can overwrite each other. That is a bigger change than I want to guess at, so I would rather ask first.

Found while working on #4375. Different mechanism, same function, and #4375 does not touch this.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with HttpOperation::SendAsync(), Abort(), CleanupSession(), and the session scheduling paths in ext/src/http/client/curl/http_operation_curl.cc and http_client_curl.cc. Add the CancelFromConnecting reproduction to ext/test/http/curl_http_test.cc and run the curl HTTP tests. Done means cancellation from Created or Connecting reaches a terminal state without leaving FinishSession or destruction blocked.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp
Domain
networking, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
54/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.