Is it ok to read a shared boolean flag without locking it when another thread may set it (at most once)?

I would like my thread to shut down more gracefully so I am trying to implement a simple signalling mechanism. I don't think I want a fully event-driven thread so I have a worker with a method to graceully stop it using a critical section Monitor (equivalent to a C# lock I believe):

DrawingThread.h

class DrawingThread {
    bool stopRequested;
    Runtime::Monitor CSMonitor;
    CPInfo *pPInfo;
    //More..
}

DrawingThread.cpp

void DrawingThread::Run() {
    if (!stopRequested)
        //Time consuming call#1
    if (!stopRequested) {
        CSMonitor.Enter();
        pPInfo = new CPInfo(/**/);
        //Not time consuming but pPInfo must either be null or constructed. 
        CSMonitor.Exit();
    }
    if (!stopRequested) {
        pPInfo->foobar(/**/);//Time consuming and can be signalled
    }
    if (!stopRequested) {
        //One more optional but time consuming call.
    }
}


void DrawingThread::RequestStop() {
    CSMonitor.Enter();
    stopRequested = true;
    if (pPInfo) pPInfo->RequestStop();
    CSMonitor.Exit();
}

I understand (at least in Windows) Monitor/locks are the least expensive thread synchronization primitive but I am keen to avoid overuse. Should I be wrapping each read of this boolean flag? It is initialized to false and only set once to true when stop is requested (if it is requested before the task completes).

My tutors advised to protect even bool's because read/writing may not be atomic. I think this one shot flag is the exception that proves the rule?

Solution 1:

It is never OK to read something possibly modified in a different thread without synchronization. What level of synchronization is needed depends on what you are actually reading. For primitive types, you should have a look at atomic reads, e.g. in the form of std::atomic<bool>.

The reason synchronization is always needed is that the processors will have the data possibly shared in a cache line. It has no reason to update this value to a value possibly changed in a different thread if there is no synchronization. Worse, yet, if there is no synchronization it may write the wrong value if something stored close to the value is changed and synchronized.

Solution 2:

Boolean assignment is atomic. That's not the problem.

The problem is that a thread may not not see changes to a variable done by a different thread due to either compiler or CPU instruction reordering or data caching (i.e. the thread that reads the boolean flag may read a cached value, instead of the actual updated value).

The solution is a memory fence, which indeed is implicitly added by lock statements, but for a single variable it's overkill. Just declare it as std::atomic<bool>.

Solution 3:

The answer, I believe, is "it depends." If you're using C++03, threading isn't defined in the Standard, and you'll have to read what your compiler and your thread library say, although this kind of thing is usually called a "benign race" and is usually OK.

If you're using C++11, benign races are undefined behavior. Even when undefined behavior doesn't make sense for the underlying data type. The problem is that compilers can assume that programs have no undefined behavior, and make optimizations based on that (see also the Part 1 and Part 2 linked from there). For instance, your compiler could decide to read the flag once and cache the value because it's undefined behavior to write to the variable in another thread without some kind of mutex or memory barrier.

Of course, it may well be that your compiler promises to not make that optimization. You'll need to look.

The easiest solution is to use std::atomic<bool> in C++11, or something like Hans Boehm's atomic_ops elsewhere.