[release/11.0] Fix ConcurrentQueue.TryDequeue spuriously reporting empty during a segment freeze

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<object>();
+            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()
         {

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;