Is this use of condition variable safe (taken from cppreference.com)

Question

Is this code taken from Example of Producer/Consumer with condition variable safe? Me and a co-worker had a disagreement whether the omission of std::unique_lock lock(m) in close() is safe. The line done = true; is set without a lock. Can the notification get to the other thread without the data being synchonized?

#include <condition_variable>
#include <mutex>
#include <thread>
#include <iostream>
#include <queue>
#include <chrono>

class condvarQueue
{
    std::queue<int> produced_nums;
    std::mutex m;
    std::condition_variable cond_var;
    bool done = false;
    bool notified = false;
public:
    void push(int i)
    {
        std::unique_lock<std::mutex> lock(m);
        produced_nums.push(i);
        notified = true;
        cond_var.notify_one();
    }

    template<typename Consumer>
    void consume(Consumer consumer)
    {
        std::unique_lock<std::mutex> lock(m);
        while (!done) {
            while (!notified) {  // loop to avoid spurious wakeups
                cond_var.wait(lock);
            }   
            while (!produced_nums.empty()) {
                consumer(produced_nums.front());
                produced_nums.pop();
            }   
            notified = false;
        }   
    }

    void close()
    {
        done = true;
        notified = true;
        cond_var.notify_one();
    }
};

int main()
{
    condvarQueue queue;

    std::thread producer([&]() {
        for (int i = 0; i < 5; ++i) {
            std::this_thread::sleep_for(std::chrono::seconds(1));
            std::cout << "producing " << i << '\n';
            queue.push(i);
        }   
        queue.close();
    }); 

    std::thread consumer([&]() {
         queue.consume([](int input){
             std::cout << "consuming " << input << '\n';
         });
    }); 

    producer.join();
    consumer.join();
}

The C++ standard doesn't guarantee that `bool` is atomic, but in practice it is [citation needed]. This is the type of thing I'd change to a `std::atomic` for that one embedded system 15 years from now that uses a 64-bit alternating 1/0 pattern for `bool`. — JohnFilleau, Apr 08 '20 at 15:49
https://en.cppreference.com/w/cpp/thread/condition_variable/notify_one explicitly says that the lock is not required. Personally, I always put it, just to me safe. — Daniel Junglas, Apr 08 '20 at 16:26
@JohnFilleau Does not the use of the condition variable introduce a barrier that forces syntonization? — Martin York, Apr 08 '20 at 18:27
@MartinYork I didn't put enough disclaimers in my first comment. Let me do that here: I'm a C++ novice and very inexperienced with concurrency in general. To be clear: I dunno! — JohnFilleau, Apr 08 '20 at 18:29
I have now updated the example at cppreference.com with the accepted answer. Thanks for helping out. — Nisse Hult, Apr 08 '20 at 18:41

Howard Hinnant · Accepted Answer · 2020-04-08T18:51:46.657

close requires locked ownership of m while setting done and notified. And it isn't about an ancient architecture, it is about modern cached architectures. Without the locked ownership of m, there's no guarantee that the updated values of done and notified are flushed beyond close's cache line so that they can be read by consume.

Whether or not m is locked during cond_var.notify_one() is not as important. Both are correct and I've seen articles arguing that one or the other is the most performant.

In summary, either of these are correct:

void close()
{
    std::lock_guard lock{m};
    done = true;
    notified = true;
    cond_var.notify_one();
}

or:

void close()
{
    {
        std::lock_guard lock{m};
        done = true;
        notified = true;
    }
    cond_var.notify_one();
}

Note, I'm assuming C++17 with the lack of <std::mutex> on lock_guard. I could have also used unique_lock instead of lock_guard here, but lock_guard is the simplest tool for this job.

Is this use of condition variable safe (taken from cppreference.com)

1 Answers1