From 4dd8214625c44737ad9232d452baef378af44b5e Mon Sep 17 00:00:00 2001 From: victorgao Date: Fri, 28 Aug 2026 03:57:01 +0800 Subject: [PATCH] Fix ConcurrentQueue.TryDequeue spuriously reporting empty during a segment freeze (#132737) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #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. --- .../tests/ConcurrentQueueTests.cs | 45 +++++++++++++++++++ .../Concurrent/ConcurrentQueueSegment.cs | 12 ++--- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs b/src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs index 6ec3ce8b0b5be2..c074104d28d676 100644 --- a/src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs +++ b/src/libraries/System.Collections.Concurrent/tests/ConcurrentQueueTests.cs @@ -20,6 +20,51 @@ public class ConcurrentQueueTests : ProducerConsumerCollectionTests protected override string CopyToNoLengthParamName => null; + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] + public void Concurrent_TryDequeue_DoesNotReportEmptyWhileItemsAreQueued() + { + // Each worker holds at most one of the seeded items at a time, so an item is always + // queued and TryDequeue must never report empty + const int WorkerCount = 4; + + var q = new ConcurrentQueue(); + for (int i = 0; i < WorkerCount; i++) q.Enqueue(new object()); + + bool stop = false; + int falseEmpties = 0; + + // 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); + } + }); + } + + Thread.Sleep(TimeSpan.FromMilliseconds(200)); + Volatile.Write(ref stop, true); + Task.WaitAll(workers); + snapshotter.Wait(); + + Assert.Equal(0, falseEmpties); + } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] public void Concurrent_Enqueue_TryDequeue_AllItemsReceived() { diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs index 5e32c7872225ef..a20f46531bc5c9 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentQueueSegment.cs @@ -98,8 +98,10 @@ internal ConcurrentQueueSegment(int boundedLength) { if (!_frozenForEnqueues) // flag used to ensure we don't increase the Tail more than once if frozen more than once { - _frozenForEnqueues = true; + // 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; } } @@ -163,8 +165,8 @@ public bool TryDequeue([MaybeNullWhen(false)] out T item) // this one that are available, but we need to dequeue in order. So before declaring // failure and that the segment is empty, we check the tail to see if we're actually // empty or if we're just waiting for items in flight or after this one to become available. - bool frozen = _frozenForEnqueues; - int currentTail = Volatile.Read(ref _headAndTail.Tail); + bool frozen = Volatile.Read(ref _frozenForEnqueues); + int currentTail = _headAndTail.Tail; if (currentTail - currentHead <= 0 || (frozen && (currentTail - FreezeOffset - currentHead <= 0))) { item = default; @@ -232,8 +234,8 @@ public bool TryPeek([MaybeNullWhen(false)] out T result, bool resultUsed) // this one that are available, but we need to peek in order. So before declaring // failure and that the segment is empty, we check the tail to see if we're actually // empty or if we're just waiting for items in flight or after this one to become available. - bool frozen = _frozenForEnqueues; - int currentTail = Volatile.Read(ref _headAndTail.Tail); + bool frozen = Volatile.Read(ref _frozenForEnqueues); + int currentTail = _headAndTail.Tail; if (currentTail - currentHead <= 0 || (frozen && (currentTail - FreezeOffset - currentHead <= 0))) { result = default;