fix: preserve semaphore state on overflow - #144
Merged
Conversation
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
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.
Summary
fetch_addrelease path instead of adding a CAS retry loopreleaseand the common acquire/release round tripProblem
The previous implementation used
fetch_addand checked its returned value afterward. Atomic integer addition wraps, so an overflowingSemaphore::releasehad already written the wrapped permit count before the assertion panicked. For example, releasing one permit atusize::MAXleft the semaphore with zero available permits.Implementation
Every path that adds available permits already holds the
waiterslock. While that lock is held, concurrent lock-free operations can only remove permits. The implementation therefore loads and checks the current count under the lock, then uses the existingfetch_add; the count cannot grow between those two operations, so a successful check guarantees the addition cannot overflow.The checked load is
Relaxedbecause it is used only for the numeric overflow decision. Thefetch_addremainsRelease, preserving the synchronization behavior of the original implementation.This avoids both failure modes considered during the change: checking after
fetch_addtemporarily corrupts observable state, while a CAS loop adds unnecessary work and complexity to every successful release.Tests
release_overflow_panicsis an independent#[should_panic]test for the documented API contractrelease_overflow_preserves_permitscatches the panic, verifies the count remainsusize::MAX, and confirms the semaphore is still usablecargo x lintcargo x testcargo x benchcargo +1.85.0 test --workspace --no-default-featurescargo +1.85.0 bench -p mea --bench primitives --no-runBenchmarks
The same benchmark source was applied to
mainand this branch. Fixed-size Divan runs used 500 samples of 100,000 iterations to avoid timer-resolution artifacts.releasewas +0.8% and +2.2%;try_acquire_releasewas -1.2% and +1.4%releasewas +2.7%;try_acquire_releasewas +1.3%The Linux results are within run-to-run variation, while macOS shows the small cost of the additional read and overflow check rather than the larger cost of a CAS retry loop.