CP guidelines for condition variables

[jwakely]

Sorry this isn't complete, I'll try to flesh it out.

Condition variables are not semaphores. Notifications will be missed if they are sent when no other thread is blocked waiting on the condition variable, so you must not just rely on notify_one() or notify_all() to signal another thread, you must always have some predicate that is tested, e.g. a boolean flag (protected by the same mutex that is used when waiting on the condition variable). With a predicate that tests some condition, even if the notification is sent before the other thread waits on the condition variable then it won't be "missed" because the predicate will be true and the wait will return immediately.

Bad:

std::condition_variable cv;
std::mutex mx;

void thread1() {
  // do some work
  // ...
  // wake other thread
  cv.notify_one();
}

void thread2() {
  std::unique_lock<std::mutex> lock(mx);
  cv.wait(lock);  // might block forever
  // do work ...
}

Good:

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

void thread1() {
  // do some work
  // ...
  // make condition true:
  std::lock_guard<std::mutex> lock(mx);
  ready = true;
  // wake other thread
  cv.notify_one();
}

void thread2() {
  std::unique_lock<std::mutex> lock(mx);
  cv.wait(lock, []{ return ready; });
  // do work ...
}

[tvaneerd]

I really like this suggestion (and the mutex locking/unlocking one as well)

For example code, I try to avoid // do some work and instead use do_some_work(), because example code already uses comments for 2 things - normal code comments like // wake other thread and meta-example comments // might block forever. So adding a third type of "pretend code was here" comment just adds to the problem.

std::condition_variable cv;
std::mutex mx;

void thread1() {
     do_some_work();
     // let others know work is done
     cv.notify_one();
}

void thread2() {
     std::unique_lock<std::mutex> lock(mx);
     // wait for some_work to be done:
     cv.wait(lock);  // ** might block forever!? **
     // ...
}

Also, you might want to explicitly explain the example here with "If thread1() finishes quickly and calls notify before thread2() gets to cv.wait(), then thread2() will wait() forever (as the notification was "missed")." I know that's already what you said, but referring directly to the example helps. Might want to also add "If you think you "know" that thread1() will "never" finish first (eg because it takes so long), don't rely on that. It is a bug waiting to happen." (that wasn't well said, but something along those lines)

Also consider mentioning "spurious wakeups" which are confusing to novices, but another reason why you need the separate condition, even if you "know" thread1() will take longer than thread2.

[tamaskenez]

Subtle detail but release lock before notify. See std::condition_variable::notify_one / Notes, 2nd paragraph.

void thread1() {
    do_some_work();
    // let others know work is done
    {
        std::lock_guard<std::mutex> lock(mx);
        ready = true;
    }
    // wake other thread
    cv.notify_one();
}

[jwakely]

@tamaskenez, unlocking the mutex before notifying is an optimisation, and not essential. I intentionally didn't do that, to keep the example simple. There could be a second guideline about that point, but it's not related to the "always use a predicate" rule. I would object strongly to complicating the example by doing that.

[jwakely]

@tvaneerd great point about using a do_some_work() function instead of comments. There could certainly be more discussion of how the deadlock can be reached, and how spurious wake-ups can happen (and aren't a problem if you use the predicate form of wait/wait_for/wait_until).

I'll incorporate those suggestions to flesh it out further.

[cubbimew]

Could this incorporate or list separately my old bit from the TODO chapter of the guidelines, "atomic bool whose value is set outside of the mutex is wrong". It seems to be a recurring misconception, a recent repeat on stackoverflow: http://stackoverflow.com/a/35775651/273767 an older one http://stackoverflow.com/q/35411571/273767 and I certainly recall more

[jwakely]

@cubbimew yes, I think it makes sense to combine "always use a predicate to test some condition" and "always use the same mutex to wait and to make the condition true" into one guideline.

[AndrewPardoe]

We generally like this idea. There are at least two separate rules in here, one about avoiding spurious wakeups and one about not missing wakeups. We'd at least want this split into two separate rules.

Additionally we might want to discuss the pattern that a condition variable needs to be associated with a mutex.

Assigning this to Herb to integrate a first draft of this guidance. Thank you, Jonathan!

[cubbimew]

yet another question on SO asking what's wrong with using unprotected atomic bool as a condition http://stackoverflow.com/questions/38147825

[cubbimew]

The main part of this issue appears to be have been written as CP.42: Don't wait without a condition back in april e8dea38 although the part about always using a mutex (and the same mutex) to set that condition is still important to note.

[cubbimew]

...and another StackOverflow sighting of the mutex use Why do I need to acquire a lock to modify a shared “atomic” variable before notifying condition_variable