[release/10.0] Fix ConcurrentQueue.TryDequeue spuriously reporting empty during a segment freeze- #132737 - #132897
Open
VSadov wants to merge 1 commit into
Open
[release/10.0] Fix ConcurrentQueue.TryDequeue spuriously reporting empty during a segment freeze- #132737#132897VSadov wants to merge 1 commit into
VSadov wants to merge 1 commit into
Conversation
…gment freeze (dotnet#132737) Fixes dotnet#132736 `ConcurrentQueueSegment.EnsureFrozenForEnqueues` published `_frozenForEnqueues = true` before bumping the Tail by `FreezeOffset`, while `TryDequeue`'s empty check reads the flag first and the Tail second. In the window between the two stores a dequeuer can pair `frozen == true` with the pre-freeze Tail and subtract `FreezeOffset` from a Tail that was never bumped. A segment holds fewer than `FreezeOffset` items, so `currentTail - FreezeOffset - currentHead <= 0` is then always true and `TryDequeue` reports the segment empty while it still holds committed items — a `false` return with no moment during the call at which the queue was empty. The `Interlocked.Add` full fence does not close the window; it only guarantees the stores become visible in exactly this order, and the freezing thread can stall between them (see the issue for the full interleaving and a reproducer that hits ~100 false-empties per 128M operations per round on current bits). The fix bumps the Tail before publishing the flag. The reader's three possible pairings become: 1. `frozen == true` — the bump is necessarily visible, so the Tail read afterwards includes `FreezeOffset` and the frozen clause reports empty only when the segment is genuinely drained. 2. `frozen == false` with a bumped Tail (freeze landed between the two reads) — `currentTail - currentHead` is a large positive value, so the check reports "not empty", spins, and retries; the next iteration takes pairing 1. This is the bounded benign retry the existing comment in `TryDequeue` already describes. 3. `frozen == false` with an un-bumped Tail — pre-freeze fast path, unchanged. Enqueuers never read the flag (a bumped Tail fails their sequence check and routes them to `EnqueueSlow`, unchanged), and `EnsureFrozenForEnqueues` only runs under the cross-segment lock, so the `if (!_frozenForEnqueues)` guard is unaffected by the reordering.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
VSadov
changed the title
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-collections |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Backport to release/10.0 addressing a rare ConcurrentQueue<T> race where TryDequeue / TryPeek can spuriously report empty during segment freezing, violating the intended linearizability of dequeues against already-committed enqueues.
Changes:
- Reorders segment-freeze publication so the Tail is bumped before the “frozen” flag is published.
- Adjusts the empty-check reads in
TryDequeue/TryPeekaround_frozenForEnqueuesand Tail. - Adds a concurrency stress test intended to catch false-empty dequeues during concurrent snapshotting (
ToArray()).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs | Reorders freeze publishing and tweaks the dequeue/peek empty-check reads to avoid false-empty results during freezing. |
| src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs | Adds a stress test to validate TryDequeue doesn’t report empty while items are continuously re-enqueued under concurrent snapshotting. |
Suppressed comments (2)
src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs:169
TryDequeueswitched fromVolatile.Read(ref _headAndTail.Tail)to a plain field read. Tail is concurrently mutated viaInterlocked/CompareExchangeand elsewhere is read withVolatile.Read(e.g.,TryEnqueue), so this should remain a volatile read to avoid observing a stale Tail and incorrectly returning empty / changing the intended retry behavior.
int currentTail = _headAndTail.Tail;
src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs:238
TryPeekswitched fromVolatile.Read(ref _headAndTail.Tail)to a plain field read. For the same reasons asTryDequeue(Tail is concurrently updated viaInterlockedoperations and other code paths use volatile reads), this should stay a volatile read to avoid stale Tail observations affecting the empty check.
int currentTail = _headAndTail.Tail;
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // Bump the Tail before setting the flag, as TryDequeue reads the flag and then | ||
| // the Tail: a set flag must guarantee that the Tail read includes the FreezeOffset. | ||
| Interlocked.Add(ref _headAndTail.Tail, FreezeOffset); | ||
| _frozenForEnqueues = true; |
Comment on lines
+36
to
+59
| // Snapshotting freezes the tail segment, racing the freeze against the empty check | ||
| Task snapshotter = Task.Run(() => | ||
| { | ||
| while (!Volatile.Read(ref stop)) q.ToArray(); | ||
| }); | ||
|
|
||
| // Workers dequeue and immediately re-enqueue, counting any spurious empty | ||
| Task[] workers = new Task[WorkerCount]; | ||
| for (int i = 0; i < WorkerCount; i++) | ||
| { | ||
| workers[i] = Task.Run(() => | ||
| { | ||
| while (!Volatile.Read(ref stop)) | ||
| { | ||
| if (!q.TryDequeue(out object item)) | ||
| { | ||
| Interlocked.Increment(ref falseEmpties); | ||
| item = new object(); | ||
| } | ||
| q.Enqueue(item); | ||
| } | ||
| }); | ||
| } | ||
|
|
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.
Backport of #132737 to release/10.0
/cc @VSadov @kafka1991
Customer Impact
#132736
A race in
ConcurrentQueuemay result inTryDequeue/TryPeekassume the queue is empty when it actually has items.The race is rare because it can happen only when the circular segment is completely full and the queue allocates a larger segment, while marking the old one "frozen" for more enqueues.
Regression
Testing
A new test is included together with the fix.
Risk
Low.
The fix changes the order of publishing
_frozenForEnqueueswith respect to updating segment Tail position to ensure that the consuming side does not see the segment in inconsistent state.