3

Why the condition variable is stuck on waiting if it was notified in worker_thread? What am I missing here?

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

std::mutex m;
std::condition_variable cv;

void worker_thread()
{
    cv.notify_one();
}

int main()
{
    std::thread worker(worker_thread);

    std::cout << "Start waiting..." << std::endl;
    std::unique_lock<std::mutex> lk(m);
    cv.wait(lk);
    std::cout << "Finished waiting..." << std::endl;

    worker.join();

    getchar();
}
einpoklum
  • 118,144
  • 57
  • 340
  • 684
theateist
  • 13,879
  • 17
  • 69
  • 109

2 Answers2

2

Your problem is that cv.notify_one() only wakes threads that are currently waiting. cv doesn't remember you notified it, and someone later comes along and waits.

Your worker thread is outpacing your main thread. So the notify happens before the main thread.

This is just a symptom of your real problem; you are using a condition variable wrong. Barring extremely advanced use, all use of condition variable should be in a triple.

  1. A std::condition_variable.

  2. A std::mutex.

  3. A payload.

Your code is missing the payload.

To signal, you:

std::unique_lock<std::mutex> l(m);
payload = /* move it to a 'set' or 'non-empty' state */;
cv.notify_one(); // or all

to listen you:

std::unique_lock<std::mutex> l(m);
cv.wait(l, [&]{ return /* payload is in a set or non-empty state */; });
// while locked, consume one "unit" of payload from the payload.

with minor variations for wait_for and the like.

Following this cargo-cult pattern is important, as it avoids a number of pitfalls. It deals with both spurious wakeups with the wait happening after the notification.

Your code is missing a payload. So your code is vulnerable to both the waiting thread outrunning the signaling thread, and spurious wakeups.

Note that getting "clever" here is highly discouraged. For example, deciding that "I'll use an atomic variable to avoid using a mutex when signaling" actually doesn't work. Either follow the above recipe dogmatically, or go and spend a few months learning the threading and memory model of C++ well enough to improvise.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • _«For example, deciding that "I'll use an atomic variable to avoid using a mutex when signaling" actually doesn't work»_ True, I hit that. Well, I didn't, but it had never occurred to me until one day someone much like your fine self wrote something about it on SO and I realised that my code was vulnerable to antics. – Lightness Races in Orbit Oct 18 '18 at 18:36
  • @LightnessRacesinOrbit I suspect I deserve to be downvoted, however, as I claim you can learn the C++ threading/memory model in a mere few months. ;) – Yakk - Adam Nevraumont Oct 18 '18 at 18:38
  • Even if that were possible, and I'm not convinced that it is, it would surely be at great cost to sanity and soul.... – Lightness Races in Orbit Oct 18 '18 at 18:46
  • @LightnessRacesinOrbit At least [1/1d8 SAN](https://rpg.rem.uz/Call%20of%20Cthulhu/Misc/CoC%20-%20Self%20Sanity.pdf). – Yakk - Adam Nevraumont Oct 18 '18 at 18:49
  • Maybe it's wrong, but what if I replace it with promise/future? Or I might have the same issue if `promise.set_value` happens before `future.get`? – theateist Oct 18 '18 at 19:05
  • @theateist no, promise/future is far less raw low level than condition variable. – Yakk - Adam Nevraumont Oct 18 '18 at 19:17
  • @Yakk-AdamNevraumont, can you explain more? Does promise/future also have the issue like conditional variable? – theateist Oct 18 '18 at 19:47
  • @Yakk-AdamNevraumont Yes, threading is very hard to get right. But I think if he started with Java 7/8's threading {Java's memory model is simple, somehow easy to get, and still have some reordering, but only `cst_seq` is supported in Java 7/8}, then begin to learn the C++ memory model, he would learn a lot. But it is really hard to be worked in production code. And also there is another solution is to use some library that implements futures out-of-the-box, and read it in his free time, do you know a good one ?? – user9335240 Oct 18 '18 at 19:49
  • @theateist yes, no. – Yakk - Adam Nevraumont Oct 18 '18 at 20:05
  • @user9335240 `std` is a library that implements futures out-of-the-box? I am unaware of a good book that covers threading; threading is one of the hard problems in CS, and honestly procedural threading models with locks and the like simply doesn't scale. – Yakk - Adam Nevraumont Oct 18 '18 at 20:07
  • @Yakk-AdamNevraumont Yeah, locks are bad. Lock-free is hard to do correctly. About books, I think there are many, also try to see this course http://www.modernescpp.com/index.php/i-m-proud-to-present-modern-c-concurrency-is-available-as-interactive-course . Also there is a tool called [CppMem](http://svr-pes20-cppmem.cl.cam.ac.uk/cppmem/) that acts as a "simulator" that simulate a weakly ordered processor, and gives you the different cases that could happen with your lock-free program – user9335240 Oct 18 '18 at 20:23
  • @user No, that isn't what I am talking about. – Yakk - Adam Nevraumont Oct 18 '18 at 21:22
  • @Yakk-AdamNevraumont, so I can use replace conditional variable with promise/future and I won't have this issue anymore? – theateist Oct 18 '18 at 22:01
1

notify_one will unblock a waiting thread if there is one. If there are no waiting threads, nothing happens. A condition_variable does not have a state to remember how many threads should be notified when it is waited on.

1201ProgramAlarm
  • 32,384
  • 7
  • 42
  • 56
  • 1
    Can you elaborate on this? `cv.wait()` is in the main thread. So, what do you mean there is no waiting threads? BTW, if I add `Sleep(1000)` before `notify_one`, then it works. – theateist Oct 18 '18 at 04:35
  • You start the worker thread, which will probably execute its notify call almost immediately then return. Meanwhile your main thread does some output before calling wait. By the time it gets to the wait, your worker thread has ended. – 1201ProgramAlarm Oct 18 '18 at 04:37
  • Right, it means that the `condition_variable` is already in `signaled` state. So, when `wait` is executed it should stop waiting since that `condition_variable` is already `signaled`, isn't it? – theateist Oct 18 '18 at 04:41
  • 1
    Condition variables don't have a state, signaled or otherwise. Read the documentation at the link in my answer: "If any threads are waiting on *this, calling notify_one unblocks one of the waiting threads." – 1201ProgramAlarm Oct 18 '18 at 04:42
  • how can I fix it beside adding `Sleep`? Maybe to restructure the code somehow? – theateist Oct 18 '18 at 04:50
  • You can *favour composition over inheritance*, make a `struct`, `class` that wraps `std::mutex`, and have an `std::atomic_flag`, or `std::atomic_bool` variable, that when notified, it atomically **set the flag** then `notify`. – user9335240 Oct 18 '18 at 06:04
  • @user9335240 Simply adding an atomic will make the problem rarer, it won't make the OP's code correct. Threading is hard to get right. – Yakk - Adam Nevraumont Oct 18 '18 at 18:27
  • @theateist Don't wait for something that has already happened. After you acquire the mutex, but before you decide to wait, check if the thing you are waiting for has already happened. The entire purpose of condition variables is to provide an atomic "unlock and wait" operation to allow you to do exactly this. – David Schwartz Oct 18 '18 at 21:11