Is volatile still needed inside lock statements?

Question

I have read at different places people saying one should always use lock instead of volatile. I found that there are lots of confusing statements about Multithreading out there and even experts have different opinions on some things here.

After lots of research I found that the lock statement also will insert MemoryBarriers at least.

For example:

    public bool stopFlag;

    void Foo()
    {
        lock (myLock)
        {
          while (!stopFlag)
          {
             // do something
          }
        }
    }

But if I am not totally wrong, the JIT compiler is free to never actually read the variable inside the loop but instead it may only read a cached version of the variable from a register. AFAIK MemoryBarriers won't help if the JIT made a register assignment to a variable, it just ensures that if we read from memory that the value is current.

Unless there is some compiler magic saying something like "if a code block contains a MemoryBarrier, register assignment of all variables after the MemoryBarrier is prevented".

Unless declared volatile or read with Thread.VolatileRead(), if myBool is set from another Thread to false, the loop may still run infinite, is this correct? If yes, wouldn't this apply to ALL variables shared between Threads?

Answer 1

Whenever I see a question like this, my knee-jerk reaction is "assume nothing!" The .NET memory model is quite weak and the C# memory model is especially noteworthy for using language that can only apply to a processor with a weak memory model that isn't even supported anymore. Nothing what's there tells you anything what's going to happen in this code, you can reason about locks and memory barriers until you're blue in the face but you don't get anywhere with it.

The x64 jitter is quite clean and rarely throws a surprise. But its days are numbered, it is going to be replaced by Ryujit in VS2015. A rewrite that started with the x86 jitter codebase as a starting point. Which is a concern, the x86 jitter can throw you for a loop. Pun intended.

Best thing to do is to just try it and see what happens. Rewriting your code a little bit and making that loop as tight as possible so the jitter optimizer can do anything it wants:

class Test {
    public bool myBool;
    private static object myLock = new object();
    public int Foo() {
        lock (myLock) {
            int cnt = 0;
            while (!myBool) cnt++;
            return cnt;
        }
    }
}

And testing it like this:

    static void Main(string[] args) {
        var obj = new Test();
        new Thread(() => {
            Thread.Sleep(1000);
            obj.myBool = true;
        }).Start();
        Console.WriteLine(obj.Foo());
    }

Switch to the Release build. Project + Properties, Build tab, tick the "Prefer 32-bit" option. Tools + Options, Debugging, General, untick the "Suppress JIT optimization" option. First run the Debug build. Works fine, program terminates after a second. Now switch to the Release build, run and observe that it deadlocks, the loop never completes. Use Debug + Break All to see that it hangs in the loop.

To see why, look at the generated machine code with Debug + Windows + Disassembly. Focusing on the loop only:

                int cnt = 0;
013E26DD  xor         edx,edx                      ; cnt = 0
                while (myBool) {
013E26DF  movzx       eax,byte ptr [esi+4]         ; load myBool 
013E26E3  test        eax,eax                      ; myBool == true?
013E26E5  jne         013E26EC                     ; yes => bail out
013E26E7  inc         edx                          ; cnt++
013E26E8  test        eax,eax                      ; myBool == true?
013E26EA  jne         013E26E7                     ; yes => loop
                }
                return cnt;

The instruction at address 013E26E8 tells the tale. Note how the myBool variable is stored in the eax register, cnt in the edx register. A standard duty of the jitter optimizer, using the processor registers and avoiding memory loads and stores makes the code much faster. And note that when it tests the value, it still uses the register and does not reload from memory. This loop can therefore never end and it will always hang your program.

Code is pretty fake of course, nobody will ever write this. In practice this tends to work by accident, you'll have more code inside the while() loop. Too much to allow the jitter to optimize the variable way entirely. But there are no hard rules that will tell you when this happens. Sometimes it does pull it off, assume nothing. Proper synchronization should never be skipped. You really are only safe with an extra lock for myBool or an ARE/MRE or Interlocked.CompareExchange(). And if you want to cut such a volatile corner then you must check.

And noted in the comments, try Thread.VolatileRead() instead. You need to use a byte instead of a bool. It still hangs, it is not a synchronization primitive.

Answer 2

the JIT compiler is free to never actually read the variable inside the loop but instead it may only read a cached version of the variable from a register.

Well, it'll read the variable once, in the first iteration of the loop, but other than that, yes, it will continue to read a cached value, unless there is a memory barrier. Any time the code crosses a memory barrier it cannot use the cached value.

Using Thread.VolatileRead() adds the appropriate memory barriers, as does marking the field as volatile. There are plenty of other things that one could do that also implicitly add memory barriers; one of them is entering or leaving a lock statement.`

Since your loop is saying within the body of a single lock and not entering or leaving it, it's free to continue using the cached value.

Of course, the solution here isn't to add in a memory barrier. If you want to wait for another thread to notify you of when you should continue on, use a AutoResetEvent (or another similar synchronization tool specifically designed to allow threads to communicate).

Answer 3

how about this

public class Singleton<T> where T : class, new()
{
    private static T _instance;
    private static object _syncRoot = new Object();

    public static T Instance
    {
        get
        {
            var instance = _instance;
            if (instance == null)
            {
                lock (_syncRoot)
                {
                    instance = Volatile.Read(ref _instance);
                    if (instance == null)
                    {
                        instance = new T();
                    }
                    Volatile.Write(ref _instance, instance);
                }
            }
            return instance;
        }
    }
}