Proposal: Redesign the curl HttpClient API to address thread-safety and lifetime issues

[owent]

It seems that ext::http::client::curl::HttpClient and the related code still suffer from numerous multithreading synchronization issues and lifetime-safety problems, even after multiple rounds of patches. Would it be possible to consider redesigning the entire API flow — re-evaluating which data is accessed only by the background thread, which is accessed only by the user thread, which is shared, and which data requires shared ownership/lifetime? Additionally, could reentrancy protection be applied to all user-facing interfaces?

BTW: I believe HttpOperation is unnecessary. All of its implementation interfaces could either become private internal interfaces within the cpp files, or serve as the implementation of ext::http::client::curl::Session, with curl-related resources bound to that class as well. For a single Session, adopting a state machine design pattern would likely help prevent errors in such complex flows.

Related issues: #4438, #4408, #4403, #4402, #4397, #4396, #4393, #4391, #4360

Do you think it would be worthwhile to completely redesign and rewrite this component?
@open-telemetry/cpp-approvers @open-telemetry/cpp-maintainers @thc1006

[thc1006]

I think the issue set now justifies redesigning the curl implementation, but I would separate agreeing on the contract from rewriting the code.

The failures no longer look independent. They cluster around a few common boundaries:

• an operation becomes visible before its callback, promise, cancel route and resource ownership are fully established;
• Session, HttpOperation and HttpClient can all act as owners of the same transfer at different points;
• the caller thread and the I/O thread both change operation state;
• user callbacks run synchronously inside state transitions and can re-enter or destroy the object that is still executing;
• more than one layer can decide and report the terminal outcome.

My preference would be to preserve the abstract HttpClient / Session / Request surface initially and replace the curl implementation behind it in stages.

Before implementation, I think we need to decide at least these contracts:

1. Is a Session single-use, or may it send more than one request?
2. Does SendRequest consume or freeze the Request?
3. Which thread owns CURLM and attached easy handles?
4. Is exactly one terminal outcome guaranteed per SendRequest?
5. On which thread are callbacks invoked, and which Session or client methods may they call?
6. What should a blocking method do when called from the I/O or callback thread?
7. When does shutdown stop admitting new operations, and how is its deadline propagated?

For the implementation, I would prefer one transfer object with strong ownership of its immutable request data, callback and curl resources. User threads would enqueue commands, while the I/O thread would be the only thread that mutates libcurl handles and transfer state. State would be committed before invoking user code, with no internal client lock held across a callback.

Merging HttpOperation into Session works if Session is formally single-use. If Session remains reusable, I think the operation still needs an identity of its own, and CURLOPT_PRIVATE should identify the exact operation rather than a Session whose current operation may already have changed.

I would keep #4395, #4405, #4406 and #4431 as drafts while this is decided. The reproductions and sanitizer cases in them are still valuable and can become the acceptance suite for a replacement. Narrow fixes that are independently correct under either design can still land if we need them before the replacement.

One correction to #4433: #4327 stopped installing the concrete curl headers through CMake, although Bazel //ext still exposes them. The measured dangling-reference bug remains, but the current external surface is narrower than I wrote there. I also agree that copying is not the right end state, and that moving or consuming immutable request state avoids both the copy and the borrowed lifetime. Correcting one thing I wrote on #4433 about my own patch: it moves at the in-tree call site rather than copying, and the by value parameters are what let it do that without changing a call site. The ownership question it raises still belongs here rather than in a patch.

I can put together an invariant/issue/test matrix from the existing reports and use that as the starting point for a design sketch.

[thc1006]

Here is the matrix I offered. It is built from the reports rather than from opinion, so that the design argument can start from what actually broke.

How to read it. Each row is an invariant the replacement has to hold, phrased so that a case can fail when it does not. Evidence is the report that showed it broken, and says whether that report carries a sanitizer result or a deterministic reproduction (measured) or is derived from control flow alone (reasoned). Pinned by is the case that fails today if the invariant breaks, and which draft it currently lives on. Rows with no case are collected at the end, and they are the part I would look at first.

I have kept the invariants at the level of "what must be true", not "how it should be built". Several of them are satisfied by more than one design, including yours.

A. Publication and ownership

#	Invariant	Evidence	Pinned by
A1	A transfer is not reachable from another thread until its callback, promise, cancel route and curl resources all exist	#4438 measured (TSan, two hundred iterations)	ACancelFromAnotherThreadDuringSetupIsStillHonoured (#4395)
A2	A cancel that arrives before the transfer runs is not lost	#4390 measured (hangs, exit 124)	CancelFromCreatedCompletes, CancelFromConnectingCompletes (#4395)
A3	One component owns the teardown of a transfer at any moment, and only that one announces its ending	#4360, #4396 reasoned	AnUnschedulableOperationAnnouncesNothingIfItHasAlreadyEnded (#4395)
A4	A session the client never registered still ends	#4393 measured (hangs)	InvalidUrlCompletes, UnparsableUrlsReleaseTheirOwnResources (#4395)

