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.

Sign up for GitHub

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

Jump to bottom

condition_variable_any with stop_token: Unlock inner mutex before relocking outer mutex #2220

Merged
merged 6 commits into from
Oct 20, 2021
Merged

condition_variable_any with stop_token: Unlock inner mutex before relocking outer mutex #2220

merged 6 commits into from
Oct 20, 2021

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Sep 27, 2021 •
edited
Loading

Fix #2218

I think we can't move _Unlock_outer after checking for time expiration, as the Standard wants us to unlock outer mutex as the first step, and this is visible for condition_variable_any. If we could, this would have been certainly a better fix.

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner September 27, 2021 14:29
@AlexGuteniev AlexGuteniev mentioned this pull request Sep 27, 2021
Closed
@AlexGuteniev AlexGuteniev changed the title Unlock inner mutex before re-locking outer mutex condition_variable_any with stop_token: Unlock inner mutex before re-locking outer mutex Sep 27, 2021
CaseyCarter
CaseyCarter approved these changes Sep 27, 2021
View reviewed changes
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I audited the other wait functions looking for similar occurrences, but found no others.

@CaseyCarter
Copy link
Member

I think we can't move _Unlock_outer after checking for time expiration, as the Standard wants us to unlock outer mutex as the first step, and this is visible for condition_variable_any. If we could, this would have been certainly a better fix.

Agreed we must actually unlock the lock when the timeout has expired (See #369).

@CaseyCarter CaseyCarter added the bug Something isn't working label Sep 27, 2021
CaseyCarter
CaseyCarter requested changes Sep 28, 2021
View reviewed changes
stl/inc/condition_variable Outdated Show resolved Hide resolved
@AlexGuteniev AlexGuteniev changed the title condition_variable_any with stop_token: Unlock inner mutex before re-locking outer mutex condition_variable_any with stop_token: Unlock inner mutex before relocking outer mutex Sep 28, 2021
@CaseyCarter CaseyCarter removed their assignment Sep 30, 2021
@StephanTLavavej StephanTLavavej removed their assignment Oct 14, 2021
@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR; please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 066c172 into microsoft:main Oct 20, 2021
@StephanTLavavej
Copy link
Member

Thanks for robustly fixing this deadlock! 🧟 ☠️ 🎃

@AlexGuteniev AlexGuteniev deleted the deadlock_stop_token_expired branch October 20, 2021 10:29
AreaZR pushed a commit to AreaZR/STL that referenced this pull request Nov 4, 2021
@AlexGuteniev @StephanTLavavej @AreaZR
Fix deadlock in condition_variable_any with stop_token (microsoft…
111f052 
…#2220)

Co-authored-by: Stephan T. Lavavej <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
4 participants
@AlexGuteniev @CaseyCarter @StephanTLavavej @BillyONeal