locked
Thread safety using bool quit / cancel flag for worker thread RRS feed

  • Question

  • I'm after peoples thoughts on the use of a 'bool' (c++ built-in type) quit flag for terminating worker threads.

    I have a main application which creates a single (and relatively simple) worker thread that I set running to do an intensive operation without blocking the main UI thread.  A new thread is created every time the operation is required and the app should enforce that the user cannot start this operation if it is already running (they can do other operations though).  Therefore there should never be more than one worker thread running.

    The main application and the worker thread share data through a shared class CMyRunData which is passed to the ThreadProc upon creation which contains 2 bool flags, m_bCancel and m_bThreadRunning.
    I use a cancel flag that can be set from the main app which is checked periodically by the worker thread which will cancel processing and terminate if set.

    void CMyMainApp::OnUserClickDoProcessing()
    {

       // Dont allow new thread to be created if old one is still running
       if ( m_myRunData.m_bProcessing )
       {
          AfxMessageBox("Oi!  Still processing, wait until finished!");
          return;
       }

       // Create a new worker thread
       CWinThread* pThread = AfxBeginThread(ThreadProc, (LPVOID)m_myRunData, THREAD_PRIORITY_NORMAL, 0, CREATE_SUSPENDED);

       if (pThread)
       {
          m_myRunData.m_bProcessing = true;
          m_myRunData.m_bCancel = false;
          pThread->ResumeThread();

          // pThread auto-deletes itself when thread ends
       }
    }

    void CMainApp::OnUserCancel()
    {
       m_myRunData.m_bCancel = true;
    }

    UINT ThreadProc(LPVOID pRunData)
    {
       CMyRunData* pRunData = static_cast<CMyRunData*>(pRunData);
       bool bFinished = false;

       while(!bFinished && !pRunData->m_bCancel)
       {
             // Do some processing that sets bFinished when complete     
       }

       pRunData->m_bProcessing = false;
    }

    I don't want to add synchronisation code unless it is strictly necessary.

    Given that the worker thread simply starts, processes then dies, I see no need to use Events in the case.  The app guards against more than one thread being active at the same time so a simple bool cancel flag and finished flag seem would appear to be entirely sufficient for the purpose. 

    My Questions are as follows:

    1) Is is safe to use a simple bool in the above scenario?
    2) Presumably even if a thread switches whilst partially through a set operation, it will still either read as true or false in the other thread?
    3) The only way I can see this being a problem is if the bool type is implemented by the compiler as a bit field that gets shared with other bool variables. In this case point 2 would no longer hold true as other variables may cause activity in that memory location which could be interpreted erroneously if interrupted mid-set.
    4) If point 3 is an issue, presumably I could use a UINT type instead and use bFlag++ to set (I understand that increment is an atomic operation on Intel) ?

    This one has bugged me for years on and off.  Any thoughts people?
    Wednesday, June 10, 2009 12:18 PM

Answers

  • armarw wrote:
    > void CMainApp::OnUserCancel()
    > {
    > m_myRunData.m_bCancel = true;
    > }
    >
    > UINT ThreadProc(LPVOID pRunData)
    > {
    > CMyRunData* pRunData = static_cast(pRunData);
    > bool bFinished = false;
    >
    > while(!bFinished && !pRunData->m_bCancel)
    > {
    > // Do some processing that sets bFinished when complete
    > }
    >
    > pRunData->m_bProcessing = false;
    > }
    >
    > 1) Is is safe to use a simple bool in the above scenario?

    Technically, you have a data race here so the program is invalid. On
    certain multiprocessor architectures with weak cache coherence model, it
    may take arbitrary time (theoretically, forever) for one CPU to notice
    that the other has changed the flag. Typical consumer systems use
    architectures with strong cache coherency (such as that found in x86
    processors), so you are unlikely to notice the problem in practice.

    If you want to be completely safe, declare the flag as LONG, set it with
    InterlockedExchange and read it with InterlockedCompareExchange

    > 2) Presumably even if a thread switches whilst partially through a
    > set operation, it will still either read as true or false in the
    > other thread?

    It shouldn't be able to - I can't think of an architecure where setting
    a boolean variable wouldn't be a single machine instruction. Thread
    switches occur between instructions, they cannot occur in the middle of
    one.

    > 3) The only way I can see this being a problem is if the bool type is
    > implemented by the compiler as a bit field that gets shared with
    > other bool variables.

    This is highly unlikely under the current C++ standard (the compiler
    would have to prove that you don't take an address of the variable
    anywhere in the whole program), and would be outright illegal in the
    upcoming version of C++ standard (one that actually specifies how
    multiple threads should interact).
    --
    Igor Tandetnik


    • Marked as answer by armarw Wednesday, June 10, 2009 5:38 PM
    Wednesday, June 10, 2009 2:33 PM

