Search code examples
c#.netmultithreadingthread-safetyclr

Is it safe to replace Volatile.Write() with simple assignment in a method that contains only that statement?


This is an advanced question in C# multithreading.

Let's say I have this code that is used as a locking mechanism to enable only one thread to start some operation:

private static int _guard = 0;

private static bool acquire() {
    return Interlocked.CompareExchange(ref _guard, 1, 0) == 0;
}

private static void release() {
    Volatile.Write(ref _guard, 0);
}

This lock is used to protect method that should be execute only by one thread at the time:

public readonly Status Status = new (); // updated from thread that runs someTask

void TryRunningTask {
    if (acquire()) {
        return await someTask();
    } else {
        InfoMessage = "Another user is currently running someTask.";
    }
}

My question is, if I change release() as following:

private static void release() {
    _guard = 0;
}

Would the program still behave completely the same? Would that break the thread-safety? Would this change make any sense?


The reasoning behind my idea for this change is following:

  1. there are no other read/write operations inside of release() method. In MS Docs for Volatile.Write method it says:

Writes a value to a field. On systems that require it, inserts a memory barrier that prevents the processor from reordering memory operations as follows: If a read or write appears before this method in the code, the processor cannot move it after this method.

So because my release() method has no other operations before/after Volatile.Write() call I guess I can just replace it with simple assignment statement _guard = 0; right?

  1. as per C# Standard section 10.6 the _guard = 0; operation is guaranteed to be atomic operation:

10.6 Atomicity of variable references

Reads and writes of the following data types shall be atomic: bool, char, byte, sbyte, short, ushort, uint, int, float, and reference types.


Solution

  • No, you cannot remove the call to Volatile.Write.

    You are correct in regards the atomicity point: C# and the CLR mandates that 32-bit and smaller data types shall be atomic.

    However, there is not just atomicity to consider. There is also instruction reordering and processor caching to consider.

    Reordering may happen by the CLR Jitter, and the size of your function is not relevant, as it may be inlined into any function which calls it (and probably will be given it's short).

    The processor also may reorder instructions, if it conforms to the instructions it has been given.

    So this needs a memory barrier to be thread-safe.


    Processor caching is another issue: if the processor core is not told that a read or write is volatile, it could just use its own cache and ignore an what is in other cores' caches.

    But, Volatile.Write may not be enough anyway. I can't tell exactly from what you have shown, but it seems you have multiple threads which read and write. I think you should therefore use Interlocked.Exchange instead.