[BUG] Prevent lost condition-variable wakeups during OTLP file, periodic metric, and batch span processor shutdown - #4365
Conversation
…dic metric, and batch span processor shutdown.
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4365 +/- ##
==========================================
+ Coverage 81.63% 81.64% +0.02%
==========================================
Files 493 493
Lines 19456 19463 +7
==========================================
+ Hits 15880 15889 +9
+ Misses 3576 3574 -2
🚀 New features to boost your workflow:
|
mateenali66
left a comment
mateenali66
left a comment
There was a problem hiding this comment.
reader and file exporter fixes look right. per the overlap discussion on #4382: dropping the batch_span_processor.cc hunk here would let both PRs land without conflict, the other two fixes aren't covered there at all. also needs a CHANGELOG entry
| { | ||
| // Acquiring cv_m_ guarantees that the next time the worker thread checks the wait condition | ||
| // on cv_ (either from notify below or any other reason) it will see IsShutdown() return true. | ||
| std::lock_guard<std::mutex> cv_guard{cv_m_}; |
There was a problem hiding this comment.
fence is right, the wait predicate checks IsShutdown() under cv_m_. the force flush path below has the same store-then-notify shape (the "must not wait for ever" workaround), in scope here or follow-up?
| // Even though is_shutdown is atomic, the lock guarantees that either a change to | ||
| // is_shutdown will be observed, or background_thread_waker_cv will see the notification | ||
| // at shutdown. | ||
| if (concurrency_file->is_shutdown.load(std::memory_order_acquire)) |
There was a problem hiding this comment.
with the early check gone, shutdown is observed only after one more flush pass, intended? fine if it's deliberate flush-on-shutdown, just checking
| { | ||
| { | ||
| std::lock_guard<std::mutex> waker_guard{file_->background_thread_waker_lock}; | ||
| file_->is_shutdown.store(true, std::memory_order_release); |
There was a problem hiding this comment.
Could we apply this same guarded store plus notify in OtlpFileSystemBackend::Shutdown() too? When there is nothing pending, ForceFlush() returns before notifying the worker, so Shutdown() can return while the background thread remains parked until flush_interval. The destructor now handles this correctly, but callers that keep the client alive after Shutdown() still retain the delayed worker.
| if (worker_thread_.joinable()) | ||
| { | ||
| synchronization_data_->is_force_wakeup_background_worker.store(true, std::memory_order_release); | ||
| { |
There was a problem hiding this comment.
Could we drop the BatchSpanProcessor changes from this PR? #4382 contains the more complete fix for this path, including ForceFlush, completion notification, and regression coverage. Keeping the trace fix there avoids overlapping changes between the two PRs.
3 participants
Changes
The issue in each changed spot is that there is a race between the "shutdown" call and the worker thread, such that the worker thread might not actually shut down. The worker will eventually shut down on the next configured periodic export, but if this could be configured to be relatively long (e.g. 1 minute). There's no workaround for this:
ForceFlushwill early-return after shutdown, so one can't useForceFlushto un-stuck a worker thread.Shutdownmore than once can lead to undefined behavior if it tries to join the worker thread from two different threads - at the very least, the return value ofjoinableis undefined, so a second thread trying to check if the worker is still joinable might seejoinableas false if the thread has been joined but is stuck.I did not update
CHANGELOG.mdsince the change seems small, but let me know if I should. I did not add unit tests as they would need to be invasive and complicated to reliably catch these. AFAIK there is no existing bug/issue documenting this.