Repository navigation
[SDK] Fix lost wakeup in BatchSpanProcessor OnEnd notify - #4741
Conversation
The worker waits for a full batch, and once the queue is full later spans are dropped before they notify. OnEnd notified without cv_m, so the notify from the span that fills the queue could be lost, parking the worker with a full queue until the schedule delay. With max_export_batch_size clamped to max_queue_size that span is the only one that completes a batch. Notify under cv_m when the queue is full, as ForceFlush and Shutdown already do; the other notifies stay lock-free. Add an OnEnd stress test in the style of the existing ones, with BusySpin and ShutdownWithWatchdog helpers now shared by all three stress tests. The clamp test no longer sleeps before the last span, and it stops the worker before reading batch_sizes so a late export cannot race the read.
MSVC rejects the non-captured local constexpr kQueueSize inside the lambda (C3493). A static local needs no capture.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4741 +/- ##
==========================================
+ Coverage 86.61% 86.62% +0.01%
==========================================
Files 530 530
Lines 21165 21169 +4
==========================================
+ Hits 18331 18335 +4
Misses 2834 2834
🚀 New features to boost your workflow:
|
mateenali66
left a comment
mateenali66
left a comment
There was a problem hiding this comment.
checked that the new tests catch the bug. with this PR's two test files on 0c630dc, OnEndRacesWorkerPark hits the watchdog in 7 of 7 runs, at rounds 336 to 580, and the clamp test without its sleep fails 26 of 300. at 8732d49 both pass, 20 of 20 and 300 of 300, and both batch span processor binaries are tsan clean. the lock is taken only by the span that fills the queue, so the usual notify stays lock free.
non-blocking, for later: the logs processor keeps the lock-free notify at batch_log_record_processor.cc:109. its worker parks only on an empty buffer (:217), so the window there is the whole empty-to-full fill. it will need the same change once #4498 brings the full-batch wait from #4466 to logs.
marcalff
changed the title
Follow-up to #4727 (review comment).
Changes
When the queue is full, new spans are dropped before they wake the worker. So the span that fills the queue must wake it.
OnEndsent that notify without holdingcv_m, so it could be lost. The worker then slept untilschedule_delay_millisand all new spans were dropped.OnEndnow notifies undercv_mwhen the queue is full, likeForceFlushandShutdowndo since #4382. Other notifies stay lock-free.Also adds a stress test for this, with small helpers shared by the existing stress tests. The clamp test added in #4727 no longer needs its sleep, because the notify from its last span can no longer be lost. It now also stops the worker before reading the batch sizes, so a late export cannot race the read.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes