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();
}

Answer 1

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.