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;