open-telemetry / open-telemetry/opentelemetry-cpp
[BUG] Cancelling from a Created or Connecting event loses the cancel and leaves the operation unfinished
Nobody has claimed this yet.
- 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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