I've written a Windows program in C++ which at times uses two threads: one background thread for performing time-consuming work; and another thread for managing the graphical interface. This way the program is still responsive to the user, which is needed to be able to abort a certain operation. The threads communicate via a shared bool variable, which is set to true when the GUI thread signals the worker thread to abort. Here is the code which implements this behaviour (I've stripped away irrelevant parts):

CODE EXECUTED BY THE GUI THREAD


class ProgressBarDialog : protected Dialog {

    /**
     * This points to the variable which the worker thread reads to check if it
     * should abort or not.
     */
    bool volatile* threadParameterAbort_;

    ...

    BOOL CALLBACK ProgressBarDialog::DialogProc( HWND dialog, UINT message, 
        WPARAM wParam, LPARAM lParam ) {

        switch( message ) {
         开发者_如何学运维   case WM_COMMAND :
                switch ( LOWORD( wParam ) ) {

                    ...

                    case IDCANCEL :
                    case IDC_BUTTON_CANCEL :
                        switch ( progressMode_ ) {
                            if ( confirmAbort() ) {
                                // This causes the worker thread to be aborted
                                *threadParameterAbort_ = true;
                            }
                            break;
                        }

                        return TRUE;
                }
        }

        return FALSE;
    }

    ...

};

CODE EXECUTED BY THE WORKER THREAD


class CsvFileHandler {

    /**
     * This points to the variable which is set by the GUI thread when this
     * thread should abort its execution.
     */
    bool volatile* threadParamAbort_;

    ...

    ParseResult parseFile( ItemList* list ) {
        ParseResult result;

        ...

        while ( readLine( &line ) ) {
            if ( ( threadParamAbort_ != NULL ) && *threadParamAbort_ ) {
                break;
            }

            ...
        }

        return result;
    }

    ...

};

threadParameterAbort_ in both threads point to a bool variable declared in a structure which is passed to the worker thread upon creation. It is declared as

bool volatile abortExecution_;

My question is: do I need to use volatile here, and is the code above sufficient to ensure that the program is thread-safe? The way I've reasoned for justifying the use of volatile here (see this question for background) is that it will:

prevent the reading of *threadParameterAbort_ to use the cache and instead get the value from memory, and
prevent the compiler from removing the if clause in the worker thread due to optimization.

(The following paragraph is only concerned with the thread-safety of the program as such and does not, I repeat, does not involve claiming that volatile in any way provides any means of ensuring thread-safety.) As far as I can tell, it should be thread-safe as setting of a bool variable should in most, if not all, architectures be an atomic operation. But I could be wrong. And I'm also worried about if the compiler may reorder instructions such as to break thread-safety. But better be safe (no pun intended) than sorry.

EDIT: A minor mistake in my wording made the question appear as if I was asking if volatile is enough to ensure thread-safety. This was not my intent -- volatile does indeed not ensure thread-safety in any way -- but what I meant to ask was if the code provided above exhibit the correct behaviour to ensure that the program is thread-safe.

You should not depend on volatile to guarantee thread safety, this is because even though the compiler will guarantee that the the variable is always read from memory (and not a register cache), in multi-processor environments a memory barrier will also be required.

Rather use the correct lock around the shared memory. Locks like a Critical Section are often extremely lightweight and in a case of no contention will probably be all implemented userside. They will also contain the necessary memory barriers.

Volatile should only be used for memory mapped IO where multiple reads may return different values. Similarly for memory mapped writes.

Wikipedia says it pretty well.

In C, and consequently C++, the volatile keyword was intended to allow access to memory mapped devices allow uses of variables between setjmp allow uses of sig_atomic_t variables in signal handlers

Operations on volatile variables are not atomic nor establish a proper happens-before relationship for threading. This is according to the relevant standards (C, C++, POSIX, WIN32), and this is the matter of fact for the vast majority of current implementations. The volatile keyword is basically worthless as a portable threading construct.

volatile is neither necessary nor sufficient for multithreading in C++. It disables optimizations that are perfectly acceptable, but fails to enforce things like atomicity that are needed.

Edit: instead of using a critical section, I'd probably use InterlockedIncrement, which gives an atomic write with less overhead.

What I'd normally do, however, is hook up a thread-safe queue (or deque) as the input to the thread. When you have something for the thread to do, you just put a packet of data describing the job into the queue, and the thread does it when it can. When you want the thread to shut-down normally, you put a "shutdown" packet into the queue. If you need an immediate abort, you use the deque instead, and put the "abort" command on the front of the deque. In theory, this has the shortcoming that it doesn't abort the thread until it finishes its current task. About all that means is that you want to keep each task to roughly the same size/latency range as the frequency with which you currently check the flag.

This general design avoids a whole host of IPC problems.

With regard to my answer to yesterday's question, no, volatile is unnecessary. In fact, multithreading here is irrelevant.

    while ( readLine( &line ) ) { // threadParamAbort_ is not local:
        if ( ( threadParamAbort_ != NULL ) && *threadParamAbort_ ) {

prevent the reading of *threadParameterAbort_ to use the cache and instead get the value from memory, and

prevent the compiler from removing the if clause in the worker thread due to optimization.

The function readLine is external library code, or else it calls external library code. Therefore, the compiler cannot assume there are any nonlocal variables it does not modify. Once a pointer to an object (or its superobject) has been formed, it might be passed and stored anywhere. The compiler can't track what pointers end up in global variables and which don't.

So, the compiler assumes that readLine has its own private static bool * to threadParamAbort_, and modifies the value. Therefore it's necessary to reload from memory.

Seems that the same use case is descibed here: volatile - Multithreaded Programmer's Best Friend by Alexandrescu. It states that exactly in this case (to create flag) volatile can be perfectly used.

So, yes exactly in this case code should be correct. volative will prevent both - reading from cache and prevent compiler from optimizing out if statement.

Ok, so you've been beaten up enough about volatile and thread safety!, but...

An example for your specific code (though a stretch and within your control) is the fact that you are looking at your variable several times in the one 'transaction':

if ( ( threadParamAbort_ != NULL ) && *threadParamAbort_ )

If for whatever reason threadParamAbort_ is deleted after the left hand side and before the right hand side then you will dereference a deleted pointer. Again, this is unlikely given you have control but is an example of what volatile and atomicity can't do for you.

While the other answers are correct, I also suggest you take a look at the "Microsoft specific" section of the MSDN documentation for volatile

I think it'll work fine (atomic or not) because you're only using it to cancel a background operation.

The main thing that volatile is going to do is prevent the variable being cached or retrieved from a register. You can be sure that it's coming from main memory.

So if I were you, yes I would define the variable as volatile.

I think it's best to assume that volatile is needed to ensure that the variable when written and then read by the other threads that they actually get the correct value for that variable as soon as possible and to ensure that IF is not optimised away (although I doubt it would be).