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<Duration>` (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

@@ -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

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;
 }

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;

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 {

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);
     }
 }

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)) {