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.

[thc1006]

Thanks. That changes the framing, so I have taken it into the document rather than argue with it.

I had written #4458 around one fact: every default consumer blocks, so the background thread, the callback contract and the CURLM are paid for by callers that cannot observe any of it. Your workload is the other half, and it is evidence where mine was only an observation about the build. A collector that is not the bottleneck, roughly 100 ms of latency, batches the processor has already merged past 4 MB, and drops that only stop at around four in flight is exactly the case the specification's own arithmetic describes: max_concurrent_requests * max_request_size / (network_latency + server_response_time), with the request size already at the top of its range.

So concurrency is a capability the transport has to have. What the blocking callers want is narrower: not to pay for it. Those seem compatible to me, and the shape that gets both is to separate what is one thing today into three.

A portable one attempt operation over an immutable request, with an identity that does not move when a session is reused, a deadline, idempotent cancellation, and exactly one settlement. A bounded scheduler above it owning how many requests are in flight, admission and backpressure, retry and Retry-After, and what ForceFlush and Shutdown mean. A backend below it, where curl can be a single owner CURLM loop and WinHTTP or NSURLSession can use their own execution models without either being visible through the contract.

Blocking is then an adapter over the first layer: submit one operation and wait for that operation's own result until its own deadline. Not ForceFlush, which waits for everyone else's work as well and has open reports of its own.

Two things I had wrong and have corrected in #4458:

The specification does not use the same language for both transports, and that document is about the HTTP one. Concurrent unary calls are a SHOULD for gRPC. For HTTP the text is that a client MAY send requests using several parallel connections, with the maximum number SHOULD be configurable. I had been quoting the stronger wording at the weaker case.

And the first draft's recommendation ended with "the default configuration should not carry the machinery". That reads as proposing to remove the capability, which is not what I meant and is not defensible against your workload. It is gone.

One number I would rather surface than leave implicit. max_concurrent_requests defaults to 64 today and max_requests_per_connection to 8. Your workload needed about four. Neither figure came from a benchmark in this repository, and the only OTLP HTTP benchmark in the tree runs against localhost with a 1 ms timeout, which is the case where latency does not exist. I have added a matrix to #4458 covering round trip times from 0 to 250 ms, payloads from 4 KiB to 16 MiB, and 1 to 64 requests in flight, reporting dropped batches and whether ForceFlush and Shutdown returned the right answer within their deadlines. Until that exists, "about four" is a credible first hand observation rather than something this project can defend or regress against, and I would rather write it down as an open question than pick a default from a discussion thread.

On the loop itself, I would measure curl_multi_poll against a curl_multi_socket_action integration before anyone reaches for libuv or libevent, and treat one CURLM for the whole process as an option to measure rather than an assumption, because it moves connection reuse, fairness, fault isolation and shutdown ownership at the same time.

Sequencing is unchanged. #4395, #4405, #4406 and #4431 stay in draft and I am not expanding their scope, keeping their deterministic reproductions and sanitizer results as evidence for whichever contract wins. The two @lalitb separated out are done and ready for review: #4453 for #4425, and #4457 for the gzip fall through in #4360.

[owent]

Thanks for the earlier work and the benchmarks. I think this is well worth doing, and it is indeed more meaningful than the empirical figures we had before. The "about four" workload I mentioned earlier was an empirical number observed under a log volume of roughly 10–20K QPS; of course, it also depends on the size of the data being sent and the number of attributes.
Let me add some more context. The original max_concurrent_requests and max_requests_per_connection options were designed around the CURL implementation, which comes with its own low-level fd management and multiplexes multiple HTTP requests over the same socket. If connection reuse is not explicitly disabled for multiple requests and requests are simply added through the curl_multi_handle, the code looks concurrent, but underneath it is still queued and executed serially, so it does not actually achieve the goal of improving concurrency. These two options only control how concurrency is increased when requests back up, and cap the maximum amount of resources consumed (to prevent unbounded use of system resources). If we redesign the HTTP request flow, I don't think we need to be bound by curl's design constraints — as long as the goals of improving concurrency and controlling total resource usage are met, that should be sufficient.

[thc1006]

That settles the part I was most unsure of. If the replacement is not bound by the two options, then the requirement is effective concurrency and a bound on total resources, and their names and current semantics do not have to survive. I have taken that into #4458.

