Thread Safety in .NET TPL Dataflow Source

Question

I was looking at the implementation for some parts of the .NET TPL's "Dataflow" library out of curiosity and I came across the following snippet:

    private void GetHeadTailPositions(out Segment head, out Segment tail,
        out int headLow, out int tailHigh)
    {
        head = _head;
        tail = _tail;
        headLow = head.Low;
        tailHigh = tail.High;
        SpinWait spin = new SpinWait();

        //we loop until the observed values are stable and sensible.  
        //This ensures that any update order by other methods can be tolerated.
        while (
            //if head and tail changed, retry
            head != _head || tail != _tail
            //if low and high pointers, retry
            || headLow != head.Low || tailHigh != tail.High
            //if head jumps ahead of tail because of concurrent grow and dequeue, retry
            || head._index > tail._index)
        {
            spin.SpinOnce();
            head = _head;
            tail = _tail;
            headLow = head.Low;
            tailHigh = tail.High;
        }
    }

(Viewable here: https://github.com/dotnet/corefx/blob/master/src/System.Threading.Tasks.Dataflow/src/Internal/ConcurrentQueue.cs#L345)

From what I understand of thread-safety, this operation is prone to a data race. I am going to explain my understanding, and then what I perceive to be the 'error'. Of course, I expect it is more likely an error in my mental model than in the library, and I'm hoping someone here can point out where I'm going wrong.

...

All the given fields (head, tail, head.Low and tail.High) are volatile. In my understanding this gives two guarantees:

• Each time all four fields are read, they must be read in-order
• The compiler may not elide any of the reads, and the CLR/JIT must take measures to prevent 'caching' of the values

From what I read of the given method, the following happens:

1. An initial read of the internal state of the ConcurrentQueue is performed (that is head, tail, head.Low and tail.High).
2. A single busy-wait spin is performed
3. The method then reads the internal state again and checks for any changes
4. If the state has changed, go to step 2, and repeat
5. Return the read state once it is considered 'stable'

Now assuming that is all correct, my "problem" is thus: The reading of state above is not atomic. I see nothing preventing a read of half-written state (e.g. a writer thread has updated head but not yet tail).

Now I'm somewhat aware that half-written state in a buffer like this isn't the end of the world- after all the head and tail pointers are completely OK to be updated/read independently, usually in CAS/spin loops.

But then I don't really see what the point of spinning once and then reading again is. Are you really going to 'catch' a change-in-progress in the time it takes to do a single spin? What is it trying to 'guard' against? Put in other words: If the whole read of state is meant to be atomic, I don't think the method does anything to help that, and if not, what exactly is the method doing?

Answer 1

You're right, but note that the out values from GetHeadTailPositions are later used as a snapshot in ToList, Count and GetEnumerator.

More worrying is the fact that the concurrent queue might hold on to values indefinitely. When the private field ConcurrentQueue<T>._numSnapshotTakers is not zero, it prevents nulling out entries or setting them to their default value for value types.

Stephen Toub blogged about this in ConcurrentQueue<T> holding on to a few dequeued elements:

For better or worse, this behavior in .NET 4 is actually “by design.” The reason for this has to do with enumeration semantics. ConcurrentQueue<T> provides “snapshot semantics” for enumeration, meaning that the instant you start enumerating, ConcurrentQueue<T> captures the current head and tail of what’s currently in the queue, and even if those elements are dequeued after the capture or if new elements are enqueued after the capture, the enumeration will still return all of and only what was in the queue at the time the enumeration began. If elements in the segments were to be nulled out when they were dequeued, that would impact the veracity of these enumerations.

For .NET 4.5, we’ve changed the design to strike what we believe to be a good balance. Dequeued elements are now nulled out as they’re dequeued, unless there’s a concurrent enumeration happening, in which case the element isn’t nulled out and the same behavior as in .NET 4 is exhibited. So, if you never enumerate your ConcurrentQueue<T>, dequeues will result in the queue immediately dropping its reference to the dequeued element. Only if when the dequeue is issued someone happens to be enumerating the queue (i.e. having called GetEnumerator on the queue and not having traversed the enumerator or disposed of it yet) will the null’ing out not happen; as with .NET 4, at that point the reference will remain until the containing segment is removed.

As you can see from the source code, obtaining an enumerator (either via the generic GetEnumerator<T> or the non-generic GetEnumerator), calling ToList (or ToArray which uses ToList) or TryPeek may cause references to be kept even after removing items. Admittedly, the race condition between TryDequeue (which calls ConcurrentQueue<T>.Segment.TryRemove) and TryPeek may be hard to provoke, but it's there.