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

Fix a race in thread wakeup #459

Merged
merged 1 commit into from Jul 3, 2018

Conversation

@stjepang
Copy link
Contributor

commented Jun 29, 2018

When unparking a worker thread, we do the following steps:

  1. Try transitioning IDLE -> NOTIFY.
  2. Lock the mutex.
  3. Try transitioning SLEEP -> NOTIFY and wakeup on success.

The problem occurs if the thread transitions from SLEEP to IDLE between steps 1 and 2. In that case, step 3 will fail to transition and simply return from unpark.

This PR changes the step 3 so that we always transition to NOTIFY, no matter what the previous state was.

The park/unpark mechanism in std::thread works somewhat similarly. In step 3, it attempts to CAS from SLEEP to NOTIFY, but jumps back to step 1 if the previous state was IDLE. See here. I've decided to unconditionally move to state NOTIFY instead - the end result should be the same.

In addition to that, I did a bunch of random improvements to the documentation.

@@ -41,7 +41,6 @@ use futures2;
/// use std::time::Duration;
///
/// # pub fn main() {
/// // Create a thread pool with default configuration values

This comment has been minimized.

Copy link
@stjepang

stjepang Jun 29, 2018

Author Contributor

I found this comment confusing. It seems to imply that the example shows how the builder is configured by default. However, this comment applies only to Builder::new(), while the chained methods do custom configuration (which is different from default).

Perhaps a different sentence would work better, but due to lack of ideas I've decided to simply remove it. :)

This comment has been minimized.

Copy link
@carllerche
@carllerche
Copy link
Member

left a comment

Good eye! That is definitely an issue.

@@ -41,7 +41,6 @@ use futures2;
/// use std::time::Duration;
///
/// # pub fn main() {
/// // Create a thread pool with default configuration values

This comment has been minimized.

Copy link
@carllerche

@carllerche carllerche merged commit 7fb579c into tokio-rs:master Jul 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stjepang stjepang deleted the stjepang:fix-wakeup-race branch Jul 3, 2018

This was referenced Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.