-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fix the barrier logic for threads synchronization #811
Conversation
455ad6b
to
2758fcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! I've added some comments to improve the PR but overall it is fine! Thanks for spending time and fixing this 🎉
I have closed and re-opened the PR to trigger again the CI, because Windows jobs were failing (probably due to a connection error, since the download of the wget package was failing). But it seems like these Windows jobs are failing again for the same reason. @GiulioRomualdi could you try to trigger again just the Windows jobs? I think I do not have the rights to trigger specific jobs. Thanks! |
For reference, the error is:
Not sure if it is a network error. Perhaps we could use curl instead of wget to download the .zip on Windows? |
Related upstream issue: actions/runner-images#9367 (comment) . There still seems to be problem: https://status.chocolatey.org/ . Probably we can just ignore the Windows-vcpkg job for now, as long as Windows conda is compiling fine, I do not think we can risk regressions. |
I restarted the CI as Chocolatey is now up: https://status.chocolatey.org/ . |
Thank you @LoreMoretti |
As explained in this comment, we found out that the barrier logic for threads synchronization is not working properly.
This is due to the following two reasons:
if
condition reads ((--m_count) == 1). Therefore no thread will wait.lambda function
passed tostd::thread()
. This means that, even if the wait command is reached, it will stop the main thread (i.e. the one which is spawning the other threads).This PR aims at addressing these problems.