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;
}
// ...
}
