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

Fixing a race with timed suspension (second attempt) #3158

Merged
merged 1 commit into from Feb 11, 2018

Conversation

Projects
None yet
2 participants
@sithhell
Copy link
Member

commented Feb 8, 2018

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.

@sithhell sithhell added this to the 1.1.0 milestone Feb 8, 2018

thread_priority priority = thread_priority_normal,
error_code& ec = throws)
{
std::atomic<bool> timer_started(false);

This comment has been minimized.

Copy link
@hkaiser

hkaiser Feb 9, 2018

Member

Won't this atomic go out of scope before the timer thread can access it?

This comment has been minimized.

Copy link
@sithhell

sithhell Feb 9, 2018

Author Member

Good catch!

@sithhell sithhell force-pushed the fix_timed_suspension branch 2 times, most recently from c0e5371 to e3ec5b7 Feb 9, 2018

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2018

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.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 9, 2018

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?

Fixing a race with timed suspension
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.

@sithhell sithhell force-pushed the fix_timed_suspension branch from e3ec5b7 to 692c675 Feb 9, 2018

@sithhell

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2018

Right. missed the missing atomic header. The other inspect error is already fixed on master.

@hkaiser
Copy link
Member

left a comment

LGTM now - thanks!

@sithhell sithhell merged commit 98effdc into master Feb 11, 2018

8 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
pycicle daint-clang-6.0.0 Build errors 0
Details
pycicle daint-clang-6.0.0 Config errors 0
Details
pycicle daint-clang-6.0.0 Test errors 0
Details
pycicle daint-gcc-6.2.0 Build errors 0
Details
pycicle daint-gcc-6.2.0 Config errors 0
Details
pycicle daint-gcc-6.2.0 Test errors 0
Details

@sithhell sithhell deleted the fix_timed_suspension branch Feb 11, 2018

@msimberg msimberg referenced this pull request Feb 12, 2018

Closed

Fix failing tests #3044

28 of 28 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.