Fix missed wakeup race condition in BatchProcessor#5409
Closed
GauthamPrabhuM wants to merge 1 commit into
Closed
Conversation
|
The worker thread cleared _worker_awaken only after its export loop finished. If an application thread filled a batch and set the event in the window between the export loop draining the queue and the worker calling clear(), the wakeup was erased and the full batch sat in the queue for up to schedule_delay (5s by default). Clear the event immediately after wait() returns instead, so any set() that arrives while the worker is exporting is preserved for the next wait(). This affects both BatchSpanProcessor and BatchLogRecordProcessor since they share this implementation. Fixes open-telemetry#5400 Assisted-by: Claude Fable 5
GauthamPrabhuM
force-pushed
the
fix-batch-processor-missed-wakeup
branch
from
f9f3348 to
0f105cc
Compare
DylanRussell
left a comment
DylanRussell
left a comment
Contributor
There was a problem hiding this comment.
I described why I thought this wasn't needed and closed the PR in #5401 -- why is this re-opened under a different user now ?
Contributor
I'll close it again, we can report the user. |
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
3 participants
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.
Description
BatchProcessor.worker()clears_worker_awakenonly after its export loop finishes. If an application thread fills a batch and calls_worker_awaken.set()in the window between the export loop draining the queue and the worker callingclear(), the wakeup signal is erased and the full batch sits in the queue for up toschedule_delay(5s by default). This affects bothBatchSpanProcessorandBatchLogRecordProcessorsince they share this implementation, and shows up as recurring export-latency spikes under high throughput.This PR clears the event immediately after
wait()returns, before exporting, so aset()that arrives while the worker is exporting is preserved for the nextwait(). Worst case is one benign spurious wakeup that exports nothing.shutdown()is unaffected because worker exit is gated on the_shutdownflag, not the event.Regarding the question raised on the earlier attempt (#5401) — whether
_should_export_batchalready covers this because it keeps exporting while the queue exceeds the batch size: it doesn't cover this window, because the lost signal arrives after the export loop has already exited and before the event is cleared. The regression test demonstrates this deterministically: it fails onmain(items stay queued while the worker sleeps) and passes with this change.Fixes #5400
Type of change
How Has This Been Tested?
test_wakeup_not_lost_when_batch_fills_during_event_clearinopentelemetry-sdk/tests/shared_internal/test_batch_processor.py, parametrized overBatchLogRecordProcessorandBatchSpanProcessor. It hooks_worker_awaken.clearto inject a full batch at the exact race point; fails without the fix, passes with it (~0.05s, stable across repeated runs).opentelemetry-sdktest suite,tox -e precommit(ruff),tox -e lint-opentelemetry-sdk(pylint 10/10), andtox -e typecheck(pyright) all pass.Does This PR Require a Contrib Repo Change?
Checklist:
Assisted-by: Claude Fable 5
🤖 Generated with Claude Code