race condition in WaitGroup class

[yoda-jm]

I think that the implementation is racy, especially the Add/Done side.
According to this link https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables atomic variable modification should be made under the lock

[qiandu2006]

I think that the implementation is racy, especially the Add/Done side.
According to this link https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables atomic variable modification should be made under the lock

Agreed on this.

// this would be called from the main thread
void WaitGroup::Add(int i) {
  std::unique_lock<std::mutex> l(mu_);
  counter_ += i;
}

// this would be called from the worker thread
void WaitGroup::Done() {
  {
    std::unique_lock<std::mutex> l(mu_);
    counter_--;
  }
  cv_.notify_all();
}

// this would be called from the main thread
void WaitGroup::Wait() {
  std::unique_lock<std::mutex> l(mu_);
  cv_.wait(l, [&] { return counter_ <= 0; });
}

[dragonquest]

@qiandu2006 @yoda-jm thank you very much for your issue. I will update it once I have again a c++ development environment setup. Feel free to create a PR. I appreciate your review, thank you!

I leave it open until I have time to fix it or someone has sent a PR.

[varkey98]

Would something like this fix the race?

    void Done() {
        // If we hit zero, we must notify while holding the mutex.
        if (counter_.fetch_sub(1, std::memory_order_acq_rel) == 1) {
            std::lock_guard<std::mutex> lk(m_);
            // Under the lock, counter is now 0.
            cv_.notify_all();
        }
    }

The variable is still atomic, but notification is only sent out when lock is acquired, so that its guaranteed that the condition variable is in it's wait state