[EXPORTER] Fix Elasticsearch log exporter Export blocking forever on a non-responding client - #4530
Open
om7057 wants to merge 2 commits into
Open
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
om7057
force-pushed
the
fix/elasticsearch-export-wait-deadline
branch
from
September 7, 2026 14:25
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 83.46% 83.46%
=======================================
Files 521 521
Lines 20412 20412
=======================================
Hits 17034 17034
Misses 3378 3378
🚀 New features to boost your workflow:
|
…elated to this change)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_).