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

Redox: Fix Condvar.wait(); do not lock mutex twice #43082

Merged
merged 1 commit into from Jul 9, 2017

Conversation

Projects
None yet
6 participants
@ids1024
Copy link
Contributor

ids1024 commented Jul 6, 2017

The atomic_xchg() loop locks the mutex, so the call to mutex_lock is
incorrect, and blocks.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 6, 2017

@bors: r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 6, 2017

📌 Commit 86191f4 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 6, 2017

🔒 Merge conflict

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Jul 6, 2017

Is there a test for this stuff? I'm assuming the previous behavior would be an immediate deadlock as soon as the convar wakes up?

@ids1024

This comment has been minimized.

Copy link
Contributor Author

ids1024 commented Jul 6, 2017

I'm assuming the previous behavior would be an immediate deadlock as soon as the convar wakes up?

Yes.

The implementation now matches https://locklessinc.com/articles/mutex_cv_futex/, which @jackpot51 says he used as a reference to implement it; it seems this line was added by accident.

I don't see a test for Condvar in the rust repository; if anyone has a test to recommend, feel free.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Jul 7, 2017

Heads up, @ids1024; you've got a merge conflict.

@ids1024

This comment has been minimized.

Copy link
Contributor Author

ids1024 commented Jul 7, 2017

But what merge conflict? It seems to merge cleanly with master, and the only file changed here is a Redox-specific file that hasn't been changed in months.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 7, 2017

@ids1024 you'll want to just rebase against rust-lang/rust's master branch

Redox: Fix Condvar.wait(); do not lock mutex twice
The atomic_xchg() loop locks the mutex, so the call to mutex_lock is
incorrect, and blocks.

@ids1024 ids1024 force-pushed the ids1024:condvar2 branch from 86191f4 to 59981e4 Jul 7, 2017

@jackpot51

This comment has been minimized.

Copy link
Contributor

jackpot51 commented Jul 8, 2017

@alexcrichton it has been rebased.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 9, 2017

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 9, 2017

📌 Commit 59981e4 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 9, 2017

⌛️ Testing commit 59981e4 with merge 12fef71...

bors added a commit that referenced this pull request Jul 9, 2017

Auto merge of #43082 - ids1024:condvar2, r=alexcrichton
Redox: Fix Condvar.wait(); do not lock mutex twice

The atomic_xchg() loop locks the mutex, so the call to mutex_lock is
incorrect, and blocks.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 12fef71 to master...

@bors bors merged commit 59981e4 into rust-lang:master Jul 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@ids1024 ids1024 deleted the ids1024:condvar2 branch Oct 5, 2017

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.