Locking a single bool variable when multithreading?

Question

Recently I have seen this code in a WebSite, and my question is the following:

        private bool mbTestFinished = false;

        private bool IsFinished()
        {
            lock( mLock )
            {
                return mbTestFinished;
            }
        }

        internal void SetFinished()
        {
            lock( mLock )
            {
                mbTestFinished = true;
            }
        }

In a multithread environment, is really necessary to lock the access to the mbTestFinished?

Solution 1

Yes, it is needed. .Net environment uses some optimizations, and sometimes if a memory location is accessed frequently, data is moved into CPU registers. So, in this case, if mbTestFinished is in a CPU register, then a thread reading it may get a wrong value. Thus using the volatile key ensures, all accesses to this variable is done at the memory location, not at the registers. On the otherhand, I have no idea of the frequency of this occurence. This may occur at a very low frequency.

Solution 2

In my opinion, no, the lock is superfluous here, for two reasons:

1. Boolean variables cannot cause assignment tearing like long for example, hence locking is not necessary.
2. To solve the visibility problem volatile is enough. It's true that the lock introduces an implicit fence, but since the lock is not required for atomicity, volatile is enough.

Solution 3

If mLock is ONLY for the variable mbTestFinished, then it's a bit of an overkill. Instead, you can use volatile or Interlocked, because both are User-Mode constructs for thread synchronization. lock (or Monitor) is a Hybrid Construct, in the sense that it is well optimized to avoid transiting from/to the Kernel-Mode whenever possible. The book "CLR via C#" has a in-depth discussion of these concepts.

Comments

[Marc Gravell]

It is the most provable mechanism for ensuring it isn't a CPU-cached read (which would not work well between threads) - volatile would work too, but for reasons that are too complex (this isn't the intent of volatile, but rather: a side-effect)

[Roman Starkov]

@MarcGravell I’ve always thought that was the intent of volatile; any chance you might drop a good link that explains what is?

[Roman Starkov]

This is true assuming the code shown is the only thing using the lock. If there are other places where mLock is taken, the conversion to volatile could break things.

[Tudor]

@romkyns: Yes, I'm assuming it's a self-contained scenario.