open-telemetry / open-telemetry/opentelemetry-cpp
[BUG] The curl HTTP client can report a request failed and still send it, and can deliver two terminal events
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 75
Description
Three places in Session::SendRequest where the shared curl client tells an exporter
something other than what it then does. This is ext/src/http/client/curl/, the production
client every HTTP exporter runs on, not the embedded test server whose scope was settled on
#4287.
All three are on main today.
1. A gzip failure reports the request as failed and then sends it anyway
http_client_curl.cc, inside the kGzip branch, which is compiled under
ENABLE_OTLP_COMPRESSION_PREVIEW, so this one reaches only builds with compression enabled:
if (stream != Z_OK)
{
if (callback)
{
callback->OnEvent(SessionState::CreateFailed, zs.msg ? zs.msg : "");
}
is_session_active_.store(false, std::memory_order_release);
}
deflateEnd(&zs);
There is no return. Control falls through to curl_operation_.reset(new HttpOperation(...))
and SendAsync, so the exporter has already been told the request failed while the request
goes out.
The body is the worrying part. deflateInPlace compresses into the caller's buffer, and
only the success path adds Content-Encoding: gzip and resizes to max_size. A failure
partway through therefore leaves a buffer that may have been written in place, at its
original length, with no encoding header, and that is what gets sent.
What an exporter does with this depends on the exporter, but the shapes are all bad: report
failure and let a retry send the batch a second time, or report failure while the server
accepts something it cannot parse.
2. Cancelled and OnResponse can both fire for one operation
In the SendAsync completion lambda:
if (operation.WasAborted())
{
callback->OnEvent(SessionState::Cancelled, "");
}
if (operation.GetSessionState() == SessionState::Response)
{
...
callback->OnResponse(*response);
}
Two independent ifs, so an operation aborted after a response arrived delivers both. A
handler that treats either as terminal sees its export settle twice. That is the same shape
as #4338 on the Elasticsearch side, where the fix under review adds a first-writer-wins
guard in the exporter. The guard is needed in every consumer as long as the client can do
this.
3. One setup failure produces two terminal events
HttpOperation::SendAsync dispatches ConnectFailed and returns non-CURLE_OK
(http_operation_curl.cc). Back in Session::SendRequest, success is false, so the else
branch dispatches CreateFailed as well. One failure, two events, and a handler counting
terminal states counts two.
What would settle it
A contract on EventHandler: exactly one terminal event per SendRequest, and no bytes on
the wire after one has been delivered. Concretely that means returning after the gzip
failure, making the abort and response branches mutually exclusive, and letting the setup
path own its single event rather than both layers reporting.
Until then every consumer needs its own idempotence guard to be correct, which is a rule
nothing in the interface states.
I have not sent a patch. Three separate behaviour changes to a shared client is more than
belongs in one, and I would rather hear which of the three you want changed and whether the
EventHandler contract should be written down first.
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 in ext/src/http/client/curl/http_client_curl.cc at Session::SendRequest and its kGzip branch, then read http_operation_curl.cc and the SendAsync completion lambda. Confirm the desired EventHandler contract with maintainers before changing scope. Done means each SendRequest produces exactly one terminal event and no request is sent after failure.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend-api-design, networking
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100