Condvars and atomics do not mix

Rust doesn't solve this. You can easily make the same mistake as the article describes, in Rust.

Thiez points out that a Rust mutex wraps the data it protects. But that doesn't fix the issue described in the article. The C++ in the article also accesses all mutex-protected data safely. You could translate it almost line-for-line to Rust and still have the same race hazard.

It's because the race hazard is from wait-condition ordering, rather than mutex-protected data.

The article explains, it's tempting and intuitive to make kill_flag an atomic, outside the data protected by the mutex, because it's a sticky state, only written in one place.

I've seen this pattern used often enough in the real world, sometimes with the flag being a global variable. People do it. Shutdowns get stuck occasionally, nobody is sure why. Sometimes the flag is set in a signal handler, where it isn't safe to use mutex/notify operations anyway, and nobody thought hard enough about making those wakups reliable.

But those patterns lead to a race hazard with wait conditions. The article goes on to explain that kill_flag should be part of the data protected by the condition's mutex. And then there's no need to make it atomic.

After seeing that, it might be a surprise to find kill_flag doesn't have to be protected by the mutex after all!

That intuition that it can go outside was correct. It just needs a little ordering. The following code will also avoid the race hazard:

  // Notify all workers that they need to die right now.
  m_kill_flag = true;
  // Acquire and release the mutex to ensure condvar waiters reliably see all state changes.
  { std::unique_lock<std::mutex> lock(m_mutex); }
  // Wake all sleeping condvar waiters.
  m_has_work.notify_all();

I would not recommend this for the situation in the article. But you can see how it generalises, for example you might have a global kill flag or memory statistic that's used for many different things, protected by different mutexes.

Not sure why this is flagged, but there's nothing actually special about atomics here, I thought this was fairly common knowledge that when using condition variables you must always couple them with a mutex to provide a critical region and avoid lost wakeup. This is easier to see with the no-abstrations pthread version:

On the waiter side

   pthread_cond_lock(&mutex)
   while (!predicate) {
     pthread_cond_wait(&cvar, &mutex);
   }

On the signaler side

  pthread_mutex_lock(&mutex);
  toggle_predicate(&predicate);
  pthread_mutex_unlock(&mutex);
  pthread_cond_signal(&cvar);

(Note that it's safe to signal after unlock, although a bit more error-prone as you have to be careful when destroying the cvar). The mutex serves two key purposes: first is providing acq/rel semantics on observations of predicate, the second is that it provides a barrier region to ensure that the predicate is either changed before the waiter enters into the wait loop, or after the waiter has added itself to the wait queue so that signal will wake it up.

Changing the predicate var to atomic allows you to get rid of the first one (and I think it can in fact be a relaxed atomic since mutex lock already has acq/rel semantics), but you still need it to serve the second purpose.

See this SO answer for more info: https://stackoverflow.com/a/9918764

I don't have the most experience writing concurrent code but wouldn't semaphores have made more sense here? The issue was that condition variables aren't "sticky" yet semaphores are and so would not have that problem.

Since the post doesn't really explain it, the race condition is because when notify_all is called there might be a thread that already has acquired the lock and is running, so that thread won't get the notification, and might not wake up on the next iteration.

I think wrapping the call to notify_all in the lock would also fix the race condition.

Rust solves this (in common cases) by having the mutex wrap the data it protects. You can't make this mistake because you can't perform any operation without first taking the lock.

Of course this doesn't cover all usecases but hopefully the people who have the exceptional situation are smart enough to avoid the problem in the article.

The right solution when using atomics is to use a binary semaphore instead of condition variables, and implement the wait as

    if (!cond) {
      sem.try_acquire();
      if (!cond) {
        sem.acquire();
      }
    }

Though this only works if there is a single waiter.

Why this article was flagged?

Well, that's the sort of mistake even noob won't make.

C++, the infinite foot-gun factory