Should volatile and readonly be mutually exclusive?

Question

Suppose that I were designing a thread-safe class that wraps an internal collection:

public class ThreadSafeQueue<T>
{
    private readonly Queue<T> _queue = new Queue<T>();

    public void Enqueue(T item)
    {
        lock (_queue)
        {
            _queue.Enqueue(item);
        }
    }

    // ...
}

Based on my other question, the above implementation is buggy, as race hazards may arise when its initialization is performed concurrently with its usage:

ThreadSafeQueue<int> tsqueue = null;

Parallel.Invoke(
    () => tsqueue = new ThreadSafeQueue<int>(),
    () => tsqueue?.Enqueue(5));

The code above is acceptably non-deterministic: the item may or may not be enqueued. However, under the current implementation, it is also broken, and can give rise to unpredictable behaviour, such as throwing IndexOutOfRangeException, NullReferenceException, enqueueing the same item multiple times, or getting stuck in an infinite loop. This occurs since the Enqueue call might run after the new instance has been assigned to the local variable tsqueue, but before the initialization of the internal _queue field completes (or appears to complete).

Per Jon Skeet:

The Java memory model doesn't ensure that the constructor completes before the reference to the new object is assigned to instance. The Java memory model underwent a reworking for version 1.5, but double-check locking is still broken after this without a volatile variable (as in C#).

This race hazard could be resolved by adding a memory barrier to the constructor:

    public ThreadSafeQueue()
    {
        Thread.MemoryBarrier();
    }

Equivalently, it could be more concisely resolved by making the field volatile:

    private volatile readonly Queue<T> _queue = new Queue<T>();

However, the latter is forbidden by the C# compiler:

'Program.ThreadSafeQueue<T>._queue': a field cannot be both volatile and readonly

Given that the above seems like a justifiable use-case for volatile readonly, is this limitation a flaw in the language design?

I'm aware that one could simply remove the readonly, since it does not affect the public interface of the class. However, that is beside the point, since the same could be said for readonly in general. I'm also aware of the existing question “Why readonly and volatile modifiers are mutually exclusive?”; however, that tackled a different issue.

Concrete scenario: This issue seems to affect code in the System.Collections.Concurrent namespace of the .NET Framework Class Library itself. The ConcurrentQueue<T>.Segment nested class has several fields that are only ever assigned within the constructor: m_array, m_state, m_index, and m_source. Of these, only m_index is declared as readonly; the others cannot be – although they should – since they need to be declared as volatile to meet the requirements of thread-safety.

private class Segment
{
    internal volatile T[] m_array;                  // should be readonly too
    internal volatile VolatileBool[] m_state;       // should be readonly too
    private volatile Segment m_next;
    internal readonly long m_index; 
    private volatile int m_low;
    private volatile int m_high; 
    private volatile ConcurrentQueue<T> m_source;   // should be readonly too

    internal Segment(long index, ConcurrentQueue<T> source)
    {
        m_array = new T[SEGMENT_SIZE];              // field only assigned here
        m_state = new VolatileBool[SEGMENT_SIZE];   // field only assigned here
        m_high = -1;
        m_index = index;                            // field only assigned here
        m_source = source;                          // field only assigned here
    }

    internal void Grow()
    {
        // m_index and m_source need to be volatile since race hazards
        // may otherwise arise if this method is called before
        // initialization completes (or appears to complete)
        Segment newSegment = new Segment(m_index + 1, m_source);
        m_next = newSegment;
        m_source.m_tail = m_next;
    }

    // ...
}

Answer 1

readonly fields are fully writable from the constructor's body. Indeed, one could use volatile accesses to a readonly field to cause a memory barrier. Your case is, I think, a good case for doing that (and it's prevented by the language).

It is true that writes made inside of the constructor might not be visible to other threads after the ctor has completed. They might become visible in any order even. This is not well known because it rarely comes into play in practice. The end of the constructor is not a memory barrier (as often assumed from intuition).

You can use the following workaround:

class Program
{
    readonly int x;

    public Program()
    {
        Volatile.Write(ref x, 1);
    }
}

I tested that this compiles. I was not sure whether it is allowed to form a ref to a readonly field or not, but it is.

Why does the language prevent readonly volatile? My best guess is that this is to prevent you from making a mistake. Most of the time this would be a mistake. It's like using await inside of lock: Sometimes this is perfectly safe but most of the time it's not.

Maybe this should have been a warning.

Volatile.Write did not exist at the time of C# 1.0 so the case for making this a warning for 1.0 is stronger. Now that there is a workaround the case for this being an error is strong.

I do not know if the CLR disallows readonly volatile. If yes, that could be another reason. The CLR has a style of allowing most operations that are reasonable to implement. C# is far more restrictive than the CLR. So I'm quite sure (without checking) that the CLR allows this.

Answer 2

ThreadSafeQueue<int> tsqueue = null;

Parallel.Invoke(
    () => tsqueue = new ThreadSafeQueue<int>(),
    () => tsqueue?.Enqueue(5));

In your example the issue is that tsqueue is published in non-thread safe manner. And in this scenario it's absolutely possible to get a partially constructed object on architectures like ARM. So, mark tsqueue as volatile or assign the value with Volatile.Write method.

This issue seems to affect code in the System.Collections.Concurrent namespace of the .NET Framework Class Library itself. The ConcurrentQueue.Segment nested class has several fields that are only ever assigned within the constructor: m_array, m_state, m_index, and m_source. Of these, only m_index is declared as readonly; the others cannot be – although they should – since they need to be declared as volatile to meet the requirements of thread-safety.

Marking fields as readonly just adds some constraints that compiler checks and that JIT might later use for optimizations (but JIT is smart enough to figure out that field is readonly even without that keyword in some cases). But marking these particular fields volatile is much more important because of concurrency. private and internal fields are under control of the author of that library, so it's absolutely ok to omit readonly there.