Using std::condition_variable with atomic<bool>

Question

There are several questions on SO dealing with atomic, and other that deal with std::condition_variable. But my question if my use below is correct?

Three threads, one ctrl thread that does preparation work before unpausing the two other threads. The ctrl thread also is able to pause the worker threads (sender/receiver) while they are in their tight send/receive loops. The idea with using the atomic is to make the tight loops faster in case the boolean for pausing is not set.

class SomeClass
{

public:
    //...                                                                                                                                                                                                                                                                                                                                                                                   
    // Disregard that data is public...                                                                                                                                                                                                                                                                                                                                                     

    std::condition_variable cv; // UDP threads will wait on this cv until allowed                                                                                                                                                                                                                                                                                                           
                                // to run by ctrl thread.                                                                                                                                                                                                                                                                                                                                   
    std::mutex cv_m;
    std::atomic<bool> pause_test_threads;
};

void do_pause_test_threads(SomeClass *someclass)
{
    if (!someclass->pause_test_threads)
    {
        // Even though we use an atomic, mutex must be held during                                                                                                                                                                                                                                                                                                                          
        // modification. See documentation of condition variable                                                                                                                                                                                                                                                                                                                            
        // notify_all/wait. Mutex does not need to be held for the actual                                                                                                                                                                                                                                                                                                                   
        // notify call.                                                                                                                                                                                                                                                                                                                                                                     
        std::lock_guard<std::mutex> lk(someclass->cv_m);
        someclass->pause_test_threads = true;
    }
}

void unpause_test_threads(SomeClass *someclass)
{
    if (someclass->pause_test_threads)
    {
        {
            // Even though we use an atomic, mutex must be held during                                                                                                                                                                                                                                                                                                                      
            // modification. See documentation of condition variable                                                                                                                                                                                                                                                                                                                        
            // notify_all/wait. Mutex does not need to be held for the actual                                                                                                                                                                                                                                                                                                               
            // notify call.                                                                                                                                                                                                                                                                                                                                                                 
            std::lock_guard<std::mutex> lk(someclass->cv_m);
            someclass->pause_test_threads = false;
        }
        someclass->cv.notify_all(); // Allow send/receive threads to run.                                                                                                                                                                                                                                                                                                                   
    }
}

void wait_to_start(SomeClass *someclass)
{
    std::unique_lock<std::mutex> lk(someclass->cv_m); // RAII, no need for unlock.                                                                                                                                                                                                                                                                                                          
    auto not_paused = [someclass](){return someclass->pause_test_threads == false;};
    someclass->cv.wait(lk, not_paused);
}

void ctrl_thread(SomeClass *someclass)
{
    // Do startup work                                                                                                                                                                                                                                                                                                                                                                      
    // ...                                                                                                                                                                                                                                                                                                                                                                                  
    unpause_test_threads(someclass);

    for (;;)
    {
        // ... check for end-program etc, if so, break;                                                                                                                                                                                                                                                                                                                                     
        if (lost ctrl connection to other endpoint)
        {
            pause_test_threads();
        }
        else
        {
            unpause_test_threads();
        }
        sleep(SLEEP_INTERVAL);

    }

    unpause_test_threads(someclass);
}

void sender_thread(SomeClass *someclass)
{
    wait_to_start(someclass);
    ...
    for (;;)
    {
        // ... check for end-program etc, if so, break;                                                                                                                                                                                                                                                                                                                                     
        if (someclass->pause_test_threads) wait_to_start(someclass);
        ...
    }
}

void receiver_thread(SomeClass *someclass)
{
    wait_to_start(someclass);
    ...
    for (;;)
    {
        // ... check for end-program etc, if so, break;                                                                                                                                                                                                                                                                                                                                     
        if (someclass->pause_test_threads) wait_to_start(someclass);
        ...
    }

Answer 1

I looked through your code manipulating conditional variable and atomic, and it seems that it is correct and won't cause problems.

Why you should protect writes to shared variable even if it is atomic:

There could be problems if write to shared variable happens between checking it in predicate and waiting on condition. Consider following:

1. Waiting thread wakes spuriously, aquires mutex, checks predicate and evaluates it to false, so it must wait on cv again.

2. Controlling thread sets shared variable to true.

3. Controlling thread sends notification, which is not received by anybody, because there is no threads waiting on conditional variable.

4. Waiting thread waits on conditional variable. Since notification was already sent, it would wait until next spurious wakeup, or next time when controlling thread sends notification. Potentially waiting indefinetly.

Reads from shared atomic variables without locking is generally safe, unless it introduces TOCTOU problems.

In your case you are reading shared variable to avoid unnecessary locking and then checking it again after lock (in conditional wait call). It is a valid optimisation, called double-checked locking and I do not see any potential problems here.

You might want to check if atomic<bool> is lock-free. Otherwise you will have even more locks you would have without it.