Describe your environment
main at 11fa0db0, Linux, GCC 14.2, -DWITH_ELASTICSEARCH=ON -DWITH_ASYNC_EXPORT_PREVIEW=ON. AsyncResponseHandler only exists under ENABLE_ASYNC_EXPORT.
Steps to reproduce
Two exports through a fake HttpClient injected via the public constructor. The first session reports two terminal events, which is what the curl path does on a setup failure (see below). The second never calls back.
int session = 0;
auto client = std::make_shared<FakeHttpClient>([&session](http_client::EventHandler &handler) {
if (++session == 1)
{
handler.OnEvent(http_client::SessionState::ConnectFailed, "");
handler.OnEvent(http_client::SessionState::CreateFailed, "");
}
// session 2 never calls back
});
ExportOnce(exporter); // session 1
ExportOnce(exporter); // session 2, still in flight
const bool flushed = exporter.ForceFlush(std::chrono::milliseconds{20});
What is the expected behavior?
flushed == false. One of the two sessions has not finished.
What is the actual behavior?
[repro] ForceFlush with one session still running returned true
Immediately, in 0 ms. finished_session_counter_ reached 2 from a single session, so finished >= running was already true.
I ran this on top of the fix in #4337, so this is not the timeout defect reported in #4336. The counters are monotonic and compared with >=, which makes the overshoot permanent: every later flush also returns one session early.
Additional context
AsyncResponseHandler accounts a session's completion neither at most once nor at least once.
More than once. OnResponse and every handled terminal OnEvent call result_callback_ directly, and the callback increments finished_session_counter_. There is no guard. Two in-tree paths produce two terminal callbacks for one session:
HttpOperation::SendAsync dispatches ConnectFailed and returns non-CURLE_OK when Setup() fails (ext/src/http/client/curl/http_operation_curl.cc, the code != CURLE_OK branch). Session::SendRequest then takes its else branch and dispatches CreateFailed for the same handler (ext/src/http/client/curl/http_client_curl.cc). This is deterministic, not a race.
- The async completion lambda in
Session::SendRequest uses two independent if blocks rather than else if, so an operation that was aborted and also has a response fires OnEvent(Cancelled) and OnResponse.
Fewer than once. ReadError, WriteError and Destroyed fall into default: break, so result_callback_ never fires and the session is never counted as finished. The synchronous handler treats all three as terminal, Destroyed since #4298 and the other two in #4331, so the two handlers in the same file currently disagree.
Suggested shape
Route every outcome through one idempotent step:
void CompleteOnce(sdk::common::ExportResult result) noexcept
{
bool expected = false;
if (completed_.compare_exchange_strong(expected, true, std::memory_order_acq_rel))
{
result_callback_(result);
}
}
with OnResponse and every terminal OnEvent going through it, and the switch enumerating all fifteen states so -Wswitch catches a future addition rather than a default: swallowing it. The guard is also what makes widening the terminal set safe, since it removes the double-completion objection to treating ReadError, WriteError and Destroyed as terminal here the way the synchronous handler does.
Worth covering: each of the three ignored states finishing one session as a failure, a response followed by another terminal event counting once, a cancellation followed by a late response counting once, and a duplicate terminal event counting once.
Two things I would rather flag than fold in. The two if blocks and the non-returning gzip failure branch in Session::SendRequest are shared HTTP client behaviour, so fixing them there would remove the double-fire for every consumer, but that is a separate change. And an exporter side guard is still warranted either way, because EventHandler is a public extension point and an injected client is free to emit whatever it likes.
I am happy to open the PR for the exporter side. It overlaps #4297 and #4337 in the same file, so I would rather land those first unless you would prefer it sooner.
Describe your environment
mainat11fa0db0, Linux, GCC 14.2,-DWITH_ELASTICSEARCH=ON -DWITH_ASYNC_EXPORT_PREVIEW=ON.AsyncResponseHandleronly exists underENABLE_ASYNC_EXPORT.Steps to reproduce
Two exports through a fake
HttpClientinjected via the public constructor. The first session reports two terminal events, which is what the curl path does on a setup failure (see below). The second never calls back.What is the expected behavior?
flushed == false. One of the two sessions has not finished.What is the actual behavior?
Immediately, in 0 ms.
finished_session_counter_reached 2 from a single session, sofinished >= runningwas already true.I ran this on top of the fix in #4337, so this is not the timeout defect reported in #4336. The counters are monotonic and compared with
>=, which makes the overshoot permanent: every later flush also returns one session early.Additional context
AsyncResponseHandleraccounts a session's completion neither at most once nor at least once.More than once.
OnResponseand every handled terminalOnEventcallresult_callback_directly, and the callback incrementsfinished_session_counter_. There is no guard. Two in-tree paths produce two terminal callbacks for one session:HttpOperation::SendAsyncdispatchesConnectFailedand returns non-CURLE_OKwhenSetup()fails (ext/src/http/client/curl/http_operation_curl.cc, thecode != CURLE_OKbranch).Session::SendRequestthen takes itselsebranch and dispatchesCreateFailedfor the same handler (ext/src/http/client/curl/http_client_curl.cc). This is deterministic, not a race.Session::SendRequestuses two independentifblocks rather thanelse if, so an operation that was aborted and also has a response firesOnEvent(Cancelled)andOnResponse.Fewer than once.
ReadError,WriteErrorandDestroyedfall intodefault: break, soresult_callback_never fires and the session is never counted as finished. The synchronous handler treats all three as terminal,Destroyedsince #4298 and the other two in #4331, so the two handlers in the same file currently disagree.Suggested shape
Route every outcome through one idempotent step:
with
OnResponseand every terminalOnEventgoing through it, and the switch enumerating all fifteen states so-Wswitchcatches a future addition rather than adefault:swallowing it. The guard is also what makes widening the terminal set safe, since it removes the double-completion objection to treatingReadError,WriteErrorandDestroyedas terminal here the way the synchronous handler does.Worth covering: each of the three ignored states finishing one session as a failure, a response followed by another terminal event counting once, a cancellation followed by a late response counting once, and a duplicate terminal event counting once.
Two things I would rather flag than fold in. The two
ifblocks and the non-returning gzip failure branch inSession::SendRequestare shared HTTP client behaviour, so fixing them there would remove the double-fire for every consumer, but that is a separate change. And an exporter side guard is still warranted either way, becauseEventHandleris a public extension point and an injected client is free to emit whatever it likes.I am happy to open the PR for the exporter side. It overlaps #4297 and #4337 in the same file, so I would rather land those first unless you would prefer it sooner.