11

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.

Community
  • 1
  • 1
gablin
  • 4,678
  • 6
  • 33
  • 47
  • 1
    Dup of better [half of that](http://stackoverflow.com/search?q=c%2B%2B+thread+volatile). Most recent IIRC was [the question](http://stackoverflow.com/questions/3372703/volatile-and-multithreading). – Dummy00001 Aug 31 '10 at 20:09
  • Question yesterday from same person covers the same ground: http://stackoverflow.com/questions/3604569/what-kinds-of-optimizations-does-volatile-prevent-in-c . If you aren't writing a device driver and you aren't intimately familiar with the compiler, you don't need `volatile`. – Potatoswatter Aug 31 '10 at 23:09
  • @Potatoswatter: But it does appear that `volatile` can be of use when writing multi-threaded code, as shown in this article (http://www.drdobbs.com/cpp/184403766) which was linked in one of the answers. – gablin Sep 01 '10 at 11:54
  • 1
    That article is obsolete. Pay it no mind. `volatile` might help work around bugs in an inadequate threading library, but that's not an issue any more. (Remember, C++ wasn't formally designed for multithreading at all, so if a language feature like `volatile` helps with multithreading, it's a mere coincidence.) And considering the issues discussed in the other question, it's insufficient to implement any synchronization from scratch. As Jerry said, neither necessary nor sufficient. NEVER use it. Get familiar with your threading library and use clean, supported synchronization. – Potatoswatter Sep 04 '10 at 02:27
  • @Potatoswatter: Aaah. Thank you very much for pointing this out. Man, I wish I had found Stackoverflow much, much sooner -- it's just so full of useful information and people. ^^ Thanks again. – gablin Sep 04 '10 at 11:52

8 Answers8

14

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.

doron
  • 27,972
  • 12
  • 65
  • 103
  • But if I remove `volatile`, can it happen that the compiler mistakenly optimize away the `if` clause? Or perhaps that the value of `*threadParamAbort_` always uses the cache? Does applying a critical section through a mutex or semaphore ensure that this does not happen? – gablin Aug 31 '10 at 21:22
  • 3
    A mutex or semaphore is a non-pure function. It may have side effects (change global variables) and therefore the compiler may not assume the value of *threadParamAbort_ will remain unchanged. And will have to be reread after the mutex or critical section lock or signal. And since only 1 thread can hold a critical section at a time, there is nothing wrong with reordering instructions inside the lock. Just ensure you apply the lock correctly. – doron Aug 31 '10 at 21:50
  • Okay, so by wrapping the `if` clause with a critical section will prevent the compiler from optimizing it away, correct? Even though `threadParameterAbort_` is in fact not a global variable? If so, then `volatile` is not needed as this is provided by the critical section. And agreed, reordering inside the critical section is not wrong. However, do I need to wrap the setting of `threadParameterAbort_` in the GUI thread with a crticial section? Seems redundant as that operation itself is atomic, yes? – gablin Aug 31 '10 at 22:02
  • I take it threadParameterAbort_ is a pointer to a shared variable so the value is effectively global. And yes, even if something looks global use a Critical Section. They are really cheap and will ensure that you are not affected by processor read/write reordering. – doron Sep 01 '10 at 09:53
  • @deus-ex-machina399: Even if reading from and writing to boolean values are atomic actions? – gablin Sep 01 '10 at 11:55
  • yes because on multiprocessor / multicore systems, reads and writes can be out of order. – doron Sep 01 '10 at 12:32
9

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.

Judah Gabriel Himango
  • 58,906
  • 38
  • 158
  • 212
Serapth
  • 7,122
  • 4
  • 31
  • 39
  • 4
    I do not understand why people keep thinking `volatile` provides any kind of thread safety whatsoever. +1 – Billy ONeal Aug 31 '10 at 19:41
  • They come from Java and even there it's no thread-safe assurance. – wheaties Aug 31 '10 at 19:43
  • 4
    Wikipedia is a terrible source to quote (Even Wikipedias founder says don't use wikipedia as a source for quotes). Use it as a starting point to do your research but at least quote an authoritative source. – Martin York Aug 31 '10 at 20:04
  • 6
    Why, the alternative would be to cite nothing at all. Or I could cite the C or C++ documentation, which would have been much more difficult to understand. Besides, its not exactly like I am submitting a thesis here. If someone answered it succinctly on wikipedia, I will link to Wikipedia. I simply do not understand knee jerk Wikipedia hate nor do I understand these arbitrary posting rules, where the other option would have been to simply quote no source, as most answers do. – Serapth Aug 31 '10 at 20:14
  • @Billy Oneal: I am well aware that `volatile` does not guarantee thread safety, and I've clearly stated in the question my reasoning for applying `volatile`. – gablin Aug 31 '10 at 21:15
  • 3
    @gablin: No, you misunderstand me. `volatile` has *nothing* to do with thread safety. At all. – Billy ONeal Aug 31 '10 at 21:24
  • @Billy ONeal: And I am not disputing that statement; in fact, I agree, very much so. I think I made a mistake in the wording of my question which made people think that I believe `volatile` has anything to do with thread-safety. The error has now been corrected. – gablin Aug 31 '10 at 21:56
3

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.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
  • OK; You are going to have to expand that. I agree that it is not sufficient. I agree that portability reliance on volatile is not going to be useful. But is it not going to provide some benefit on Windows using Cl1 in that the memory is not cached and thus we do not need to worry about cache coherence problems across multiple threads. – Martin York Aug 31 '10 at 20:09
  • @Jerry Coffin: Again, I am well aware that `volatile` does in no way guarantee thread safety or atomicity. Also, is not writing or reading a `bool` value an atomic operation? – gablin Aug 31 '10 at 21:24
  • 1
    @Martin: `volatile` only prevents the compiler for keeping data in a register; it does *nothing* to prevent/help with cache coherence problems. @gablin: reading or writing a `bool` might be atomic -- then again, it might not. It probably usually will be by default with MS VC++, but it's definitely possible to create a `bool` that won't be read/written atomically. – Jerry Coffin Aug 31 '10 at 21:52
  • @Jerry Coffin: OK. So how do we deal with Cache coherence when we have multiple threads (potentially on different processors) Are all modern processors handling this at the hardware level? Or is there something else? – Martin York Aug 31 '10 at 22:04
  • @Jerry Coffin: I see. In that case it would be necessary to wrap the reading of and writing to `threadParameterAbort_` with a critical section. Just in case. Thanks. – gablin Aug 31 '10 at 22:07
  • @Martin York: Cache coherence usually is provided by the hardware. But I believe there are architectures which do not provide that, which leaves you to deal with it. – gablin Aug 31 '10 at 22:09
  • @Martin: At least so far, anything that runs Windows handles cache coherence in hardware. That doesn't guarantee ordering or atomicity though. That's why they have atomic update instructions, memory fence instructions, etc. – Jerry Coffin Aug 31 '10 at 22:32
  • @Jerry Coffin (concerning the latest edit): Seems like using a thread-safe queue just to signal a thread to stop is a bit of an overkill. – gablin Sep 01 '10 at 11:57
  • 1
    @gablin: if that was all it was for, I'd agree. OTOH, I tend to design it in from the beginning. Most programs grow over time, and having it there usually makes that growth a lot more manageable. – Jerry Coffin Sep 01 '10 at 13:16
  • @Jerry Coffin: Very true. I'll +1 that comment. – gablin Sep 01 '10 at 13:51
3

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_ ) {
  1. prevent the reading of *threadParameterAbort_ to use the cache and instead get the value from memory, and
  2. 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.

Potatoswatter
  • 134,909
  • 25
  • 265
  • 421
1

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.

Shcheklein
  • 5,979
  • 7
  • 44
  • 53
1

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.

BenMorel
  • 34,448
  • 50
  • 182
  • 322
Michael
  • 111
  • 2
  • Yes, this is a correct observation, but I know that `threadParamAbort_` will never have been deleted when exeucting this line of code since my code is structured as such. Unless there is a bug, of course, but that's a competely different problem. – gablin Sep 01 '10 at 11:21
0

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

Nemanja Trifunovic
  • 24,346
  • 3
  • 50
  • 88
  • It does indeed look very similar to what I've applied in my program. But should I still use a critical section? Is its need warranted in my program? – gablin Aug 31 '10 at 21:44
  • No need to reply to that (look at the discussion of one of the other answers). – gablin Aug 31 '10 at 22:11
0

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).

hookenz
  • 36,432
  • 45
  • 177
  • 286
  • If reading from and writing to `threadParamAbort_` is indeed not atomic, then I would need to worry about it on a single CPU system as well. And as I've too pointed out to everyone, I *know* that `volatile` does not provide atomicity to any operations; so can we please stop pointing that out to me? – gablin Sep 01 '10 at 11:39
  • Settle down and listen. I wrote this originally 2-3 days ago so no need to say anything to me. Mine is not a new answer ok? Atomicity is a different but related to thread safety which is all that you have mentioned in your question. Now I have updated my response after thinking more about what it is you are needing. – hookenz Sep 02 '10 at 02:01