Do I need to lock or mark as volatile when accessing a simple boolean flag in C#?

user_119738 0
Viewed 8759

Lets just say you have a simple operation that runs on a background thread. You want to provide a way to cancel this operation so you create a boolean flag that you set to true from the click event handler of a cancel button.

private bool _cancelled;

private void CancelButton_Click(Object sender ClickEventArgs e)
{
    _cancelled = true;
}

Now you're setting the cancel flag from the GUI thread, but you're reading it from the background thread. Do you need to lock before accessing the bool?

Would you need to do this (and obviously lock in the button click event handler too):

while(operationNotComplete)
{
    // Do complex operation

    lock(_lockObject)
    {
        if(_cancelled)
        {
            break;
        }
    }
}

Or is it acceptable to do this (with no lock):

while(!_cancelled & operationNotComplete)
{
    // Do complex operation
}

Or what about marking the _cancelled variable as volatile. Is that necessary?

[I know there is the BackgroundWorker class with it's inbuilt CancelAsync() method, but I'm interested in the semantics and use of locking and threaded variable access here, not the specific implementation, the code is just an example.]

There seems to be two theories.

1) Because it is a simple inbuilt type (and access to inbuilt types is atomic in .net) and because we are only writing to it in one place and only reading on the background thread there is no need to lock or mark as volatile.
2) You should mark it as volatile because if you don't the compiler may optimise out the read in the while loop because it thinks nothing it capable of modifying the value.

Which is the correct technique? (And why?)

[Edit: There seem to be two clearly defined and opposing schools of thought on this. I am looking for a definitive answer on this so please if possible post your reasons and cite your sources along with your answer.]

thread-safety locking multithreading .net c#
5 Answers

Firstly, threading is tricky ;-p

Yes, despite all the rumours to the contrary, it is required to either use lock or volatile (but not both) when accessing a bool from multiple threads.

For simple types and access such as an exit flag (bool), then volatile is sufficient - this ensures that threads don't cache the value in their registers (meaning: one of the threads never sees updates).

For larger values (where atomicity is an issue), or where you want to synchronize a sequence of operations (a typical example being "if not exists and add" dictionary access), a lock is more versatile. This acts as a memory-barrier, so still gives you the thread safety, but provides other features such as pulse/wait. Note that you shouldn't use a lock on a value-type or a string; nor Type or this; the best option is to have your own locking object as a field (readonly object syncLock = new object();) and lock on this.

For an example of how badly it breaks (i.e. looping forever) if you don't synchronize - see here.

To span multiple programs, an OS primitive like a Mutex or *ResetEvent may also be useful, but this is overkill for a single exe.

Marc_Gravell_233540

_cancelled must be volatile. (if you don't choose to lock)

If one thread changes the value of _cancelled, other threads might not see the updated result.

Also, I think the read/write operations of _cancelled are atomic:

Section 12.6.6 of the CLI spec states: "A conforming CLI shall guarantee that read and write access to properly aligned memory locations no larger than the native word size is atomic when all the write accesses to a location are the same size."

bruno_conde_311360

Locking is not required because you have a single writer scenario and a boolean field is a simple structure with no risk of corrupting the state (while it was possible to get a boolean value that is neither false nor true). But you have to mark the field as volatile to prevent the compiler from doing some optimizations. Without the volatile modifier the compiler could cache the value in a register during the execution of your loop on your worker thread and in turn the loop would never recognize the changed value. This MSDN article (How to: Create and Terminate Threads (C# Programming Guide)) addresses this issue. While there is need for locking, a lock will have the same effect as marking the field volatile.

user_775070

For thread synchronization, it's recommended that you use one of the EventWaitHandle classes, such as ManualResetEvent. While it's marginally simpler to employ a simple boolean flag as you do here (and yes, you'd want to mark it as volatile), IMO it's better to get into the practice of using the threading tools. For your purposes, you'd do something like this...

private System.Threading.ManualResetEvent threadStop;

void StartThread()
{
    // do your setup

    // instantiate it unset
    threadStop = new System.Threading.ManualResetEvent(false); 

    // start the thread
}

In your thread..

while(!threadStop.WaitOne(0) && !operationComplete)
{
    // work
}

Then in the GUI to cancel...

threadStop.Set();
Adam_Robinson_821870

Look up Interlocked.Exchange(). It does a very fast copy into a local variable which can be used for comparison. It is faster than lock().

user_102930
Related