Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

race condition in WaitGroup class #1

Open
yoda-jm opened this issue Feb 13, 2020 · 2 comments
Open

race condition in WaitGroup class #1

yoda-jm opened this issue Feb 13, 2020 · 2 comments

Comments

@yoda-jm
Copy link

yoda-jm commented Feb 13, 2020

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
Copy link

qiandu2006 commented Jul 6, 2020

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
Copy link
Owner

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants