Repository navigation
synchronization: don't lose a release() when counting_semaphore::wait_until times out - #7802
Conversation
…_until times out A waiter whose deadline expires can still be queued on the semaphore's condition variable when release() runs: it has already decided it timed out but has not yet reacquired the lock. release() increments the count and notify_one() hands the wakeup to that waiter, which then returned false from wait_until without looking at the count again. The released credit stayed unclaimed while the next waiter was never notified, so try_acquire_for/try_acquire_until (and binary_semaphore) could lose a release() and leave a waiter blocked despite an available credit. Only report a timeout when no credit is available, the same way timed_mutex::try_lock_until rechecks its predicate after a timeout. Add a regression test that holds the semaphore lock across the waiter's deadline and signals afterwards, so the waiter is notified after it has already timed out. Signed-off-by: aneek22112007-tech <[email protected]>
|
Can one of the admins verify this patch? |
Up to standards ✅🟢 Issues
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main bug fix: preventing counting_semaphore::wait_until from losing a release() after timeout. Description check The description directly explains the wait_until behavior change, the race condition, the regression test, and validation results. 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.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- 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.
|
@aneek22112007-tech, the new I think the test assumes something about timing that the CI doesn't guarantee: the main thread has to lock Could you please make the test independent of this? For instance, the main thread could check, right after it has locked |
Describe the PRs merged since the release branch was rebuilt on master cba138f (#7675, #7720, #7723, #7725, #7726, #7735, #7736, #7738, #7740, #7741, #7749, #7754, #7755, #7757, #7759), since it was rebuilt on master 97c7e25 (#7750, #7752, #7762, #7771, #7781, #7782, #7791, #7793), and since it was rebuilt on master 67e52ba (#7718, #7739, #7751, #7753, #7756, #7770, #7797, #7800, #7802, #7804, #7805). Regenerate the closed issue and pull request lists for release-2.0.X at a55fd37 against the v1.11.0 tag (765 PRs, 157 issues). Signed-off-by: Hartmut Kaiser <[email protected]>
…pendent of scheduling The regression test added in #7802 assumed the main thread acquires the mutex before the waiter's 10 ms deadline. On loaded CI nodes (Jenkins gcc-12-cuda-12) the main thread can be scheduled after the deadline; the waiter then correctly times out without the credit, and the test failed with state 2 and a left-over credit even though the semaphore behaved correctly. Record whether the lock was taken before the deadline, always signal past the deadline, join and drain the semaphore, and only check the iterations that actually exercised the timeout/signal race, with an upper bound on attempts so the test cannot run forever. Signed-off-by: aneek22112007-tech <[email protected]>
Proposed Changes
counting_semaphore::wait_untilonly whenvalue_ < count.cond_.wait_untilreturnsthread_restart_state::timeout, consume it and return true.test_wait_until_timeout_does_not_lose_signaltolibs/core/synchronization/tests/unit/counting_semaphore_cpp20.cpp.Any background context you want to provide?
A waiter whose deadline expires can still be queued on the semaphore's condition variable when
release()runs.execution_agent::sleep_untilhas already seenwas_reset_ == falseand returnedthread_restart_state::timeout, but the waiter has not yet reacquired the semaphore spinlock. It leaves the condition-variable queue only after that relock (reset_queue_entryruns onceunlock_guardhas taken the mutex again, and only ifnotify_onehas not already clearedctx_).release()then doesvalue_ += countandnotify_one()pops this waiter, because it is still at the front of the queue. The waiter relocks, observes timeout, and used to return false without looking atvalue_again. The released credit stayed unclaimed, and the next waiter was never notified.try_acquire_for/try_acquire_until, andbinary_semaphore(acounting_semaphore<1>), could lose arelease()and leave a later waiter blocked even though a credit was available.timed_mutex::try_lock_untilalready breaks out of its wait on timeout and then rechecksowner_id_before reporting failure.counting_semaphore::wait_untilnow does the same for the count: a timeout is a failed wait only when no credit is available. Athread_restart_state::signaledwakeup still falls through and re-evaluates the predicate, which is the behavior #7617 established.The regression test holds the semaphore lock across the waiter's deadline and calls
signalafterwards, so the notification happens only after the waiter has already timed out. It requires the waiter to take the credit (state == 3) and requires a followingtry_acquireto see no leftover count.Without this change,
counting_semaphore_cpp20_testfailed 10/10 with--hpx:threads=4and 10/10 with--hpx:threads=2(statestayed 2, andtry_acquirefound a leftover credit). With this change,counting_semaphore_cpp20_testpassed 20/20 at 2 threads, 20/20 at 4 threads, and once at 1 thread.binary_semaphore_cpp20_testpassed 3/3 at 2 threads and 3/3 at 4 threads.counting_semaphore_testpassed 3/3 at 2 threads and 3/3 at 4 threads.Checklist