Skip to content

Bump subcrate versions #524

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

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Bump subcrate versions #524

merged 1 commit into from
Aug 7, 2018

Conversation

carllerche
Copy link
Member

  • tokio-current-thread 0.1.1
  • tokio-executor 0.1.3
  • tokio-fs 0.1.3
  • tokio-reactor 0.1.3
  • tokio-tcp 0.1.1
  • tokio-timer 0.2.5

This may want to wait until rust-lang/futures-rs#1170 is inspected as it might relate to #459.

* tokio-current-thread 0.1.1
* tokio-executor 0.1.3
* tokio-fs 0.1.3
* tokio-reactor 0.1.3
* tokio-tcp 0.1.1
* tokio-timer 0.2.5
@ghost
Copy link

ghost commented Aug 6, 2018

I just double checked that the changes in #459 are correct. It's a bit suspicious because it fixes a bug in the wakeup mechanism, but after it got merged we got deadlocks (ironically). However, it looks correct to me.

If someone wants to take a second look, I encourage you to do so - the diff is really small. You can also compare the code to these functions in libstd:

The equivalent functions in tokio have almost identical implementations, with one slight difference:

  • In unpark, we finish with a swap operation to set the state to NOTIFY. In thread::unpark there's a CAS operation in a loop that attempts to change the state from PARKED to NOTIFIED. But if that fails, the next CAS will attempt to go from EMPTY to NOTIFIED anyway, so it doesn't really matter.

@sfackler
Copy link
Contributor

sfackler commented Aug 6, 2018

Assuming that #459 is correct, what was the specific broken behavior it was fixing? Could that bug have been masking a separate bug elsewhere?

@carllerche
Copy link
Member Author

Is there a reliable repro?

@sfackler
Copy link
Contributor

sfackler commented Aug 6, 2018

I've only seen it in production unfortunately - I spent a while trying to make a small reproduction and couldn't seem to make it happen.

@jsgf
Copy link

jsgf commented Aug 6, 2018

We've also been seeing apparent lost wakeup bugs which may well correlate to the 0.1.4 -> 0.1.5 transition (we've seen quite a few in the last week, but none previously).

I checked over the logic and it does look correct to me, but these things can be subtle. I'll look more closely.

In the meantime I'll try rolling back to 0.1.4.

@sfackler
Copy link
Contributor

sfackler commented Aug 6, 2018

Looking into #459, it does seem like the correct change and in any case it strictly increases the number of times you notify. Maybe we just got lucky after the downgrade?

@carllerche
Copy link
Member Author

#459 is the only change between 0.1.4 and 0.1.5.

@carllerche
Copy link
Member Author

I opened #525 to discuss the lost wakeup.

@carllerche
Copy link
Member Author

I'm going to merge this. Discussion can continue in the new issue.

@carllerche carllerche merged commit e964c41 into master Aug 7, 2018
@carllerche carllerche deleted the release-crates2 branch August 9, 2018 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants