threading: lock-free fast path for SemaphoreSlim.WaitAsync #125452
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -401,10 +401,14 @@ private bool WaitCore(long millisecondsTimeout, CancellationToken cancellationTo | |
| // that synchronous waiter succeeds so that they have a chance to release. | ||
| Debug.Assert(!waitSuccessful || m_currentCount > 0, | ||
| "If the wait was successful, there should be count available."); | ||
| // Use CAS rather than a plain decrement: the lock-free fast path in WaitAsync | ||
| // can decrement m_currentCount concurrently (it holds no lock). | ||
| int currentCount = m_currentCount; | ||
| while (currentCount > 0 && Interlocked.CompareExchange(ref m_currentCount, currentCount - 1, currentCount) != currentCount) | ||
| currentCount = m_currentCount; | ||
| if (currentCount > 0) | ||
| { | ||
| waitSuccessful = true; | ||
| } | ||
| else if (oce is not null) | ||
| { | ||
|
|
@@ -678,12 +682,41 @@ private Task<bool> WaitAsyncCore(long millisecondsTimeout, CancellationToken can | |
| if (cancellationToken.IsCancellationRequested) | ||
| return Task.FromCanceled<bool>(cancellationToken); | ||
|
|
||
| // Fast path: try a lock-free acquire; falls through to the lock if it fails. | ||
| // Skipped when m_waitHandle is non-null to keep its state consistent under the lock. | ||
| if (m_waitHandle is null) | ||
| { | ||
| int current = m_currentCount; | ||
| // The waiter checks are best-effort: a sync waiter incrementing m_waitCount inside | ||
| // the lock may not be visible yet, but the CAS will fail if the count has changed. | ||
| if (current > 0 | ||
| && Volatile.Read(ref m_asyncHead) is null | ||
| && Volatile.Read(ref m_waitCount) == 0 | ||
| && Interlocked.CompareExchange(ref m_currentCount, current - 1, current) == current) | ||
|
Comment on lines
+692
to
+695
There was a problem hiding this comment. The fast-path gating reads m_asyncHead and m_waitCount outside the lock, but both fields are updated under the lock without volatile/Volatile.Write semantics. These checks can legally observe stale values, letting the CAS decrement run while a Release/Wait* locked path is concurrently releasing waiters / updating counts, which can corrupt accounting (e.g., negative m_currentCount or too many waiters completed). Please make the “no waiters” signal reliable (e.g., make m_waitCount and m_asyncHead volatile and ensure all writes are volatile, or add a dedicated volatile/Interlocked state flag used by the fast path), and consider making in-lock decrements of m_currentCount CAS-based so they can’t be invalidated by a concurrent lock-free acquire.
Author
There was a problem hiding this comment. m_asyncHead doesn't need volatile writes. When m_asyncHead is non-null, the invariant guarantees m_currentCount == 0 — so the m_waitCount stale reads are a real concern, but the root fix isn't volatile writes — it's making the in-lock check-then-decrement atomic. Even with Volatile.Write on m_waitCount, a fast-path thread that read m_waitCount = 0 before WaitCore's write can still have its CAS execute between WaitCore's line 404 check and the decrement. Replaced both in-lock plain decrements (WaitCore and WaitAsyncCore slow path) with the same CAS-loop pattern used in the fast path. Both threads' operations now serialize on m_currentCount atomically — only one wins, eliminating the double-grant. |
||
| { | ||
| // Handle the rare race where AvailableWaitHandle was initialised concurrently. | ||
| if (current == 1 && m_waitHandle is not null) | ||
| { | ||
| lock (m_lockObjAndDisposed) | ||
| { | ||
| if (m_waitHandle is not null && m_currentCount == 0) | ||
| m_waitHandle.Reset(); | ||
| } | ||
| } | ||
| return Task.FromResult(true); | ||
|
Comment on lines
+685
to
+706
There was a problem hiding this comment. This change introduces a lock-free acquire path with special handling for the race where |
||
| } | ||
| } | ||
thomhurst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| lock (m_lockObjAndDisposed) | ||
| { | ||
| // If there are counts available, allow this waiter to succeed. | ||
| // Use CAS rather than a plain decrement: the lock-free fast path in WaitAsync | ||
| // can decrement m_currentCount concurrently (it holds no lock). | ||
| int current = m_currentCount; | ||
| while (current > 0 && Interlocked.CompareExchange(ref m_currentCount, current - 1, current) != current) | ||
| current = m_currentCount; | ||
| if (current > 0) | ||
| { | ||
| if (m_waitHandle is not null && m_currentCount == 0) m_waitHandle.Reset(); | ||
| return Task.FromResult(true); | ||
| } | ||
|
|
@@ -899,7 +932,9 @@ public int Release(int releaseCount) | |
| waiterTask.TrySetResult(result: true); | ||
| } | ||
| } | ||
| // Use Interlocked.Add (relative delta) rather than an absolute write so that | ||
| // the lock-free CAS fast path in WaitAsync cannot be overwritten. | ||
| Interlocked.Add(ref m_currentCount, currentCount - returnCount); | ||
|
|
||
| // Exposing wait handle if it is not null | ||
| if (m_waitHandle is not null && returnCount == 0 && currentCount > 0) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Debug.Assert that assumes
waitSuccessfulimpliesm_currentCount > 0is no longer valid now thatWaitAsynccan decrementm_currentCountconcurrently without taking the lock. In Debug builds this can fire spuriously (e.g.,WaitUntilCountOrTimeoutobserved a positive count, but the lock-free fast path acquired the last permit before the assertion executes). Consider removing this assert or rewriting it to assert something that remains true under concurrent lock-free decrements (e.g., assert only after the CAS-based acquire attempt succeeds).