B. Terminal outcome

#	Invariant	Evidence	Pinned by
B1	Exactly one terminal outcome per SendRequest	#4360 measured (two events on the cancel path)	ACancelAfterTheResponseReportsOneOutcome (all four); CancelFromConnectingCompletes pins the current two so the number cannot drift silently
B2	An operation nothing will run still completes	#4390, #4393 measured (hang); #4404 reasoned	ASessionTheMultiHandleRefusesIsFinished, AClientWithoutAMultiHandleReachesATerminalOutcome (#4395, #4406)
B3	Once a terminal outcome is reported, nothing further goes on the wire	#4360 reasoned (gzip failure reports and continues)	nothing
B4	Every terminal state completes the caller's wait, including ReadError, WriteError and Destroyed	#4425 reasoned	nothing

C. Callbacks and re-entrancy

#	Invariant	Evidence	Pinned by
C1	Two callbacks for one request never run at the same time	#4408 measured (TSan on session_state_)	CancelFromConnectingWhilePollingCompletes (#4395), which now requires one concurrent callback where it used to require two
C2	A handler may finish or cancel its own session from a callback without waiting on itself	#4402 measured (hangs)	FinishSessionFromAnEventTheIoThreadDelivers, FinishInAsyncCallback (#4395)
C3	A handler may start another request on its session from a callback without destroying the operation it is inside	#4396 reasoned	ASecondRequestFromInsideTheResponseIsRefused (#4431), which refuses rather than allows
C4	No client lock is held across a user callback	#4389 measured (deadlock)	ResetMultiHandleWithASessionDoesNotDeadlock (all four)
C5	Anything the client reports through the global log handler is reported after the operation it describes has been finished	measured this week (hangs, exit 124)	AnAddRejectionIsFinishedBeforeItIsAnnounced (#4395)

C5 is the one I would not have predicted. OTEL_INTERNAL_LOG_ERROR calls application code synchronously, so a log line is a re-entrancy point with the same weight as a callback.

D. libcurl handle ownership

#	Invariant	Evidence	Pinned by
D1	Only the IO thread touches an easy handle once it is attached	#4375 measured, fixed in #4392	ACancelFromTheCallerThreadReportsCancelled (all four)
D2	A handle is removed from the multi handle before anything it points at is freed	#4391 reasoned	ARefusedRemovalKeepsTheHandleAndTheSessionItNames, AHandleTheMultiHandleNeverTookIsFreedNotStranded (#4405)
D3	The client knows which handles the multi handle holds	#4391 reasoned	TheAttachmentLedgerIsEmptyOnceARequestFinishes (#4405), and see the gap list
D4	Teardown follows the order libcurl documents: remove, clean the easy handles, clean the multi handle	libcurl manual; the current order is the reverse	nothing

On D4, I measured the current order on libcurl 8.14.1 under ASan and UBSan and saw nothing reported, and the manual says curl_multi_cleanup does not touch individual easy handles. So the present code appears to work while not following the documented order. That is an argument for fixing the order, not for relying on the observation.

E. Worker lifecycle

#	Invariant	Evidence	Pinned by
E1	At most one thread drives a given CURLM	reasoned: WaitBackgroundThreadExit clears the member before it joins, and MaybeSpawnBackgroundThread looks only at the member	nothing
E2	A failed curl_multi_poll or curl_multi_wait does not continue on the same multi stack	reasoned: the return is assigned and never read, while the curl_multi_perform return next to it drives a full recovery path	nothing
E3	A request that can never be scheduled still ends, without the caller having to cancel it	reasoned: no deadline exists anywhere in the file	partly, by cases that cancel first (#4406)

F. Allocation and failure paths

#	Invariant	Evidence	Pinned by
F1	Every libcurl allocation result is checked before use	#4404, #4434 reasoned	AFailedHeaderAllocationIsReported, AFailedMultiHandleAllocationIsReported, AHeaderListThatFailsPartWayThroughIsNotLost (#4406)
F2	A setup that fails frees what it allocated	#4397 measured (LSan)	nothing

G. Request lifetime

#	Invariant	Evidence	Pinned by
G1	The transfer reads request storage no caller can mutate	#4433 measured (TSan, three runs out of three)	MutatingASentRequestDoesNotReachTheTransfer (#4431)
G2	A session sends one request	#4396 reasoned	ASessionSendsOneRequest (#4431)

H. Retry

#	Invariant	Evidence	Pinned by
H1	A request the caller has withdrawn is not sent again	measured this week (ten runs out of ten)	FinishingFromARetryableResponseDoesNotSendAgain (#4395)
H2	A retry deadline is decided once per attempt	#4403 measured (eight calls, eight deadlines)	nothing
H3	A retry that cannot be re-armed ends the request rather than dropping it	reasoned: the retry path drops the removal result and erases the entry either way	nothing

What nothing holds today

These are the rows above with no case, plus one measured coverage gap.

• B3, a terminal outcome followed by bytes on the wire. This is the oldest of the reports and the only one where the client tells the caller it failed and sends anyway.
• B4, three terminal states that never complete an injected client's wait.
• D4, teardown order against the documented contract.
• E1, E2, E3, the whole worker lifecycle. All three are reasoned only, and E1 is the one I would want a case for most, because two threads on one CURLM is the failure mode [BUG] HttpOperation::Abort writes to the curl easy handle from the cancelling thread #4375 was about.
• F2, the setup-failure leak.
• H2, H3.
• The attachment ledger insertion. This one is measured rather than argued: I deleted the attached_handles_.insert(easy_handle) that records a handle the multi accepted, and the whole suite of 35 cases passed with nothing reporting. TheAttachmentLedgerIsEmptyOnceARequestFinishes asserts the ledger ends empty, and a ledger that was never written also ends empty. Whatever replaces this, an emptiness assertion will not hold it.

The seven questions, and what forces each one

Question	Forced by
Is a Session single-use?	#4396, #4433, G2. If it is, the transfer state can be the Session implementation and A1 becomes structural rather than a rule.
Does SendRequest consume or freeze the Request?	#4433 measured, G1
Which thread owns CURLM and the attached easy handles?	#4375, #4391, E1, D1 to D3
Exactly one terminal outcome per SendRequest?	#4360 measured, B1, B3
Which thread runs callbacks, and what may they call?	#4402, #4408, #4396, C1 to C5
What does a blocking method do when called from the IO or callback thread?	#4402 measured, C2
When does shutdown stop admitting work, and how does its deadline propagate?	E3, and #4359 for the same question one layer up in the Elasticsearch exporter

What I would do with this

Two things follow that I think are worth deciding before any code.

The first is that C1 and C5 together decide the shape of the whole thing. If callbacks for one request are serialised and log lines count as callbacks, then no internal state may be in flux while user code runs, and the state machine has to commit before it calls out. If callbacks are allowed to overlap, then every handler in the ecosystem has to be re-entrant, which the current interface never asked for.

The second is that the gap list is the acceptance suite for a replacement, not a backlog. Nine invariants have no case. A replacement that keeps the existing cases green tells us nothing about those nine, and they include the worker lifecycle and the one path that sends after reporting failure.

If this shape is useful I will turn the gap list into cases against the current implementation first, so they fail on main and can be carried to whatever replaces it. That way the acceptance suite exists before the design does, and it is written against behaviour rather than against either implementation.

[lalitb]

@owent @thc1006 - Thanks for initiating the discussion, and also collecting these reports and the invariant matrix. I agree the current implementation has issues, and several of these issues also affect the default build, without the async preview enabled.

However, before redesigning the current asynchronous implementation, I suggest that we step back and confirm what we actually need from the HTTP transport.

Concurrent HTTP export is still preview functionality and is disabled by default. In the normal configuration, OTLP/HTTP and Elasticsearch use the asynchronous curl client but then wait for completion, while Zipkin already uses the synchronous client. We therefore carry the background-thread, callback, and CURLM complexity even for effectively blocking operations.

The OTLP specification requires bounded export time and retry behavior, but parallel HTTP requests are optional. This raises the main design question:

Does the HTTP transport itself need to manage asynchronous concurrency, or could we use a simpler request/result model with optional concurrency managed at another layer?

The abstraction should remain independent of curl. Exporters should depend only on a clear transport contract, allowing curl to be one implementation alongside custom clients or platform-native implementations such as WinHTTP or NSURLSession.

I suggest preparing a short requirements-and-options document comparing the current model, a simpler implementation-neutral request/result transport, and - if transport-level concurrency is required - a single-owner CURLM event loop. It should consider throughput and connection reuse, cancellation and shutdown, responsibility for retry and concurrency, and compatibility with the installed HTTP interfaces and custom clients. It can also consider whether a future C++ baseline version (say switching to C++17/20) would simplify the implementation without making the design dependent on it.

We can review the document as a pull request before choosing the implementation direction. Until then, I suggest keeping the overlapping implementation PRs in draft while focused reproductions and independent correctness fixes continue.

The gzip failure fallthrough and #4425 can proceed independently of this design decision.

Edit - additional context from earlier work:

1. Compatibility scope: Since [BUILD] Install an explicit list of ext headers instead of the whole tree #4327, CMake installs the abstract HTTP interfaces and http_client_factory_curl.h, but not the concrete curl client and operation headers. Bazel's public //ext:headers target still includes every header under ext/include, so the concrete curl headers remain exposed there. The design should decide how to align these surfaces deliberately, including the impact on existing Bazel consumers.

2. Original purpose of the abstraction: HTTP client API for exporter developers #6 and Providing an abstract HTTP interface. #370 introduced the HTTP abstraction so that exporters would not depend directly on curl. The original discussion anticipated multiple implementations, including native Windows and Apple transports and user-provided clients. Remove curl dependency for Windows platform - Adding support for WinINet API #1145 and Evaluate removing curl dependency for Apple platform - Adding support for CFNetwork API #1146 were opened specifically to explore native implementations for those platforms.

3. Reason for the current asynchronous design: Earlier work considered a shared event-driven framework for asynchronous I/O in Support async networking #63 and Add support for event driven framework #1250. The curl-specific implementation later came through Could we add worker thread pool for BatchLogProcessor and BatchSpanProcessor? #1175, Do not waitForResponse in OtlpHttpClient::Export ? #1176, Using libcurl-multi to implement opentelemetry::ext::http::client::curl::HttpClient #1243, and Merge all commits of async-changes into main #1413 to improve throughput, reuse connections, support cancellation, and avoid creating one thread per request. These remain valid requirements to evaluate, but we should reconsider whether asynchronous execution belongs in a common event framework, the exporter, or each transport implementation.

[thc1006]

Thanks, and yes to stepping back. Taking your direct question first, because I think the evidence answers it.

The default configuration pays for machinery it does not use. OTLP HTTP and Elasticsearch both hand a request to the asynchronous client and then wait for it, Zipkin uses the synchronous one, and the async preview is off by default. Everything in the invariant matrix under publication, callback re-entrancy and worker lifecycle exists to make concurrency safe, and none of it is needed by a caller that is going to block anyway. So my answer is that transport level concurrency looks like an optional capability rather than the shape the interface should be built around. What I cannot answer from the reports is whether removing it costs throughput that someone depends on, which is the part worth measuring rather than asserting.

Three tracks, then.

I will put up the requirements and options document as a pull request. You asked for a short one and I will keep it short. The three options you named are the ones to compare, and I will not assume that HttpOperation has to go, that a Session is single use, or where retry belongs. The long form evidence stays where it is, linked rather than copied in: the matrix above already carries the bug to invariant mapping and says which rows are measured and which are only reasoned.

The two independent fixes go up as small separate pull requests, #4425 first and then the gzip fall through. Neither touches the curl client's structure and neither pre-commits the design.

#4395, #4405, #4406 and #4431 stay in draft with their scope frozen. I will not rebase them for the sake of looking current. Their deterministic reproductions and sanitizer cases are the part worth keeping whichever direction this goes, and the matrix marks which invariant each one holds.

Thank you for the history in your edit. #6, #370, #1175 and #1243 are the context I was missing, and knowing that the asynchronous work came in for throughput, connection reuse and avoiding a thread per request tells me what the document has to weigh rather than dismiss.

[owent]

I believe async concurrency is necessary. A few years ago, back when the HTTP exporter had no async support, we frequently ran into situations where the exporter couldn't keep up with the rate at which data was being produced (especially in high-volume logging scenarios), resulting in massive drops. After investigation, the bottleneck turned out not to be the server, but network latency (around 100ms). By that point, the BatchProcessor had already merged a large amount of data — each submission exceeded 4MB. The actual problem was that consumption simply couldn't keep pace with production. The drops only disappeared once we raised the concurrency to around 4.
My suggestion is that the HttpClient layer should adopt an async/concurrent design, and for cases that require synchronous behavior, provide an interface similar to ForceFlush to wait for completion. How it's implemented should be left to each concrete HttpClient implementation.
curl itself supports multiple IO event libraries (libuv, libevent, etc.), and its built-in one uses a Reactor model based on select/poll. The current implementation relies on the built-in event loop, which is fine in itself when the number of fds is limited. However, if we're going to have all HttpClient instances share a single CURLM handle, do we need to think about concurrent performance under high fd counts? Consolidating everything into one IO loop has another benefit: it avoids unnecessary thread creation and teardown.
As for WinHTTP — or a potential future C++ standard networking library — each can be integrated in whatever way suits its own implementation.