Reading them was worth doing, because they turn out to hold less than they look like they do.

max_concurrent_requests is not an admission limit. Export() builds the request, hands it to the HTTP client, and only then waits for the running session count to fall below it, so the request already exists in memory by the time the number applies.

max_requests_per_connection counts nothing per connection. Session::SendRequest computes reuse_connection = session_id_ % max_sessions_per_connection != 0, and every Nth session by global id then sets CURLOPT_FRESH_CONNECT and CURLOPT_FORBID_REUSE. It is a periodic rotation keyed on a counter that spans every session, not a quota.

And neither shapes what is on the wire, because the client never asks. Grepping ext/ and exporters/:

option	set anywhere
CURLMOPT_MAX_HOST_CONNECTIONS	no
CURLMOPT_MAX_TOTAL_CONNECTIONS	no
CURLMOPT_MAX_CONCURRENT_STREAMS	no
CURLMOPT_PIPELINING	no
CURLOPT_HTTP_VERSION	no

So the connection count, the stream count and the protocol are whatever libcurl negotiates, and the same two values can mean different things against a different server or a different system libcurl. That looks worth fixing whichever contract wins.

On the serial execution point, I would add one qualifier rather than disagree. It holds on one connection that cannot multiplex: HTTP/1.1 responses come back in request order and libcurl removed HTTP/1 pipelining, so one such connection carries one exchange at a time. It does not hold across several connections to the same host, or on HTTP/2 and HTTP/3 where one connection carries several streams under the server's own limits.

That is not a correction so much as the reason one number cannot express it. #4458 now separates admitted export operations, HTTP attempts, physical connections, streams per connection, and bytes retained for requests that have not settled. Today's max_concurrent_requests stands in for the first and, through the rotation, indirectly for the third.

The byte dimension is there because of your point about payload size and attribute count. 64 in flight at the 4 MB you described is 256 MB of request payload before responses, retries or compression buffers, so a count alone does not bound what a process holds.

I have put the 10 to 20K QPS into the benchmark plan as log volume, and the run will report HTTP requests per second alongside it with records and serialized bytes per request, so the two rates cannot be confused later. It will also read back CURLINFO_HTTP_VERSION and CURLINFO_NUM_CONNECTS rather than trust the option values, since a result at four in flight otherwise cannot be told apart from four connections, four streams on one connection, or four requests serialized behind each other.

What I think is now decided, and what I would still leave open:

Decided: a backend neutral one attempt contract over an immutable request with an identity, a deadline, idempotent cancellation and exactly one settlement; a bounded governor above it for in flight work, retained bytes, retry and what ForceFlush and Shutdown mean; one owner thread per CURLM, with the existing interface kept working through an adapter.

Open, and I would rather benchmark than assume: whether a CURLM is per client, per origin or shared; the connection and stream geometry; and every default, including whether four or sixty four is closer to right.

[owent]

Yes — and your reading of the code is sharper than my wording. max_concurrent_requests was never an admission limit (the request is already built and handed to the client before the count applies), and max_requests_per_connection is a periodic rotation keyed on the global session counter, not a per-connection quota. They were designed with curl's model in mind, but deliberately as a simple, roughly uniform way to express concurrency control for the OTLP/HTTP exporters — never an exact mapping, and we can't realistically force every IO backend to expose identical resource limits or concurrency semantics anyway.
What we can't drop is the goal behind them: a mechanism that lets concurrency scale up while keeping total resource usage bounded. As your numbers show, an in-flight count alone doesn't bound memory (64 × 4 MB before responses and retries), so the bounded governor in #4458, with both a count and a byte dimension, looks like the right shape.
On CURLM: I'd avoid multiple multi handles where we can. A CURLM can only be driven from its own poll loop — there's no way to wait on two multi handles in one call — so each extra CURLM in practice means another polling thread, and the connection cache is per multi handle, so separate CURLMs can't share connections. That said, I agree per-client vs per-origin vs shared should be settled by the benchmark, since a single process-wide CURLM also couples fault isolation and shutdown ownership across exporters. My preference: default to one shared CURLM, keep the geometry behind the contract, and let the curl_multi_poll vs curl_multi_socket_action numbers decide.

[thc1006]

Thanks — this resolves the two main questions I was trying to separate. I will treat the existing options as legacy curl compatibility controls, and I will update #4458 so that a bounded governor with both a count dimension and a retained-byte dimension is a requirement rather than an open question. I will also replace the current “count, bytes, or both?” question with the narrower questions that remain: what is counted — export operations, HTTP attempts, or running transfers — and at what point capacity is acquired.

On CURLM, I agree that one shared multi handle should be the preferred default baseline rather than merely one option. A multi handle owns its connection, DNS, and TLS caches, so sharing it is the cleanest path to cross-client connection reuse and avoids one polling loop per logical client in the curl_multi_poll design. I will keep one qualification in the document: this is a preferred curl-backend baseline, not part of the portable transport contract, and the sharing scope still needs to be validated because a process-wide multi handle also makes fairness, failure recovery, and shutdown ownership shared concerns.

To preserve isolation, I do not think the whole logical HttpClient should become a process-global singleton. I will describe the baseline as a reference-counted shared curl runtime beneath separate logical HTTP clients: one owner thread and one CURLM by default, with each logical client retaining its own operation namespace, admission lane, deadlines, cancellation, ForceFlush/Shutdown watermark, and legacy compatibility settings. Shutting down one exporter would settle and unregister only that client’s operations; the shared runtime and multi handle would be destroyed only after the final client releases them.

A shared runtime also requires a hierarchical governor. Per-client count and retained-byte limits provide isolation and backpressure, while runtime-wide count, retained-byte, and connection limits ensure that several clients cannot each consume the entire process budget. The benchmark therefore needs to measure not only aggregate throughput and connection reuse, but also fairness between trace, metric, and log clients, memory retained during retries, independent ForceFlush and Shutdown, and the blast radius and recovery behavior of a multi-level failure.

I will update #4458 accordingly, use one shared CURLM as the preferred default benchmark hypothesis, and retain per-client or per-origin multi handles as comparison cases rather than equal recommendations. I will also compare curl_multi_poll with curl_multi_socket_action, rebase the PR, and ask you and @lalitb to review the resulting supported/open decision list before any redesign implementation starts.

[thc1006]

One of the seven open questions has a measurable answer on main today, so here it is rather than as another issue.

What a callback may call is not a documentation question. A handler that starts another request on the same session, from a callback that session dispatched, destroys the operation whose member function is running the dispatch. AddressSanitizer on main at 58ed80d9, single threaded, deterministic.

The free:

HttpOperation::SendAsync
  HttpOperation::DispatchEvent            <- Connecting
    Handler::OnEvent
      Session::SendRequest
        std::unique_ptr<HttpOperation>::reset
          HttpOperation::~HttpOperation   <- destroys the operation still on the stack

and then the use, back in the frame that dispatched:

Session::SendRequest
  HttpOperation::SendAsync
    HttpOperation::DispatchEvent          <- WRITE of size 1 on the freed object

Session::curl_operation_ is a std::unique_ptr<HttpOperation> and Session::SendRequest assigns into it with reset at http_client_curl.cc:203, so the handler's call frees the object its own caller is still inside. Nothing on this path can refuse: SendRequest has no way to know a dispatch is in progress, and the operation has no way to keep itself alive.

The probe is about sixty lines: a handler whose OnEvent calls SendRequest once when it sees Connecting, against the test server the existing cases use. I have not attached it because it is a mechanical thing to write from the description, and I would rather not have a reproducer for this sitting in a public comment than have one. Happy to send it privately or as a draft pull request if that is more useful.

Two notes on scope, so this is not read as more than it is.

It is not a race. One thread, no timing, it reproduces on the first attempt. That distinguishes it from most of what is in the invariant matrix above, where the entries marked reasoned rather than measured are the ones I could not put a case around.

It is also not new. #4395 adds one more instance of the same shape, on the path where a refused session is told nothing will run its request, and I have said so in that pull request rather than trying to solve the class inside it. The shape belongs to the contract, not to any one call site.

For the redesign, what this argues is narrow. Whichever answer the callback question gets, permitted or refused, it has to be enforceable at the point of call rather than written down. A Session that owns its operation by unique_ptr and swaps it in place cannot enforce either answer, because by the time it could check, the object it would have checked for is gone.