All replies

  • I believe that using bool's in this way is safe.

    David Wilkinson | Visual C++ MVP
    Wednesday, June 10, 2009 12:28 PM
  • Yes it is safe to share a bool between threads, without synchronization.  The operations you have described will work OK.  But you have not described how you will close the app safely, making sure that the thread exits before you close and before m_myRunData is destroyed. 
    Wednesday, June 10, 2009 12:42 PM
  • Since the m_bCancel member is accessed from a concurrently executing thread, I think that it has to be declared with volatile keyword, especially if optimisation options are turned on.

    Wednesday, June 10, 2009 12:43 PM
  • It's okay, although not great.  In a multi-core machine, everybody has one today, the thread can keeping running for quite a while before ever seeing the m_bCancel member set to TRUE.  The processor cache needs to be flushed to see the update.  That is guaranteed to happen sooner or later, the thread scheduler is bound to flush the cache.  Later.  The volatile keyword isn't strong enough to cause a flush, you need a memory barrier.  Get one with InterlockedExchange() or the MemoryBarrier() macro.  In a worker thread scenario where the thread is usually blocking, waiting for work to do, you should use CreateEvent() and WaitForMultipleObjects().

    Hans Passant.
    Wednesday, June 10, 2009 2:24 PM
  • armarw wrote:
    > void CMainApp::OnUserCancel()
    > {
    > m_myRunData.m_bCancel = true;
    > }
    >
    > UINT ThreadProc(LPVOID pRunData)
    > {
    > CMyRunData* pRunData = static_cast(pRunData);
    > bool bFinished = false;
    >
    > while(!bFinished && !pRunData->m_bCancel)
    > {
    > // Do some processing that sets bFinished when complete
    > }
    >
    > pRunData->m_bProcessing = false;
    > }
    >
    > 1) Is is safe to use a simple bool in the above scenario?

    Technically, you have a data race here so the program is invalid. On
    certain multiprocessor architectures with weak cache coherence model, it
    may take arbitrary time (theoretically, forever) for one CPU to notice
    that the other has changed the flag. Typical consumer systems use
    architectures with strong cache coherency (such as that found in x86
    processors), so you are unlikely to notice the problem in practice.

    If you want to be completely safe, declare the flag as LONG, set it with
    InterlockedExchange and read it with InterlockedCompareExchange

    > 2) Presumably even if a thread switches whilst partially through a
    > set operation, it will still either read as true or false in the
    > other thread?

    It shouldn't be able to - I can't think of an architecure where setting
    a boolean variable wouldn't be a single machine instruction. Thread
    switches occur between instructions, they cannot occur in the middle of
    one.

    > 3) The only way I can see this being a problem is if the bool type is
    > implemented by the compiler as a bit field that gets shared with
    > other bool variables.

    This is highly unlikely under the current C++ standard (the compiler
    would have to prove that you don't take an address of the variable
    anywhere in the whole program), and would be outright illegal in the
    upcoming version of C++ standard (one that actually specifies how
    multiple threads should interact).
    --
    Igor Tandetnik


    • Marked as answer by armarw Wednesday, June 10, 2009 5:38 PM
    Wednesday, June 10, 2009 2:33 PM
  • Viorel_ wrote:
    > Since the m_bCancel member is accessed from a concurrently executing
    > thread, I think that it has to be declared with volatile keyword,
    > especially if optimisation options are turned on.

    Volatile as defined in the C++ standard is (almost) never helpful for
    inter-thread communication. The problem is usually not in compiler
    optimizations, but in CPUs reordering instructions at execution time.

    In the OP's example, the only way the compiler could optimize away a
    check for pRunData->m_bCancel is if the body of the loop is so simple as
    to be completely inlined (in particular, no calls to functions in other
    DLLs, including Win API calls). Otherwise, the compiler has to assume
    that pRunData could be stored in a global variable somewhere, and any
    external function could modify m_bCancel through that pointer (a
    situation known as aliasing). So, again, most of the time it's not the
    compiler optimizations we need to be worrying about.

    Having said that, as of VS 2005 (if I recall correctly), accessing a
    volatile variable also generates memory barrier instructions, and thus
    a) prevents instruction reordering, and b) flushes CPU cache. This is
    not the case for other compilers, or for older MSVC compilers.
    --
    Igor Tandetnik


    Wednesday, June 10, 2009 2:53 PM
  • Theoretically the volatile keyword could instruct the compiler to perform all of required stuffs related to multithreading and mentioned above (e.g. use intrinsic interlocked operations etc.). Otherwise it is almost useless. The MSDN sample for this keyword shows usage of a boolean flag similar to original question.
    Wednesday, June 10, 2009 3:17 PM
  • Thanks very much, thats great folks. Some really useful info in there.

    Sounds as though I can scrape by with the current solution but a better one would be to use a LONG with InterlockedExchange() to ensure cache consistency.

    In answer to Scott's point, I was intending to ensure that the thread cannot outlive the CMyRunData by forcing a cancel when the app is closing and then waiting on the thread handle (stored when thread created in m_hThread) until it dies, thus:

    void CMainApp::OnDestroy()
    {

       // If thread was started and not yet flagged as finished
       if (m_pRunData->m_bProcessing)
       {

          // Set cancel flag
          m_pRunData->m_bCancel = true;

          // wait for thread to die
          ::WaitForSingleObject(m_hThread, INFINITE);

       }

       // m_pRunData gets destroyed
      
    }

    Based on the above answers re multi-core processors, it seems as though the thread could exit (setting m_bProcessing=false) and there be some long delay between the CMainApp thread getting the m_bProcessing update.  Therefore there may be an issue with waiting on m_hThread if the thread has long since died?  I guess this problem is also solved by using InterlockedExchange().

    FURTHER POSSIBLE MULTI-CORE ISSUE

    This multi-core issue raises another point for me. Whilst the above was a simple example, in a seperate project I am likely to be using more of a producer-consumer model:

    A producer worker thread puts data into a CArray protected by a CCriticalSection.  The main app thread then reads the data out.  Is there something I need to do in order to ensure that the memory occupied by the CArray doesn't end up being cached by the multi-cores and not reflect the updates?
    Wednesday, June 10, 2009 3:35 PM
  • Viorel_ wrote:
    > Theoretically the volatile keyword could instruct the compiler to
    > perform all of required stuffs related to multithreading and
    > mentioned above (e.g. use intrinsic interlocked operations etc.).
    > Otherwise it is almost useless.

    It's useful for its original intended purpose - reading and writing memory-mapped hardware registers. It is in indeed almost useless for the purpose it was never designed to address.

    > The MSDN sample for this keyword
    > shows usage of a boolean flag similar to original question.
     
    Note how this example doesn't exist in the VS2003 version of the same article (choose the version in the gray box at the top right).
     
    So yes, if you are happy to restrict yourself to VS2005 and up, then you can rely on this extension of volatile's behavior. Just realize that your programs are non-portable (which admittedly is not a concern for many developers).
     
    Coming "soon" (which in practice could mean "within several years"), we'll have C++0x standard and its atomic<bool> variables. Those will behave just the way you expect (but the standard doesn't guarantee) volatile bool to behave.
    --
    Igor Tandetnik
    Wednesday, June 10, 2009 3:36 PM
  • armarw wrote:
    > Therefore there may be an issue with waiting
    > on m_hThread if the thread has long since died?

    No, it's perfectly OK. Thread handle is valid until closed, regardless
    of whether the thread is actually running or has already terminated.
    Otherwise there would be no way to use the handle in WaitFor...
    functions reliably.

    > A producer worker thread puts data into a CArray protected by a
    > CCriticalSection. The main app thread then reads the data out. Is
    > there something I need to do in order to ensure that the memory
    > occupied by the CArray doesn't end up being cached by the multi-cores
    > and not reflect the updates?

    No. Entering and leaving a critical section executes all the necessary
    memory barriers. Basically, as long as you use OS-provided
    synchronization primitives, you are safe (assuming you use them
    correctly, of course). Tricky issues only arise if you try to be clever
    and avoid them.
    --
    Igor Tandetnik


    Wednesday, June 10, 2009 3:56 PM
  • Excellent stuff, many thanks to everyone who replied and especially to Igor and Nobugz for your insightful contributions.  I hand't considered a multi-core aspect to this issue.
    Wednesday, June 10, 2009 5:37 PM
  • Agreed, just some side-notes.  Just beware that the m_bCancel flag or InterlockedExchange() is an inherent race condition.  The thread is not instantly going to recognize this, it needs to trundle on to the code where the flag is actually checked.  In other words, setting the flag isn't good enough to assume that you won't have concurrency issues past that.  You'll need some kind of acknowledgement from the thread that it has indeed seen the flag.  WaitForSingleObject() on the thread handle in this case is quite suitable.  That should make the simple boolean flag work too.

    The volatile keyword does not generate a memory barrier in any version of the MSVC compiler that I'm aware of.

    Yes, all Windows synchronization objects use memory barriers, a critical section will work just fine.  Fwiw, it is not the greatest way to sync in a producer/consumer scenario.  Something like the .NET's ReaderWriterLockSlim can do a better job at improving concurrency.  That's not something you should try to do yourself, lock design is *hard*.  If you at all consider a better mouse trap, be sure to use one only from a trusted source. 
    Hans Passant.
    Wednesday, June 10, 2009 5:54 PM
  • Thanks very much, thats great folks. Some really useful info in there.
    I should have mentioned that what I often do in the worker thread to check for cancellation is use SendMessage() back to the main thread at the end of each "iteration". You can use the return value to indicate the cancellation.

    As always with SendMessage() you must be sure that the main thread cannot block.

    David Wilkinson | Visual C++ MVP
    Wednesday, June 10, 2009 6:31 PM
  • nobugz wrote:
    > The volatile keyword does not generate a memory barrier in any
    > version of the MSVC compiler that I'm aware of.

    http://msdn.microsoft.com/en-us/library/12a04hfd(VS.80).aspx

    "- A write to a volatile object (volatile write) has Release semantics;
    a reference to a global or static object that occurs before a write to a
    volatile object in the instruction sequence will occur before that
    volatile write in the compiled binary.
    - A read of a volatile object (volatile read) has Acquire semantics; a
    reference to a global or static object that occurs after a read of
    volatile memory in the instruction sequence will occur after that
    volatile read in the compiled binary.
    This allows volatile objects to be used for memory locks and releases in
    multithreaded applications."

    Note that, if you actually expect to see memory barrier instructions in
    generated assembly, you need to compile for Itanium. x86 as well as
    AMD64 architectures feature strong cache coherence and lack such
    instructions.
    --
    Igor Tandetnik


    Wednesday, June 10, 2009 10:02 PM
  • Hmya, quotes.  Is anybody still posting to news groups?  You left out the tricky part:

    This allows volatile objects to be used for memory locks and releases in multithreaded applications.

    Hans Passant.
    Thursday, June 11, 2009 2:25 AM
  • nobugz wrote:
    > Hmya, quotes. Is anybody still posting to news groups?

    Yes, there is a thriving community at news://news.microsoft.com

    > You left out
    > the tricky part:
    >
    > This allows volatile objects to be used for memory locks and releases
    > in multithreaded applications.

    First, what do you mean "left out"? It's right there in my post for
    everyone to see.

    Second, this statement actually supports my point (that accessing a
    volatile variable generates appropriate memory barriers on recent enough
    MSVC compilers - that's why they can be used to implement locks). How
    exactly do you believe it helps your claim that "The volatile keyword
    does not generate a memory barrier"?
    --
    Igor Tandetnik


    Thursday, June 11, 2009 2:41 AM
  • While the C++ standard has nothing to say about volatile and memory barriers, VC++ 2005 and later do in fact apply memory barriers to volatile data members. In fact, there was a bug on MS Connect for the IA64 compiler for a while that it didn't do this, even though that behavior was (at the time) undocumented, IIRC.

    Also note that the C++/CLI standard does guarantee a memory barrier for volatile data members, so if you're compiling with /clr it's definitely sufficient.
    Thursday, June 11, 2009 5:31 PM