-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
signal: use eventfd instead of UnixStream for signal notification #7845
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
base: master
Are you sure you want to change the base?
Conversation
fallback to UnixStream on non-Linux platform
tokio/src/signal/unix.rs
Outdated
| if fd < 0 { | ||
| panic!("eventfd failed: {}", io::Error::last_os_error()); | ||
| } |
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.
Instead of panicking can we return an error here?
tokio/src/signal/unix.rs
Outdated
| std::os::unix::net::UnixStream::from_raw_fd(receiver_fd) | ||
| }); | ||
| let inner = | ||
| UnixStream::from_std(original.try_clone().expect("failed to clone UnixStream")); |
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.
Same comment, this is a fallible operation, we should return an error here, not panic
|
Thanks for review. It's time to make it better. |
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.
Comparing with what mio does, this seems to not have a bunch of logic to reset the eventfd and so on... Is it missing here?
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.
A read on fd returned by eventfd resets eventfd. Here's libc::eventfd_read in Receiver::read() function.
| OsStorage: 'static + Send + Sync + Default, | ||
| { | ||
| static GLOBALS: OnceLock<Globals> = OnceLock::new(); | ||
| static GLOBALS: OnceLock<std::io::Result<Globals>> = OnceLock::new(); |
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.
Drive by review: I think you should store Result<Globals, i32> like I did for the per-signal result storage. You can then convert the i32 to an error via Error::from_raw_os_error and avoid allocating.
| } | ||
| } | ||
|
|
||
| impl Sender { |
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.
This could be merged with the impl above.
| if r == 0 { | ||
| Ok(0) | ||
| } else { | ||
| Err(std::io::Error::last_os_error()) |
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.
Since it is non-blocking it may return EAGAIN (ErrorKind::WouldBlock) which is not an error.
let err = std::io::Error::last_os_error();
// On a non-blocking eventfd, it is expected to get an EAGAIN
// when the counter is 0.
if err.kind() == std::io::ErrorKind::WouldBlock {
Ok(0)
} else {
Err(err)
}| } | ||
| } | ||
|
|
||
| impl OsExtraData { |
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.
This impl could be merged with the one above
| // SAFETY: it's ok to call libc API | ||
| let r = unsafe { libc::eventfd_write(self.fd.as_raw_fd(), 1) }; | ||
| if r == 0 { | ||
| Ok(0) |
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.
There is a small different between the eventfd and unixstream impls here.
The eventfd's Sender::write() returns 0 on success. The UnixStream returns the number of written bytes (1).
AFAIS the successful result is not used.
Maybe change eventfd to return 1 too or change both to return Result<()> ?!
Motivation
According to the Linux eventfd doc:
So, I made an attempt to replace
mio::UnixStreamwithlibc::eventfd.Solution
The benchmark(
RUSTFLAGS="" cargo bench -p benches --bench signal) shows some improvements: