[thc1006]

Fixes #4390.
Fixes #4393.
Fixes #4408.
Fixes #4402.

An operation could be given a promise and then never handed to the IO thread, and then nobody was in a position to complete it. FinishSession() blocked forever.

Three ways in, and each one hangs on main today:

A handler cancels from the Created or Connecting event	SendAsync dispatched the event and only then reset is_aborted_, so the cancel was thrown away, while CleanupSession() had already taken the session out of the client. ScheduleAddSession then erased the pending abort for good measure
The URL does not parse	CreateSession hands back a session it never registered and never gave an id. CURLOPT_URL is not checked when it is set, so Setup() succeeds and the request reaches ScheduleAddSession(0), which nothing can find
curl_multi_add_handle rejects the handle	The result was dropped, so the operation sat there with nothing to run it

They are one invariant with three holes in it: an operation that never gets scheduled still has to finish.

What changed

The flags and the cancel route are published before the first event, so a handler cancelling from it is not overwritten by the rest of SendAsync. The future is published after the event, deliberately: a handler calling FinishSession() from Connecting would otherwise wait on a transfer that has not been scheduled yet, with SendAsync unable to schedule it because it is inside that handler.

ScheduleAddSession now reports whether the session was still registered, and SendAsync finishes the operation itself when it was not, or when the event cancelled it. doAddSessions does the same for the ones libcurl rejects, outside sessions_m_, since finishing one reaches the caller's handler and a handler that cancels takes that lock again.

All three say the same thing to the handler, a failed create carrying the reason, through one method rather than the same three lines in two places. The one libcurl refuses used to arrive as Cancelled, which the enum calls "(manually) cancelled" and which both exporters print in those words, for a request nobody cancelled and without the reason libcurl gave. Nothing behaves differently for the change: CreateFailed and Cancelled both set need_stop in the OTLP HTTP handler and both end the wait in the Elasticsearch one, so what moves is the message.

There is one ordering subtlety worth pointing at. Between publishing the future and scheduling the add, the IO thread can already have torn the operation down and found no future to complete, so SendAsync rechecks is_cleaned_ and settles it with an exchange. Whichever side gets there first, the value is set exactly once.

That same window had a second defect in it, raised by @lalitb in review. DispatchEvent stored session_state_ after the handler returned, so a handler cancelling from the first event on a client that is already polling had the IO thread storing a Cancelled of its own at the same time, and the store after the handler returned overwrote it. ThreadSanitizer reports it on this branch before the change, five runs out of five. The store now happens before the handler runs, which orders it ahead of the cancel through sessions_m_ and session_ids_m_, and the member is a std::atomic for the paths where a third thread cancels instead of the handler. SessionState is a std::uint8_t enum, and on the toolchains this was built with std::atomic of it stays one byte with alignment one, so the layout does not move. The standard promises neither, so both are static_asserts in http_operation_curl.cc rather than a claim here: a toolchain where they do not hold fails the build instead of changing the layout quietly. Worth being exact about whose layout, since I was not. ext/CMakeLists.txt installs four headers from ext/http/client and http_operation_curl.h is not one of them, so a CMake consumer never sees this type. //ext takes hdrs = glob(["include/**/*.h"]), so a Bazel consumer does, which is why the guard is worth keeping rather than dropping. Lock freedom is a separate property and nothing here checks it, since is_always_lock_free is C++17 and this still builds at C++14.

What a second review round found

Three things, all in the code this branch adds rather than in what it inherited, and each one
is held by a case that fails without the fix.

FinishUnscheduled cleaned up and then dispatched the terminal event. The operation holds the
caller's handler as a bare pointer and the only strong reference to it is the one
Session::SendRequest captured in the completion callback, so the cleanup that releases that
callback is what frees the handler, and the event that followed read it. AddressSanitizer
reports a heap-use-after-free at the OnEvent call reached from FinishUnscheduled. Nothing in
the suite caught it, because every case that reaches this path either keeps its own reference to
the handler or hands SendAsync a completion capturing none, so the ownership that breaks was
never assembled. The event goes first now. AnUnscheduledSessionTellsAHandlerNothingElseHolds is
the only case in the file that leaves the completion callback as the sole owner, which is what
every real caller of SendRequest arranges, and it records its own event against its own
destruction into state the case owns, so it fails without a sanitizer as well as with one.

AsyncData::callback_thread was a plain std::thread::id written on the thread running a
callback and read by Finish() and by the destructor on whichever thread called them. Its
accessors put a standalone atomic_thread_fence either side of a plain load and store, which
orders nothing: fences synchronize through atomic operations on a common object and there was
none. ThreadSanitizer stayed quiet because both accessors carried
OPENTELEMETRY_SANITIZER_NO_THREAD, and the fences were compiled out under that same sanitizer,
so what it was told to ignore was a bare unsynchronized access. The callback scope already
answers the question the member existed to answer and answers it without shared state, so the
member, its accessors and the friend declaration are gone. Finish() consulted the scope before
it ever reached the member, which made the second check unreachable as anything but true.

FinishUnscheduled is private now, with the client as a friend. It forces a terminal state,
cleans up, dispatches, fulfils the promise and hands the easy resource back, and the client is
the only thing that ever discovers a request nothing will run. ext/CMakeLists.txt does not
install this header, but //ext globs it, so a public method here is source API for a Bazel
consumer.

The refusal was reported before the operation it describes was settled

A third reading of doAddSessions found the loop in the wrong order. It logged the refusal, then read GetOperation(), then finished it.

The log goes through the global handler, which is replaceable application code and, unlike everything else on this path, is not inside a callback scope for that operation. A handler answering with Session::FinishSession() reaches HttpOperation::Finish(), which is not InsideCallbackFor(this) and so waits on a promise only the FinishUnscheduled below it can fulfil, on the same thread. Reading the operation after the log is the other half: a handler calling SendRequest() from there swaps the operation, and the loop then finishes the new one and leaves the refused one unsettled.

FinishUnscheduled already dispatches its event before it cleans up, and its comment says why that matters: a handler calling FinishSession() from that event is inside a callback for the operation, so Finish() returns rather than waiting. Reporting after it is what brings the log handler under the same protection. Nothing else moved.

ARejectedSessionIsSettledBeforeItIsReported installs a global log handler that records whether the terminal event has arrived by the time the refusal is logged, and puts the previous handler and level back through a scope guard. With the report moved back above the settlement it fails at the ordering assertion; with the change it passes. Forty cases in the binary, all passing.

Worth naming because it nearly went in wrong: the first version of that assertion asked only whether the handler had seen any event. SendAsync dispatches Created during setup, so that is true in both orders, and the case passed with the defect deliberately put back. It asks for CreateFailed now.

What this path shares with the rest of the client, and does not fix

Telling a refused session that nothing will run its request means dispatching a terminal event, and a handler is free to answer it by starting another request on the same session. Session::curl_operation_ is a std::unique_ptr<HttpOperation> and Session::SendRequest assigns into it with reset, so that answer frees the operation whose member function is dispatching the event. AddressSanitizer reports a heap use after free, single threaded and on the first attempt.

This is not introduced here. The same two stacks reproduce on main at 58ed80d9 through Connecting, which is dispatched from inside SendAsync and has nothing to do with this change. I have put the measurement on #4448, where the question of what a callback may call is one of the open contract items, rather than opening an issue for it.

I looked at fixing it inside this pull request and decided not to. Taking ownership of the operation before finishing it removes the use after free, and it also makes FinishSession() on another thread return without waiting, because that wait is keyed on the session still holding an operation rather than on there being outstanding work. FinishFromAnotherThreadWaitsForTheTerminalEvent fails on that change, and it is right to. Both behaviours follow from the same ownership model, so choosing between them is the contract decision rather than something this change should settle on its own.

The part I looked hardest at

This makes the completion callback run on the calling thread in those paths, where it always ran on the IO thread before. I went through every in-tree caller rather than assume:

• OtlpHttpClient::addSession closes its session_manager_lock_ scope before SendRequest, which the comment there says is deliberate, and the lock is recursive anyway. The handler's Cancelled reaches Unbind and ReleaseSession, and ReleaseSession moves the session to gc_sessions_ rather than calling FinishSession, so nothing waits.
• The Elasticsearch exporter holds no lock across either of its SendRequest calls, and the synchronous waitForResponse() waits on a predicate, which is exactly the case where the completion is recorded before the waiter arrives.
• A handler calling FinishSession() from inside a callback this operation dispatched is covered by a callback scope. Entering one records the operation on the calling thread, and Finish() returns rather than waiting on a completion that thread has not published yet. That covers the completion callback and every event, which is what closes [BUG] FinishSession() from an OnEvent handler deadlocks the curl client's background thread #4402.
• This is not a new shape either way. Session::SendRequest already calls callback->OnEvent(CreateFailed, "") inline on the calling thread when SendAsync fails.

The behaviour change a user could notice is that a cancel from Connecting is now honoured. It used to be dropped and the request went out regardless.

Tests

CancelFromCreatedCompletes, CancelFromConnectingCompletes and InvalidUrlCompletes. All three hang rather than fail without the change: timeout 25 gives exit 124 and zero cases finished, and they pass in about 500 ms with it. Worth knowing for a bisect, since a regression here looks like a CI timeout rather than a red assertion.

CancelFromConnectingWhilePollingCompletes is the fourth, and the only one that puts an IO thread there before the event runs. A client spawns one only after SendAsync returns, so it warms the client with a completed request first, then cancels from Connecting and stays in the handler until the IO thread has entered it too. Two things follow from that. It reports the state store race without the change and none with it, five runs out of five each way. And instrumenting both tail branches of SendAsync and running the whole binary shows it is the only thing that reaches the is_cleaned_ recheck, which the three cases above never touch.

What it stays in the handler on took a second look. It waited on inside_events_, which is raised on the way into an event and lowered on the way out, and the event the IO thread dispatches there is a few atomics long, so sampling it every millisecond almost never caught it at two: four runs measured 30005, 30509, 30006 and 30015 ms and all passed, because the count the assertion reads is written by the thread that creates the overlap and never goes down. Waiting on that one instead leaves as soon as the overlap has happened, and the case goes from 30006 ms to 511 ms, ten runs out of ten, with the binary down from about 53 seconds to 23.5. The shorter wait is also the one that works: against a faithful revert of what this branch changed about session_state_, a plain member again and the store back after the handler, ThreadSanitizer reports the race five runs out of five with it and none at all with the thirty second version, which spends that long and a great many reads of the same address between the two writes. A bound that does expire now means the overlap really did not happen, so it is recorded and checked rather than left to read as one dispatched event.

ASessionTheMultiHandleRefusesIsFinished is the third root cause, and nothing reached it before. SendAsync does the whole async setup and Session::SendRequest is what starts the worker, so the case builds the operation, sends it, and drives doAddSessions itself on one thread. What refuses the add is a seam: the client reaches curl_multi_add_handle through a member, and a case replaces it with one that returns the code it wants. It used to swap the client's multi handle for a null one, which works, but it works by handing libcurl a handle its own documentation says not to use after curl_multi_init returns null, so what was under test was libcurl's defence against invalid input rather than a refusal it promises. The manual only promises that a non-zero return means the add failed. Reverting that finish to the plain one it had fails the case twice over, on the state it reports and on the absence of a cancel.

Its overlap counters are relaxed on purpose. With acq_rel the two threads synchronize through the counter itself, which orders the very stores the case exists to keep unordered, and ThreadSanitizer goes quiet. A deliberate unsynchronized counter used as a control confirms it: the acq_rel build reports neither, the relaxed build reports both.

Checks

Re-measured on 9325e30e, this branch merged with main at ac7b87a8. Seven configurations, each configured and built on its own, each run the whole binary alone and once:

	cases	result
OTELCPP_WITH_STL=CXX14, OTELCPP_MAINTAINER_MODE=ON	53	all pass, no build warnings
ThreadSanitizer	53	all pass, nothing reported
AddressSanitizer, ASAN_OPTIONS=detect_leaks=1	53	all pass, nothing reported
UndefinedBehaviorSanitizer, -fno-sanitize-recover=all	53	all pass, nothing reported
OTELCPP_WITH_STL=CXX20	53	all pass, no build warnings
OTELCPP_WITH_THREAD_INSTRUMENTATION_PREVIEW=ON	53	all pass, no build warnings
OTELCPP_WITH_OTLP_HTTP_COMPRESSION=ON	56	all pass, no build warnings

The eighth configuration the earlier run used set WITH_OTLP_RETRY_PREVIEW, and #4668 has since deleted that flag, so it is gone rather than replaced.

A sanitizer that was never linked in reports nothing in exactly the way one that finds nothing does, so the three sanitizer rows are checked and not assumed: nm counts 30 __tsan_ symbols in the ThreadSanitizer binary, 35 __asan_ in the AddressSanitizer one and 13 __ubsan in the third, none of them present in the other builds. The last two rows are checked the same way, by the three gated case names appearing in the compression binary and in no other, and by the preview macro appearing in that build.ninja and not in the default one.

53 where this section used to say 38, and none of that is this branch: the file holds 53 cases, 40 of them main's and 13 this branch's, and main grew the file while this sat. Of the 53, three are behind ENABLE_OTLP_COMPRESSION_PREVIEW, which is the last row, and one is disabled on main.

Each run reports its own counts rather than going through CTest, because a case an #ifdef compiles out still gets a CTest entry and passes as zero tests run. Each of them carries a CTest timeout of 120 seconds, read back from ctest --show-only=json-v1, because what these cases catch fails by hanging: without a bound a regression stops the job rather than reporting, which happened three times while this was being written. The whole binary takes 31.3 seconds, and the two slowest cases are the ones that wait out a request timeout on purpose, at 5007 and 5005 ms.

One number in here is pinned as it is rather than as it should be. Cancelling once the transfer belongs to the IO thread delivers two terminal events, one from Cleanup and one from the completion callback, and CancelFromConnectingCompletes requires exactly two so that a change to it is visible. One is the goal, and that is #4360, which this change does not close. A cancel from Created is reported as a failed create for the same reason: the event is dispatched from the constructor, before the Session holds the operation, so the cancel never reaches it. Moving the first events out of the constructor is the startup ordering work, not something to add here, and both are pinned rather than smoothed over. Under ThreadSanitizer the whole binary reports zero races with the change, and the new case reports the state store race five times out of five without it. Clean under OTELCPP_MAINTAINER_MODE=ON and clang-format 18.1.8, both re-run on the merge. clang-tidy and include-what-you-use are left to the CI jobs on this head rather than quoted here, since their limits moved while this sat and a local figure from before that does not compare.

Related

#4392 fixed the cross thread write to the easy handle on the same file and has landed, and this is rebased on it: the resolution keeps its CURLOPT_NOPROGRESS placement and its atomic AsyncData::session alongside the ordering here. #4402 is closed here. That is a correction to what this description said before: I had scoped the claim to the terminal event this client generates itself, but the mechanism is not specific to it. A callback scope wraps every dispatch and Finish() consults it, so a handler is covered on the events the IO thread delivers during a transfer as well, which is exactly the case the issue describes. FinishSessionFromAnEventTheIoThreadDelivers is that case, taken from the issue and made deterministic by waiting on a condition rather than sleeping; take the check out of Finish() and it hangs. The alternative was to narrow the mechanism back to the unscheduled path, and that only puts the deadlock back for everyone else, so I would rather claim what the code does. FinishFromAnotherThreadWaitsForTheTerminalEvent holds the other direction, that an outside Finish() waits for the terminal event and the completion callback rather than for the promise alone.

#4405 changes teardown after Cleanup() has handed the easy resource back to the client, so the two are independent, but whichever lands second needs a real rebase rather than a metadata one because both touch the same removal path. Measured today against each other, they conflict in CHANGELOG.md, http_client_curl.cc and curl_http_test.cc. doAddSessions is one thing wanted from two ends, since this branch finishes a session curl_multi_add_handle rejected and #4405 keeps that same handle off its attachment ledger, and it resolves as the union. The test file wants judgement rather than a merge tool: both branches add cases at the same anchor whose first four lines are identical, so a mechanical union folds them into one function with two TEST_F headers stacked and a variable declared twice, and it does not compile.

This paragraph used to name PerformCurlMessage as a third conflict, because this branch adds a function immediately above it and #4405 changed what it returns. #4632 has since landed that return value change on main, so #4405 no longer carries it and the two branches now agree there. It also used to give a commit count and a combined suite result from a stacking experiment; both described a tree that no longer exists, so they are gone rather than restated.

About the lines Codecov marks

Codecov reports 5 lines of this change without cover. Two are the guards in
doAddSessions() for a session whose operation or easy handle has gone before
the queue drains; they were evaluated 39 times each and never taken, and
reaching them needs a cancel to land inside that window.

The other three are the wait in ~HttpOperation() for a transfer that is still
in flight. Those are worth naming rather than covering. The only route to them
is a second SendRequest() on a session whose first is still running, since the
client holds a strong reference to every registered session and nothing else
destroys an operation in Connecting, Connected or Sending.

#4431 has since landed, so that route is closed on main: SendRequest()
returns after reporting CreateFailed rather than replacing the operation. By
the argument above those three lines are now unreachable, and this branch is
merged with a main that carries the guard.

I have not removed them here, and the reason this paragraph used to give has expired. It said the removal belonged after #4618 landed. #4618 merged on 26 September and 4fe97765 is an ancestor of this branch's head, so there is no competing version of that hunk left to avoid.

What remains is narrower. #4618 kept the wait rather than dropping it, so the line survives in changed form, now as async_data_ && async_data_->result_future.valid() && !CallbackScope::InsideCallbackFor(this) in HttpOperation::Cleanup. The coverage figures this paragraph used to quote were measured on a head from before that merge and the lines have moved since, so I am not restating them as current. Taking the three lines out is a change worth reviewing on its own rather than folding in here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.74%. Comparing base (ac7b87a) to head (9325e30).

Files with missing lines	Patch %	Lines
ext/src/http/client/curl/http_client_curl.cc	93.75%	2 Missing ⚠️
ext/src/http/client/curl/http_operation_curl.cc	93.55%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4395      +/-   ##
==========================================
+ Coverage   86.69%   86.74%   +0.06%     
==========================================
  Files         527      527              
  Lines       20902    20933      +31     
==========================================
+ Hits        18119    18157      +38     
+ Misses       2783     2776       -7     

Files with missing lines	Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h	95.13% <ø> (ø)
...lemetry/ext/http/client/curl/http_operation_curl.h	90.91% <100.00%> (ø)
ext/src/http/client/curl/http_client_curl.cc	95.42% <93.75%> (+1.32%)	⬆️
ext/src/http/client/curl/http_operation_curl.cc	65.67% <93.55%> (+0.74%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thc1006]

One thing on this page that a reviewer would otherwise have to dig for.

The two red jobs are both #4265

CMake gcc 14 (maintainer mode, abiv2) and CMake clang 18 (maintainer mode, sync) each fail on one test and pass everything else, and it is the same test with the same number in both:

ext/test/http/curl_http_test.cc:881: Failure
Value of: cost < std::chrono::milliseconds{20}
  Actual: false      cost ms: 247

That is #4265, which @dbarker opened on 17 July and @yogarajalakshmi-s is on. main hit it on 7 August as well.

This change does touch session teardown, so I did not want to leave it at "known flake". Same case, same build configuration, 25 runs on main and 25 on this branch:

	n	median	max	over 20 ms
main	25	0 ms	0 ms	0
this branch	25	0 ms	0 ms	0

The assertion is a 20 ms wall clock budget, so an idle box gives 0 and a loaded runner gives 247. Nothing here moves it.

Edited 16 Aug. This used to carry a second section on the eleven uncovered patch lines. It is gone rather than kept, because the comment below supersedes its numbers and one of its claims stopped being true: it said a test could not make libcurl reject a handle without more machinery than the branch was worth, and the branch now has that seam, so a case names the code it wants back instead of arranging an unusable multi handle.

[lalitb]

Thanks, the callback publication reads correct now.

One more concern on this line, from the same window: if the client already has an IO thread, a handler can call CancelSession() from Connecting, and Cleanup() can dispatch Cancelled on the IO thread before this Connecting callback returns. That also makes both DispatchEvent() calls write session_state_ concurrently.

The current test uses a new client, so the background thread starts only after SendAsync() returns and does not cover this interleaving. Could we serialize cleanup with the initial event dispatch and add a test that starts the IO thread before cancelling from Connecting?

[thc1006]

Measured it, and both halves hold.

The interleaving happens. On a client that has already completed one request, a handler cancelling from Connecting is still inside OnEvent when the IO thread runs Cleanup() and dispatches its own Cancelled. I held the handler open and counted threads inside it: two, five runs out of five. Drop the warming request and it is one, which is what every case in the file does today.

The stores race. ThreadSanitizer on that scenario, on this branch before the change, five runs out of five:

  Write of size 1 by main thread:
    #0 HttpOperation::DispatchEvent(...)  http_operation_curl.cc:415
    #1 HttpOperation::SendAsync(...)      http_operation_curl.cc:1466
    #2 Session::SendRequest(...)          http_client_curl.cc:209

  Previous write of size 1 by thread T2:
    #0 HttpOperation::DispatchEvent(...)  http_operation_curl.cc:415
    #1 HttpOperation::Cleanup()           http_operation_curl.cc:536
    #2 Session::FinishOperation()         http_client_curl.cc:269
    #3 HttpClient::doAbortSessions()      http_client_curl.cc:808

Past the undefined behavior the later store wins, so GetSessionState() finished at Connecting for an operation that had been cancelled.

What changed. DispatchEvent stores before it calls the handler, and session_state_ is now a std::atomic. Storing first is what fixes the value: the store is then ordered ahead of the cancel, which goes through sessions_m_ and session_ids_m_, and the IO thread takes session_ids_m_ before it can reach Cleanup(). The atomic covers the paths with no such handoff, where a third thread cancels rather than the handler. SessionState is a std::uint8_t enum and std::atomic of it is one byte with alignment one and lock free, so the layout of the installed type does not move.

I went through the other readers before moving the store. Nothing reads GetSessionState() from inside its own event, and every internal reader, ~HttpOperation, Cleanup, the progress and debug callbacks, and PerformCurlMessage, reads it after a dispatch has already returned, where both orders give the same value. The two synchronous Get and Post paths run on one thread with no handler at all.

The test. CancelFromConnectingWhilePollingCompletes warms the client, cancels from Connecting, and stays in the handler until the IO thread has entered it too, so the overlap is deterministic rather than lucky. Without the change it reports the race five runs out of five, with it none, and the whole file is clean under ThreadSanitizer. It is also the only thing in this file that reaches the is_cleaned_ recheck at the end of SendAsync, which closes the coverage gap I flagged on the Codecov comment. I instrumented both tail branches and ran the whole binary: the recheck was entered exactly once, by this scenario, and the three cases already on the branch all took the other branch instead.

One detail worth knowing for anyone who touches it. My first version counted the overlap with acq_rel atomics and ThreadSanitizer went quiet. The IO thread's release and the caller's later read-modify-write on the same counter synchronize with each other, which orders the two state stores and hides exactly what the case exists to catch. The counters are relaxed now. I confirmed that with a deliberate unsynchronized counter on the same line as a control: the acq_rel build reported neither it nor the real race, the relaxed build reported both.

I had opened #4408 for this before deciding to fix it here, since it reproduces on main independently of this change. This branch closes it, and I have corrected the claim there that the member could not be made atomic without moving the layout.

[thc1006]

One correction to what I wrote above, because I stated it more broadly than it holds.

I said every internal reader takes GetSessionState() after a dispatch has already returned. There is one that does not have to. ~HttpOperation switches on it to decide whether to wait on the future, and a handler that destroys its own operation from inside an event, which is what Session::SendRequest does when a session is reused and is the shape of #4396, runs that switch while the dispatch is still in the handler. Its case list is Connecting, Connected, Sending, and it does not include Created, so storing first can change which side of it that path lands on.

I measured it rather than reason about it. Instrumenting the destructor to record every state it observes and running the same set of cases both ways:

store after the handler:   Response 28  ConnectFailed 11  SendFailed 2  Created 2  Connecting 1
store before the handler:  Response 29  ConnectFailed 11  SendFailed 1  Created 2  Connecting 1

The one that moves between SendFailed and Response is run to run variation in a suite that talks to a socket, not the ordering. So nothing the suite exercises changes, and the only path where the two could differ is a handler destroying its own operation, which is a use after free today with or without this.

[thc1006]

Update on the eleven uncovered patch lines I wrote about earlier, now that the polling client case is on the branch. Measured with the same all-options-abiv2-preview configuration ci/do_ci.sh code.coverage uses, intersecting the uncovered lines with the lines this branch adds.

	before	now
http_operation_curl.cc	3	0
http_client_curl.cc	8	8

The three were the is_cleaned_ recheck at the end of SendAsync, which I had said nothing in the suite reached. CancelFromConnectingWhilePollingCompletes reaches it, and it is still the only thing that does: instrumenting both tail branches and running the whole binary shows the recheck entered exactly once, by that case.

Of the eight left, three are the continue guards at the top of doAddSessions, unchanged except for the indentation of wrapping that loop in a scope, so the diff counts them as added. They were not executed before this change either.

The other five are the curl_multi_add_handle rejection branch, and I owe a better answer than the one I gave. I said a test cannot make libcurl reject a handle. What I have since measured is more specific than that. A client whose multi handle is null does reject, but never in doAddSessions: the IO loop calls curl_multi_perform first, that rejects the null handle, the mc != CURLM_OK branch resets it, and doAddSessions then runs against a working handle. I confirmed that on a client built with curl_multi_init forced to fail, with the internal log captured in the same run to prove the handle really was null: the request completed normally, three runs out of three.

So reaching that branch needs the handle to be taken away and doAddSessions driven directly, which the HttpClientTestPeer friend added by #4394 can do in about fifteen lines. The reason I have not sent it is that the peer would have to win a race against the IO loop's reset, and a case that covers the branch only when it wins is a flaky case rather than a test. If you would rather have that branch covered than have the suite stay deterministic, say so and I will add it with the peer holding multi_handle_m_ across the call, which makes the reset wait rather than race.