C++ thread deadlock on waiting condition

Question

Trying to expand in my two previous questions Move operations for a class with a thread as member variable and Call function inside a lambda passed to a thread

I'm not understanding why the thread doing a wait_for is somtimes not being notified wich results in a deadlock. Cppreference says on condition variables http://en.cppreference.com/w/cpp/thread/condition_variable/notify_one

The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock.

MCVE, the commented line explains what changes if I hold the lock, but I dont undrestand why:

#include <atomic>
#include <condition_variable>
#include <mutex>
#include <thread>

#include <iostream>

using namespace std;

class worker {
public:
    template <class Fn, class... Args>
    explicit worker(Fn func, Args... args) {
        t = std::thread(
            [&func, this](Args... cargs) -> void {
                std::unique_lock<std::mutex> lock(mtx);
                while (true) {
                    cond.wait(lock, [this]() -> bool { return ready; });
                    if (terminate) {
                        break;
                    }

                    func(cargs...);
                    ready = false;
                }
            },
            std::move(args)...);
    }

    ~worker() {
        terminate = true;
        if (t.joinable()) {
            run_once();
            t.join();
        }
    }

    void run_once() {
        // If i dont hold this mutex the thread is never notified of ready being
        // true.
        std::unique_lock<std::mutex> lock(mtx);
        ready = true;
        cout << "ready run once " << ready << endl;
        cond.notify_all();
    }

    bool done() { return (!ready.load()); }

private:
    std::thread t;
    std::atomic<bool> terminate{false};
    std::atomic<bool> ready{false};
    std::mutex mtx;
    std::condition_variable cond;
};

// main.cpp

void foo() {
    worker t([]() -> void { cout << "Bark" << endl; });
    t.run_once();
    while (!t.done()) {
    }
}

int main() {
    while (true) {
        foo();
    }
    return 0;
}

It's "ready=true:" that must happen under mutex. Doesn't matter that it's atomic. — Cubbi
@user2079303 Fixed it! Still its hard to see if execution "expires". — aram
@user2079303 Its not an instant deadlock at least not on my computer. Thats why I put a loop so I could call the function several times. — aram
@user2079303 Yes very misleading, my bad, again. Sorry. I fixed it just in case someone else has the same problem. — aram

Sven Nilsson Sven Nilsson · Accepted Answer · 2016-07-28T13:57:59

You need a memory barrier to make sure that the other thread will see the modified "ready" value. "ready" being atomic only ensures that the memory access is ordered so that modifications that happened before the atomic access are actually flushed to main memory. This does not guarantee that the other threads will see that memory, since these threads may have their own cache of the memory. Therefore, to ensure that the other thread sees the "ready" modification, the mutex is required.

{
  std::unique_lock<std::mutex> lock(mtx);
  ready = true;
}

C++ thread deadlock on waiting condition

1 Answers