Hacktoberfest 2026: le issue che i maintainer hanno segnato per ottobre, aperte e adatte ai principianti. Sfoglia le issue Hacktoberfest

[BUG] The curl HTTP client can report a request failed and still send it, and can deliver two terminal events

Aperta
  • Qualcosa non va?
#4,360 6 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
42/100
Tipo di issue
Bug
Chiarezza
Abbastanza chiara
Stato di attività
Tranquilla
Stack tecnologico
cpp
Ambito
backend-api-design, networking

Direzione di ricerca

Inizia in ext/src/http/client/curl/http_client_curl.cc, in Session::SendRequest e nel relativo ramo kGzip, poi leggi http_operation_curl.cc e la lambda di completamento di SendAsync. Conferma con i maintainer il contratto desiderato di EventHandler prima di modificare l’ambito. Il lavoro è completato quando ogni SendRequest produce esattamente un evento terminale e non viene inviata alcuna richiesta dopo un errore.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

bug triage/accepted

Four places in the shared curl client where it 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.

Two of the four are fixed on main. Two are still live.

status on f77c1a5c
1. gzip failure reports failed and sends anyway fixed by #4457, the branch now returns
2. Cancelled and OnResponse both fire fixed by #4392, the branches are else if
3. one setup failure, two terminal events still present
4. Cancelled twice for one cancelled transfer still present, measured below

The original three are kept below so the reports stay readable next to the fixes.

1. A gzip failure reports the request as failed and then sends it anyway (fixed, #4457)

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 (fixed, #4392)

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 (still on main)

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.

4. One cancelled transfer delivers two Cancelled events (still on main, measured)

#4392 fixed 2 by making the two arms of the completion callback mutually exclusive. That separates a response from a cancel. It does not separate a cancel from a cancel, because the first one is dispatched before the callback runs.

HttpOperation::Cleanup() dispatches for a transfer that had got as far as Created, Connecting, Connected or Sending:

  switch (GetSessionState())
  {
    case SessionState::Created:
    case SessionState::Connecting:
    case SessionState::Connected:
    case SessionState::Sending: {
      const char *message = GetCurlErrorMessage(last_curl_result_);
      DispatchEvent(SessionState::Cancelled, message);   // first
      break;
    }

DispatchEvent calls OnEvent and then assigns session_state_ = type, so the operation's state is now Cancelled. Cleanup() goes on to run the completion callback, and the else if from #4392 is reached precisely because the if above it can no longer match:

  if (operation.GetSessionState() == SessionState::Response)   // false, Cleanup just wrote Cancelled
  { ... }
  else if (operation.WasAborted())
  {
    callback->OnEvent(SessionState::Cancelled, "");            // second
  }

The two are distinguishable at the handler: the first carries a curl message, the second an empty reason.

Measured on f77c1a5c, using the case already in the tree. BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean runs twenty cancelled transfers and sums the terminal events:

PROBE terminal_total=40 over 20 iterations, mean=2
[       OK ] BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean (501 ms)

Exactly two per transfer, every time. The case is green because its assertion is EXPECT_GE(terminal_total, 20), a lower bound of one each, and its own comment says the count belongs to this issue.

This matters for the exactly-once contract below: a consumer that dedupes on "first terminal event wins" is fine, but one that counts them, or that treats a second Cancelled as a second export settling, is not.

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. Two of the concrete steps are done: the gzip branch returns and the abort and response branches are mutually exclusive. What is left is letting one layer own each terminal event rather than two reporting it, which is the same shape in 3 and in 4. A test for it has to count events rather than assert a lower bound, since both remaining paths deliver one correct event and one extra.

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.

Edited 2026-09-22. The opening said all three were on main, which stopped being true when #4392 and #4457 merged. Re-checked against f77c1a5c: the gzip branch now says the request does not go out, the abort and response branches are else if, and SendAsync still dispatches ConnectFailed before Session::SendRequest dispatches CreateFailed. A fourth path was added the same day: #4392's else if does not separate a cancel from a cancel, and the count is measured rather than argued.

Lingua principale
C++
Stelle
1.4k
Fork
647
Merge medio
1g 10h
PR unite (30g)
73

Preparare l'ambiente

Apri in Codespaces

Avvia il container di sviluppo del progetto nel browser, con il tuo account GitHub.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di open-telemetry/opentelemetry-cpp

Tutte le issue di open-telemetry/opentelemetry-cpp

Issue simili

Altre issue su C++

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.