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

Fixed possible memory order bug #84

Conversation

Evgeny-S-Sorokin
Copy link

Describe the changes

I believe, there was a memory order bug, concerning usage of atomic flags before condition_variable.notify.
According to https://en.cppreference.com/w/cpp/thread/condition_variable

Even if the shared variable is atomic, it must be modified under the mutex in order to correctly publish the modification to the waiting thread.

So I fenced both flags and notify statement with mutex.

Testing

Have you tested the new code using the provided automated test program and/or performed any other tests to ensure that it works correctly? If so, please provide information about the test system(s):

Yes, I did.

  • CPU model, architecture, # of cores and threads: Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz 2.71 GHz, 2 cores, 4 threads
  • Operating system: Windows 10 Pro, x64
  • Name and version of C++ compiler: Visual Studio 19 compiler
  • Full command used for compiling, including all compiler flags: I don't think it matters.

Additional information

When I used Your project as a template for my personal project, I had a bug where threads would be notified before end flag set to true causing deadlock. It was captured using high-end system and I could not replicate it on my personal computer.

@bshoshany
Copy link
Owner

Thanks for the PR, @Evgeny-S-Sorokin!. This seems like a necessary modification, based on the quote from cppreference. However, I prefer to use the C++17 std::scoped_lock instead of std::lock_guard. I will incorporate this into the next release (which should be published in a few weeks).

@bshoshany bshoshany closed this Oct 22, 2022
@bshoshany
Copy link
Owner

Update: This issue has been fixed in v3.5.0, which was just released. (I did it in a slightly different way than what you suggested.) Thanks again!

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

Successfully merging this pull request may close these issues.

2 participants