-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
io: update to Mio 0.7 #2893
io: update to Mio 0.7 #2893
Conversation
This also makes Mio an implementation detail, removing it from the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo changing Cargo.toml
to depend on `mio = 0.7.2" looks good to me!
I'll let Sean comment on the io-driver changes
tokio/src/signal/unix/driver.rs
Outdated
// Mio 0.7 removed `try_clone()` as an API due to unexpected behavior | ||
// with registering dups with the same reactor. In this case, duping is | ||
// safe as each dup is registered with separate reactors **and** we | ||
// only at least one dup to receive the notification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only expect at least one dup to receive the notification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All signal registrations are held in a global data structure. As long as some running SignalDriver
gets woken, it will dispatch all pending signal notifications. A potential issue could be if we only wake one driver which isn't turning for whatever reason, but I believe this is unlikely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe he was mostly commenting on the missing word :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. This threw me off a bit when reading this comment. It makes sense but took a bit of thinking to reconstruct the intent :)
This also makes Mio an implementation detail, removing it from the
public API.
This is based on #1767.
Remaining