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
Fixing a race with timed suspension (second attempt) #3158
Conversation
thread_priority priority = thread_priority_normal, | ||
error_code& ec = throws) | ||
{ | ||
std::atomic<bool> timer_started(false); |
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.
Won't this atomic go out of scope before the timer thread can access it?
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.
Good catch!
c0e5371
to
e3ec5b7
Compare
The inspect failure is not related to this PR. Pycicle tests run through and I verified that everything works locally. This PR is now potentially ready to be merged. |
At least one of the inspect failures seems to be related to your changes (using atomic). Would you mind fixing the other one while you're at it as well? |
When doing a timed suspension on a thread, there were several race conditions between setting the different participating thread states, which should now be fixed. The problem was observable with the wait_for_1751 regression test.
e3ec5b7
to
692c675
Compare
Right. missed the missing atomic header. The other inspect error is already fixed on master. |
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.
LGTM now - thanks!
When doing a timed suspension on a thread, there were several race conditions
between setting the different participating thread states, which should now be
fixed. The problem was observable with the wait_for_1751 regression test.
This is the second attempt (after #3153), fixing the previous issues.