Is it safe to use Volatile.Read combined with Interlocked.Exchange for concurrently accessing a shar-CodePudding

Experts on threading/concurrency/memory model in .NET, could you verify that the following code is correct under all circumstances (that is, regardless of OS, .NET runtime, CPU architecture, etc.)?

class SomeClassWhoseInstancesAreAccessedConcurrently
{
    private Strategy _strategy;
    
    public SomeClassWhoseInstancesAreAccessedConcurrently()
    {
        _strategy = new SomeStrategy();
    }
    
    public void DoSomething()
    {
        Volatile.Read(ref _strategy).DoSomething();
    }

    public void ChangeStrategy()
    {
        Interlocked.Exchange(ref _strategy, new AnotherStrategy());
    }
}

This pattern comes up pretty frequently. We have an object which is used concurrently by multiple threads and at some point the value of one of its fields needs to be changed. We want to guarantee that from that point on every access to that field coming from any thread observe the new value.

Considering the example above, we want to make sure that after the point in time when ChangeStrategy is executed, it can't happen that SomeStrategy.DoSomething is called instead of AnotherStrategy.DoSomething because some of the threads don't observe the change and use the old value cached in a register/CPU cache/whatever.

To my knowledge of the topic, we need at least volatile read to prevent such caching. The main question is that is it enough or we need Interlocked.CompareExchange(ref _strategy, null, null) instead to achieve the correct behavior?

If volatile read is enough, a further question arises: do we need Interlocked.Exchange at all or even volatile write would be ok in this case?

As I understand, volatile reads/writes use half-fences which allows a write followed by a read reordered, whose implications I still can't fully understand, to be honest. However, as per ECMA 335 specification, section I.12.6.5, "The class library provides a variety of atomic operations in the System.Threading.Interlocked class. These operations (e.g., Increment, Decrement, Exchange, and CompareExchange) perform implicit acquire/release operations." So, if I understand this correctly, Interlocked.Exchange should create a full-fence, which looks enough.

But, to complicate things further, it seems that not all Interlocked operations were implemented according to the specification on every platform.

I'd be very grateful if someone could clear this up.

CodePudding user response：

Yes, your code is safe. It is functionally equivalent with using a lock like this:

public void DoSomething()
{
    Strategy strategy;
    lock (_locker) strategy = _strategy;
    strategy.DoSomething();
}

public void ChangeStrategy()
{
    Strategy strategy = new AnotherStrategy();
    lock (_locker) _strategy = strategy;
}

Your code is more performant though, because the lock imposes a full fence, while the Volatile.Read imposes a potentially cheaper half fence.

You could improve the performance even more by replacing the Interlocked.Exchange (full fence) with a Volatile.Write (half fence). The only reason to prefer the Interlocked.Exchange over the Volatile.Write is when you want to retrieve the previous strategy as an atomic operation. Apparently this is not needed in your case.

For simplicity you could even get rid of the Volatile.Write/Volatile.Read calls, and just declare the _strategy field as volatile.