signal: use eventfd instead of UnixStream for signal notification#7845
signal: use eventfd instead of UnixStream for signal notification#7845winter-loo wants to merge 12 commits intotokio-rs:masterfrom
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.
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.
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.
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.
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.
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.
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.
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.
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.
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: