Conversation
…a non-responding client The synchronous export path waited on its response condition variable with no deadline of its own, entirely trusting the injected HttpClient to eventually deliver a terminal event via OnResponse or OnEvent. Nothing in the HttpClient interface actually guarantees that: a client that accepts a request and never calls back (a dead thread, a reused socket, a swallowed error) left Export() blocked for the life of the process, with no way for a caller's Shutdown() to release it either. waitForResponse() now takes an absolute deadline, derived from the exporter's own configured response timeout and captured before the request is sent, so the wait is bounded independent of whether the client honors its side of the contract. A deadline that passes without a terminal event reads as failure, the same outcome a terminal error event would already produce, so no successful path changes. Added a SilentHttpClient/SilentSession test double whose SendRequest() never calls back into its handler at all, and verified the regression test actually catches the bug: reverting the fix locally makes the test hang and get killed by its own timeout wrapper (exit 124), rather than passing vacuously. Fixes open-telemetry#4362
83187ea to
37d54e4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4530 +/- ##
=======================================
Coverage 86.51% 86.51%
=======================================
Files 525 525
Lines 20469 20469
=======================================
Hits 17707 17707
Misses 2762 2762
🚀 New features to boost your workflow:
|
…elated to this change)
mateenali66
left a comment
There was a problem hiding this comment.
Ran this against the curl client with response_timeout = 1. A server that never replies fails the export at about 1s on both main and this PR, and replies at 990 to 999ms split the same way. So the change only matters for a custom client.
For that case, Export() still calls session->FinishSession() after the deadline (:495). SilentSession::FinishSession returns at once, so the test can't see it. A client built like curl waits there on the transfer (http_operation_curl.cc:523-538), so a dead worker thread still hangs Export(). curl's CancelSession() does not block (http_client_curl.cc:255-262). Should the expired path cancel instead?
The conflict is only CHANGELOG.md.
FinishSession() waits for the in-flight transfer to complete, which defeats the purpose of the response deadline for HTTP clients (e.g. curl) whose worker thread blocks on the transfer itself. Cancel the session instead when the deadline expires. Addresses review feedback from open-telemetry#4530 (review)
Fixes #4362.
The synchronous export path waited on its response condition variable with no deadline of its own, trusting the injected
HttpClientto always eventually deliver a terminal event viaOnResponseorOnEvent. Nothing in theHttpClientinterface actually guarantees that: a client is a supported public surface, not a test seam, and one that accepts a request and never calls back (a dead thread, a reused socket, a swallowed error) leftExport()blocked for the life of the process, with no way for a caller'sShutdown()to release it either.Changes
waitForResponse()now takes an absolutestd::chrono::steady_clock::time_pointdeadline instead of waiting unconditionally, viacv_.wait_until()in place ofcv_.wait().response_timeout_and captured beforeSendRequest()is called, so it reflects this exporter's own timeout budget rather than whatever the client does with it.Pending, which reads as failure, the same outcome a terminal error event already produces today. No successful path changes.Testing
Added
SilentHttpClient/SilentSessiontest doubles whoseSendRequest()never calls back into the handler at all (noOnResponse, noOnEvent), the exact scenario the issue describes.ExportReturnsOnTimeoutWhenClientNeverRespondsconstructs the exporter with a 1 secondresponse_timeout_and this client, and assertsExport()returnskFailurerather than hanging.Verified the test actually catches the regression: reverted the fix locally (kept the test) and reran under a
timeoutwrapper, the test hung and was killed at exit code 124 instead of passing vacuously. Restored the fix and confirmed all four tests in the file pass, total runtime 1 second (the deadline in the new test), not 30 (the defaultresponse_timeout_).