Deadlock after notify_one() (notifying thread locks mutex)

Question

I have following problem. I have some multiple threads that do some work and one main thread that wakes them up when work is available. So far, I have managed to write some code using conditional variables and mutexes and most of the time this works okay, but from time to time, notifying thread will lock the mutex right after call notify_one(), thus blocking the notified thread and deadlocking.

I have written minimal code to illustrate this situation.

#include <iostream>
#include <thread>
#include <condition_variable>

std::mutex lock;
std::condition_variable cv;

void foo() {
    std::cout << "Thread: Entering doWork()" << std::endl;
    std::unique_lock<std::mutex> l(lock);
    std::cout << "Thread: Acquired lock, going to wait." << std::endl;
    cv.wait(l , []{return true;});
    std::cout << "Thread: Done waiting, exit." << std::endl;
}

int main(void) {
    std::unique_lock<std::mutex> l(lock);
    std::cout << "MAIN: Creating thread." << std::endl;
    std::thread t(foo);
    std::cout << "MAIN: Unlocking mutex." << std::endl;
    l.unlock();
    std::cout << "MAIN: Notifying thread." << std::endl;
    cv.notify_one();
    //std::this_thread::sleep_for(std::chrono::seconds(1));
    l.lock();
    std::cout << "MAIN: Acquired lock." << std::endl;
    std::cout << "MAIN: Joining thread." << std::endl;
    t.join();
    return 0;
}

In ideal situation, the output should be

MAIN: Creating thread.
MAIN: Unlocking mutex.
Thread: Entering doWork()
Thread: Acquired lock, going to wait.
MAIN: Notifying thread.
Thread: Done waiting, exit.
MAIN: Acquired lock.
MAIN: Joining thread.

but more often than not it is

MAIN: Creating thread.
MAIN: Unlocking mutex.
MAIN: Notifying thread.
MAIN: Acquired lock.
MAIN: Joining thread.
Thread: Entering doWork()

Is there any better way to eliminate chance of deadlock except of adding sleep into notifying thread (which I don't want to do)? Thank you in advance.

Answer 1

It's "condition variable" not "conditional variable", and the reason it's called that is that you use it to wait on some condition.

You aren't doing that, you just wait on a lambda that always returns true, and that's the cause of your problem. That and the fact your main thread is holding the lock all the time (why?!)

Sometimes the main thread runs the unlock, the notify_one, and the lock quickly, before the foo thread has even started. That means the foo thread misses the notification, then tries to acquire the lock, but can't because the main thread has it.

A condition variable is not like a semaphore, the notify_one` call does not set a state that can be detected later. If the condition variable isn't waiting when the notify_one call happens then it misses it, and it is gone forever. If you miss the notification and then sleep you will never wake up.

The solution is not to add arbitrary sleeps, that doesn't solve anything (ever!)

The correct solution is to have a condition that is tested, and to stop holding the lock when you're not updating any shared data. In the example below the condition being tested is "is the boolean ready true?" and the foo thread will wait until that condition is true. The main thread sets the variable, making the condition true, and then notifies the other thread that it should re-check the condition.

#include <iostream>
#include <thread>
#include <condition_variable>

std::mutex lock;
std::condition_variable cv;
bool ready = false;

void foo() {
    std::cout << "Thread: Entering doWork()" << std::endl;
    std::unique_lock<std::mutex> l(lock);
    std::cout << "Thread: Acquired lock, going to wait." << std::endl;
    cv.wait(l , []{return ready;});
    std::cout << "Thread: Done waiting, exit." << std::endl;
}

int main(void) {
    std::cout << "MAIN: Creating thread." << std::endl;
    std::thread t(foo);
    {
        std::cout << "MAIN: Locking mutex." << std::endl;
        std::unique_lock<std::mutex> l(lock);
        ready = true;
    }
    std::cout << "MAIN: Notifying thread." << std::endl;
    cv.notify_one();
    std::cout << "MAIN: Joining thread." << std::endl;
    t.join();
}