1
#include <thread>
#include <mutex>
#include <condition_variable>
#include <iostream>
#include <chrono>
using namespace std;
using namespace std::chrono_literals;

condition_variable cv;
mutex mu;

void thread1()
{
    mu.lock();
    unique_lock lck(mu);
    cv.wait(lck);
    cout << 1;
}

void thread2()
{
    this_thread::sleep_for(1s);
    mu.lock();
    cv.notify_all();
    cout << 2;    
}

int main()
{
    thread t1(thread1);
    thread t2(thread2);
    this_thread::sleep_for(2s);
}

This code was expected to show number 1 and 2, but shows nothing. If a condition variable has waited with a mutex, then the mutex is unlocked, right?

Jan Schultke
  • 17,446
  • 6
  • 47
  • 96
zhzhy
  • 19
  • 2
  • The code is not correct. It has a condition race. `thread2` may lock mutex first and raise notify to nowhere. `sleep(1)` is absolutely wrongly used as a synchronization point. `sleep(2)` is absolutely wrong too. You get a program terminate. Use `t1.join()` and `t2.join()`. See the proper usage example: [std::condition_variable](https://en.cppreference.com/w/cpp/thread/condition_variable) – 273K Aug 16 '23 at 06:18
  • The code as posted is calling `mu.lock()` without any corresponding `mu.unlock()`, which leads to problems. Get rid of your explicit calls to `mu.lock()`, and add a `unique_lock lck(mu);` line to `thread2()`. Also add `t1.join(); t2.join()` to the end of `main()` to guarantee that your program won't exit before the threads do. – Jeremy Friesner Aug 16 '23 at 06:19
  • 1
    Note a condition_variable is not a variable it is a signal. So you always need an extra variable to test once the condition variable is signalled. You should also ALWAYS wait with a predicate (which will test your variable), this is to avoid problems with spurious wakeups. Read more here : https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables/. – Pepijn Kramer Aug 16 '23 at 06:21
  • Also never lock mutexes manually, use [std::unique_lock(https://en.cppreference.com/w/cpp/thread/unique_lock) to lock and unlock your mutex for you., This avoids deadlocks when your function exits somewhere halfway due to a return statement or an exception being thrown. – Pepijn Kramer Aug 16 '23 at 06:23
  • 1
    My answers here might be relevant: (1) https://stackoverflow.com/questions/71882767/multi-threaded-input-processing/71883693#71883693, (2) https://stackoverflow.com/questions/72260569/minimal-mutexes-for-stdqueue-producer-consumer/72261477#72261477. – wohlstad Aug 16 '23 at 06:37
  • Offtopic: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice – Marek R Aug 16 '23 at 09:49

2 Answers2

3

Several issues with the code:

  • thread2 is locking the mutex, but not unlocking it.

  • thread1 is locking it twice. Once with the explicit mu.lock and again with the unique_lock constructor. This is undefined behavior - usually an automatic deadlock so the developer knows they have coded it incorrectly. The constructor of unique_lock that only takes a mutex as a parameter will automatically call mu.lock for you. And the destructor will call mu.unlock.

  • condition variables need some sort of "condition" to check to guard against spurious wakeup. In other words, when thread 1 wakes up, it can't assume that thread 2 was the one that woke it up. It's not a good pattern to assume so. So while having the mutex locked, the code needs to check for "has the thing I'm waiting for actually happened". And if not, then it needs to go back to waiting. I've updated your code below to simulate thread 2 setting a global variable and having thread 1 wait for it to change.

  • No need to sleep(2) in main. Just wait for both threads to be done to decide it's ok to exit main.

Closer to what you want:


condition_variable cv;
mutex mu;
bool has_value_been_set = false;
int value = 0;

void thread1()
{
    unique_lock<mutex>lck(mu);
    while (has_value_been_set == false)
    {
        cv.wait(lck);
    }
    
    cout << "thread1: the value set by the other thread is: " << value << "\n";

}
void thread2()
{
    sleep(1);
    {
        unique_lock<mutex>lck(mu);
        value = 42;
        has_value_been_set = true;
        cout << "thread2: setting the value: " << value << "\n";
    }

    cout << "thread2: notifying other threads: " << value << "\n";
    cv.notify_all();

    
}
int main()
{
    thread t1(thread1);
    thread t2(thread2);


    // wait for both threads to be done
    t1.join();
    t2.join();

}
selbie
  • 100,020
  • 15
  • 103
  • 173
1

Like this :

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

using namespace std::chrono_literals;

std::condition_variable cv;
std::mutex mu;
int value{ 1 }; // (one of) the values associated with the condition_variable

void thread1()
{
    std::unique_lock<std::mutex> lock{mu};

    // always wait with a predicate, in this case I use a lambda expression
    // this prevents the wait from falling through
    // if the predicate is not true
    // sometimes wait wakes up even if the condition_variable has not been notified (they sometimes wakeup due to OS limitations)
    cv.wait(lock, [&] {return value == 1; });
    std::cout << "1\n";

    value = 2;
    // notify all blocked thread so their waits can check their predicates
    cv.notify_all();
}

void thread2()
{
    std::unique_lock<std::mutex> lock{mu};
    cv.wait(lock, [&] {return value == 2; });
    std::cout << "2\n";
}

int main()
{
    // in C++20 you can also use std::jthread (which will join in destructor)
    // or if you use std::thread don't forget to call join 
    auto future1 = std::async(std::launch::async, thread1); // destructor of future will join at program shutdown
    auto future2 = std::async(std::launch::async, thread2);
    
    std::cout << "waiting for 2" << "\n";
    future2.get();
    std::cout << "done";
    return 0;
}
Pepijn Kramer
  • 9,356
  • 2
  • 8
  • 19