Repository navigation
Fix condition_variable abort_all/notify_one waking threads that already left the wait - #7879
Conversation
condition_variable::abort_all() and notify_one() release the lock before waking the waiter. By then the waiter may have left wait_until() on its own (timeout) and even terminated: - abort_all() used ctx.abort(), which lands on whatever suspension point the thread reaches next. When that is the spinlock backoff inside ~unlock_guard, yield_aborted is thrown from a noexcept destructor and the process terminates (the occasional condition_variable_abort_stress failures in CI). - waking through the agent_ref of a thread that has terminated throws null_thread_id, or wakes an unrelated thread that reuses its thread_data. abort_all() now marks the queue entry as aborted while holding the lock and only resumes the thread; the waiter turns the flag into an abort after re-acquiring the lock (wait() throws yield_aborted, wait_until() returns thread_restart_state::abort). New waiters are rejected while abort_all() drains the queue, which bounds the loop. Queue entries hold a counted reference to the waiting thread, and waiters are woken through it, so a late wakeup of a terminated thread is a no-op. Signed-off-by: Hartmut Kaiser <[email protected]>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Retain waiter identity and dispatch wakeups libs/core/synchronization/src/detail/condition_variable.cpp |
Queue entries retain counted thread IDs. notify_one() uses a shared wakeup helper to schedule HPX threads by ID or resume an agent reference. |
Drain aborted waiters and report abort state libs/core/synchronization/include/hpx/synchronization/detail/condition_variable.hpp, libs/core/synchronization/src/detail/condition_variable.cpp |
A counter tracks abort drains. abort_all() marks and wakes removed waiters; wait() and wait_until() report abort state when the drain begins or marks a waiter. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Waiter
participant condition_variable
participant queue_entry
participant resume_waiter
participant HPX_scheduler
Waiter->>condition_variable: call wait()
condition_variable->>queue_entry: enqueue waiter
condition_variable->>queue_entry: mark removed waiter aborted
condition_variable->>resume_waiter: wake removed waiter
resume_waiter->>HPX_scheduler: schedule retained thread ID
Possibly related PRs
- TheHPXProject/hpx#7787: Fixes queue-pointer races in the same
condition_variableimplementation and adds an abort stress test. - TheHPXProject/hpx#7878: Updates the abort stress-test follow-up to handle
yield_abortedand non-joinable workers.
Merge Risk: 
dd0b2
-
No identified issue blocks merging the condition-variable wakeup and abort changes after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the primary change: condition_variable abort_all() and notify_one() no longer wake threads that already left the wait. The raw summary confirms the diff changes abort_all(), notify_one… Description check The description covers the changes in the diff. It explains that abort_all() marks queue entries as aborted under the lock, that new waiters are rejected while abort_all() drains the queue, and that q… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the primary change: condition_variable abort_all() and notify_one() no longer wake threads that already left the wait. The raw summary confirms the diff changes abort_all(), notify_one(), and wait paths in condition_variable.cpp, and adds the aborting_ member to the header. The title is specific and concise, and it matches the PR objectives.
Full details: Description check
-
Explanation
The description covers the changes in the diff. It explains that abort_all() marks queue entries as aborted under the lock, that new waiters are rejected while abort_all() drains the queue, and that queue entries hold a counted thread_id_ref_type that abort_all() and notify_one() use for wakeups. It also gives the failing CI symptom and the testing performed, so it is related to the changeset.
-
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Up to standards ✅🟢 Issues
|
Follow-up to #7787, supersedes #7878.
Proposed Changes
abort_all()marks a waiter's queue entry as aborted while holding the lock and only resumes the thread (instead of forcingctx.abort()after releasing the lock). The waiter turns the flag into an abort after re-acquiring the lock:wait()throwsyield_abortedas before,wait_until()returnsthread_restart_state::abort(already handled bytimed_mutex::try_lock_untilandcondition_variable::wait_until).abort_all()drains the queue, new waiters are rejected (wait()throwsyield_aborted,wait_until()returnsabort). Since the lock is released for every woken thread, an aborted thread that waits again could otherwise keep refilling the queue andabort_all()would never finish.thread_id_ref_type) to the waiting thread.abort_all()andnotify_one()wake the waiter through it instead of through itsagent_ref, so a wakeup that arrives after the waiter has terminated is a no-op instead of throwingnull_thread_idor waking an unrelated thread that reuses the thread_data.Any background context you want to provide?
condition_variable_abort_stress(added in #7787) fails occasionally in CI withterminate called after throwing ... aborted (yield returned wait_abort): HPX(yield_aborted), e.g. in runs 40208092792, 40241656555, 40241996935, 40284047350 and 40307299100. The message format is the one fromexecution_agent::do_yield.abort_all()pops a waiter, releases the lock and only then callsctx.abort(). By that time the waiter may already have leftwait_until()on its own (100us timeout) and be re-acquiring the spinlock inside~unlock_guard, spinning inyield_kwithpending_boost.set_thread_statetreatspending_boostas different frompendingand stores the abort into the already queued thread, sodo_yieldthrowsyield_abortedfrom the (implicitlynoexcept) destructor, which terminates the process. No change to the test can catch this, which is why #7878 does not help.Widening the window between unlock and wakeup (a temporary 200us busy wait) also exposed the agent lifetime problem: in the final
abort_all()afterstopthe waiter can exit and terminate before it is woken, and the late wakeup through itsagent_refthrewnull_thread_idinhpx_main. The same race exists innotify_one().Testing (Windows, MSVC, Release and RelWithDebInfo): 2x100 runs of
condition_variable_abort_stresswith--hpx:queuing=local-workrequesting-fifo --hpx:threads=2and with--hpx:threads=4, all synchronization and threading unit tests (50/50), and 300 runs with the forced 200us window, which crashed or hung before these changes. As a side effect a stress run takes about 0.15s instead of about 6s, since aborting threads that are still running no longer spins inset_thread_state.