From b4e8cc8b793961eb26d9ea771c723d0ce80c5db4 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 4 Jan 2025 18:22:38 -0600 Subject: [PATCH] Use explicit Timeout enum instead of magic constants The FdReadableSet api was always intended to be converted to use Duration instead of usec/msec once the ffi was removed. This lets us be explicit about forever/infinite timeouts and removes the (small) chance of a collision between u64::MAX and INFINITE. I tried this out with `type Timeout = Option` (only without the alias) but was unhappy with easy it is to accidentally use `None` when you meant a timeout of zero. --- src/fd_monitor.rs | 12 ++++-- src/fd_readable_set.rs | 96 ++++++++++++++++++++--------------------- src/input_common.rs | 6 +-- src/tests/fd_monitor.rs | 6 +-- src/threads.rs | 4 +- src/topic_monitor.rs | 4 +- 6 files changed, 67 insertions(+), 61 deletions(-) diff --git a/src/fd_monitor.rs b/src/fd_monitor.rs index ab98b8b80..9bf5a1ca5 100644 --- a/src/fd_monitor.rs +++ b/src/fd_monitor.rs @@ -9,7 +9,7 @@ use std::sync::{Arc, Mutex}; use std::time::Duration; use crate::common::exit_without_destructors; -use crate::fd_readable_set::FdReadableSet; +use crate::fd_readable_set::{FdReadableSet, Timeout}; use crate::fds::AutoCloseFd; use crate::flog::FLOG; use crate::threads::assert_is_background_thread; @@ -136,7 +136,11 @@ impl FdEventSignaller { /// but guarantees that the next call to wait() will not block. /// Return true if readable, false if not readable, or not interrupted by a signal. pub fn poll(&self, wait: bool /* = false */) -> bool { - let timeout = if wait { FdReadableSet::kNoTimeout } else { 0 }; + let timeout = if wait { + Timeout::Forever + } else { + Timeout::ZERO + }; FdReadableSet::is_fd_readable(self.read_fd(), timeout) } @@ -383,8 +387,8 @@ impl BackgroundFdMonitor { drop(data); let ret = fds.check_readable( timeout - .map(|duration| duration.as_micros() as u64) - .unwrap_or(FdReadableSet::kNoTimeout), + .map(|d| Timeout::Duration(d)) + .unwrap_or(Timeout::Forever), ); if ret < 0 && !matches!(errno().0, libc::EINTR | libc::EBADF) { // Surprising error diff --git a/src/fd_readable_set.rs b/src/fd_readable_set.rs index 3dc61d03c..4ef13c42c 100644 --- a/src/fd_readable_set.rs +++ b/src/fd_readable_set.rs @@ -1,10 +1,36 @@ use libc::c_int; use std::os::unix::prelude::*; +use std::time::Duration; + +pub enum Timeout { + Duration(Duration), + Forever, +} + +impl Timeout { + pub const ZERO: Timeout = Timeout::Duration(Duration::ZERO); + + /// Convert from usecs to poll-friendly msecs. + #[allow(unused)] + fn as_poll_msecs(&self) -> c_int { + match self { + // Negative values mean wait forever in poll-speak. + Timeout::Forever => -1 as c_int, + Timeout::Duration(duration) => { + assert!( + duration.as_millis() < c_int::MAX as _, + "Timeout too long but not forever!" + ); + duration.as_millis() as c_int + } + } + } +} /// Returns `true` if the fd is or becomes readable within the given timeout. /// This returns `false` if the waiting is interrupted by a signal. -pub fn is_fd_readable(fd: i32, timeout_usec: u64) -> bool { - FdReadableSet::is_fd_readable(fd, timeout_usec) +pub fn is_fd_readable(fd: i32, timeout: Timeout) -> bool { + FdReadableSet::is_fd_readable(fd, timeout) } /// Returns whether an fd is readable. @@ -23,11 +49,6 @@ pub struct FdReadableSet { nfds_: c_int, } -#[allow(dead_code)] -const kUsecPerMsec: u64 = 1000; -#[allow(dead_code)] -const kUsecPerSec: u64 = 1000 * kUsecPerMsec; - #[cfg(target_os = "macos")] impl FdReadableSet { /// Construct an empty set. @@ -66,17 +87,18 @@ impl FdReadableSet { /// Call `select()`. Note this destructively modifies the set. Returns the result of /// `select()`. - pub fn check_readable(&mut self, timeout_usec: u64) -> c_int { + pub fn check_readable(&mut self, timeout: Timeout) -> c_int { let null = std::ptr::null_mut(); let mut tvs; - let timeout = if timeout_usec == Self::kNoTimeout { - std::ptr::null_mut() - } else { - tvs = libc::timeval { - tv_sec: (timeout_usec / kUsecPerSec) as libc::time_t, - tv_usec: (timeout_usec % kUsecPerSec) as libc::suseconds_t, - }; - &mut tvs + let timeout = match timeout { + Timeout::Forever => std::ptr::null_mut(), + Timeout::Duration(duration) => { + tvs = libc::timeval { + tv_sec: duration.as_secs() as libc::time_t, + tv_usec: duration.subsec_micros() as libc::suseconds_t, + }; + &mut tvs + } }; unsafe { return libc::select(self.nfds_, &mut self.fdset_, null, null, timeout); @@ -85,24 +107,21 @@ impl FdReadableSet { /// Check if a single fd is readable, with a given timeout. /// Returns `true` if readable, `false` otherwise. - pub fn is_fd_readable(fd: RawFd, timeout_usec: u64) -> bool { + pub fn is_fd_readable(fd: RawFd, timeout: Timeout) -> bool { if fd < 0 { return false; } let mut s = Self::new(); s.add(fd); - let res = s.check_readable(timeout_usec); + let res = s.check_readable(timeout); return res > 0 && s.test(fd); } /// Check if a single fd is readable, without blocking. /// Returns `true` if readable, `false` if not. pub fn poll_fd_readable(fd: RawFd) -> bool { - return Self::is_fd_readable(fd, 0); + return Self::is_fd_readable(fd, Timeout::ZERO); } - - /// A special timeout value which may be passed to indicate no timeout. - pub const kNoTimeout: u64 = u64::MAX; } #[cfg(not(target_os = "macos"))] @@ -162,45 +181,29 @@ impl FdReadableSet { return false; } - /// Convert from usecs to poll-friendly msecs. - fn usec_to_poll_msec(timeout_usec: u64) -> c_int { - let mut timeout_msec: u64 = timeout_usec / kUsecPerMsec; - // Round to nearest, down for halfway. - if (timeout_usec % kUsecPerMsec) > kUsecPerMsec / 2 { - timeout_msec += 1; - } - if timeout_usec == FdReadableSet::kNoTimeout || timeout_msec > c_int::MAX as u64 { - // Negative values mean wait forever in poll-speak. - return -1; - } - return timeout_msec as c_int; - } - - fn do_poll(fds: &mut [libc::pollfd], timeout_usec: u64) -> c_int { + fn do_poll(fds: &mut [libc::pollfd], timeout: Timeout) -> c_int { let count = fds.len(); assert!(count <= libc::nfds_t::MAX as usize, "count too big"); return unsafe { libc::poll( fds.as_mut_ptr(), count as libc::nfds_t, - Self::usec_to_poll_msec(timeout_usec), + timeout.as_poll_msecs(), ) }; } /// Call poll(). Note this destructively modifies the set. Return the result of poll(). - /// - /// TODO: Change to [`Duration`](std::time::Duration) once FFI usage is done. - pub fn check_readable(&mut self, timeout_usec: u64) -> c_int { + pub fn check_readable(&mut self, timeout: Timeout) -> c_int { if self.pollfds_.is_empty() { return 0; } - return Self::do_poll(&mut self.pollfds_, timeout_usec); + return Self::do_poll(&mut self.pollfds_, timeout); } /// Check if a single fd is readable, with a given timeout. /// Return true if `fd` is our set and is readable, `false` otherwise. - pub fn is_fd_readable(fd: RawFd, timeout_usec: u64) -> bool { + pub fn is_fd_readable(fd: RawFd, timeout: Timeout) -> bool { if fd < 0 { return false; } @@ -209,16 +212,13 @@ impl FdReadableSet { events: libc::POLLIN, revents: 0, }; - let ret = Self::do_poll(std::slice::from_mut(&mut pfd), timeout_usec); + let ret = Self::do_poll(std::slice::from_mut(&mut pfd), timeout); return ret > 0 && (pfd.revents & libc::POLLIN) != 0; } /// Check if a single fd is readable, without blocking. /// Return true if readable, false if not. pub fn poll_fd_readable(fd: RawFd) -> bool { - return Self::is_fd_readable(fd, 0); + return Self::is_fd_readable(fd, Timeout::ZERO); } - - /// A special timeout value which may be passed to indicate no timeout. - pub const kNoTimeout: u64 = u64::MAX; } diff --git a/src/input_common.rs b/src/input_common.rs index c29ee6be8..0f0f471e5 100644 --- a/src/input_common.rs +++ b/src/input_common.rs @@ -5,7 +5,7 @@ use crate::common::{ str2wcstring, write_loop, WSL, }; use crate::env::{EnvStack, Environment}; -use crate::fd_readable_set::FdReadableSet; +use crate::fd_readable_set::{FdReadableSet, Timeout}; use crate::flog::{FloggableDebug, FLOG}; use crate::fork_exec::flog_safe::FLOG_SAFE; use crate::global_safety::RelaxedAtomicBool; @@ -334,9 +334,9 @@ fn readb(in_fd: RawFd, blocking: bool) -> ReadbResult { // Here's where we call select(). let select_res = fdset.check_readable(if blocking { - FdReadableSet::kNoTimeout + Timeout::Forever } else { - 0 + Timeout::ZERO }); if select_res < 0 { let err = errno::errno().0; diff --git a/src/tests/fd_monitor.rs b/src/tests/fd_monitor.rs index 1be5972ad..ca3424e3b 100644 --- a/src/tests/fd_monitor.rs +++ b/src/tests/fd_monitor.rs @@ -13,7 +13,7 @@ use std::time::Duration; use errno::errno; use crate::fd_monitor::{FdEventSignaller, FdMonitor}; -use crate::fd_readable_set::FdReadableSet; +use crate::fd_readable_set::{FdReadableSet, Timeout}; use crate::fds::{make_autoclose_pipes, AutoCloseFd, AutoClosePipes}; use crate::tests::prelude::*; @@ -182,8 +182,8 @@ where // Timeout after 500 msec. // macOS will eagerly return EBADF if the fd is closed; Linux will hit the timeout. - let timeout_usec = 500 * 1_000; - let ret = fd_set.check_readable(timeout_usec); + let timeout = Timeout::Duration(Duration::from_millis(500)); + let ret = fd_set.check_readable(timeout); if ret < 0 { Err(errno().0) } else { diff --git a/src/threads.rs b/src/threads.rs index 9a4253f35..0596b4465 100644 --- a/src/threads.rs +++ b/src/threads.rs @@ -496,7 +496,9 @@ pub fn iothread_port() -> i32 { } pub fn iothread_service_main_with_timeout(ctx: &mut Reader, timeout: Duration) { - if crate::fd_readable_set::is_fd_readable(iothread_port(), timeout.as_millis() as u64) { + use crate::fd_readable_set::Timeout; + + if crate::fd_readable_set::is_fd_readable(iothread_port(), Timeout::Duration(timeout)) { iothread_service_main(ctx); } } diff --git a/src/topic_monitor.rs b/src/topic_monitor.rs index 1b9d286c0..1b49f91f0 100644 --- a/src/topic_monitor.rs +++ b/src/topic_monitor.rs @@ -20,7 +20,7 @@ also provides the ability to perform a blocking wait for any topic to change in set. This is the real power of topics: you can wait for a sigchld signal OR a thread exit. */ -use crate::fd_readable_set::FdReadableSet; +use crate::fd_readable_set::{FdReadableSet, Timeout}; use crate::fds::{self, make_fd_nonblocking, AutoClosePipes}; use crate::flog::{FloggableDebug, FLOG}; use crate::wchar::WString; @@ -240,7 +240,7 @@ impl BinarySemaphore { // call until data is available (that is, fish would use 100% cpu while waiting for // processes). This call prevents that. if cfg!(feature = "tsan") { - let _ = FdReadableSet::is_fd_readable(fd, FdReadableSet::kNoTimeout); + let _ = FdReadableSet::is_fd_readable(fd, Timeout::Forever); } let mut ignored: u8 = 0; match unistd::read(fd, std::slice::from_mut(&mut ignored)) {