Clamp long timeouts (like Duration::MAX)

CHANGELOG.md

@@ -23,6 +23,10 @@ project adheres to [Semantic Versioning](https://semver.org/).
 ### Fixed
 * Fix looking up `UsbPortInfo::interface` on macOS.
   [#193](https://github.com/serialport/serialport-rs/pull/193)
+* Fix issues with very long timeout values like `Duration::MAX` by clamping to
+  maximum supported value for underlying platform.
+  [#207](https://github.com/serialport/serialport-rs/issues/207),
+  [#208](https://github.com/serialport/serialport-rs/pull/208)
 ### Removed
 
 

Cargo.toml

@@ -16,7 +16,6 @@ categories = ["hardware-support"]
 
 [target."cfg(unix)".dependencies]
 bitflags = "2.4.0"
-cfg-if = "1.0.0"
 nix = { version = "0.26", default-features = false, features = ["fs", "ioctl", "poll", "signal", "term"] }
 
 [target.'cfg(all(target_os = "linux", not(target_env = "musl")))'.dependencies]
@@ -36,6 +35,7 @@ features = [
 ]
 
 [dependencies]
+cfg-if = "1.0.0"
 scopeguard = "1.1"
 serde = { version = "1.0", features = ["derive"], optional = true }
 

src/lib.rs

@@ -47,6 +47,9 @@ mod windows;
 #[cfg(windows)]
 pub use windows::COMPort;
 
+#[cfg(test)]
+pub(crate) mod tests;
+
 /// A type for results generated by interacting with serial ports
 ///
 /// The `Err` type is hard-wired to [`serialport::Error`](struct.Error.html).
@@ -375,6 +378,14 @@ impl SerialPortBuilder {
     }
 
     /// Set the amount of time to wait to receive data before timing out
+    ///
+    /// <div class="warning">
+    ///
+    /// The accuracy is limited by the underlying platform's capabilities. Longer timeouts will be
+    /// clamped to the maximum supported value which is expected to be in the magnitude of a few
+    /// days.
+    ///
+    /// </div>
     #[must_use]
     pub fn timeout(mut self, timeout: Duration) -> Self {
         self.timeout = timeout;
@@ -487,6 +498,14 @@ pub trait SerialPort: Send + io::Read + io::Write {
     fn set_stop_bits(&mut self, stop_bits: StopBits) -> Result<()>;
 
     /// Sets the timeout for future I/O operations.
+    ///
+    /// <div class="warning">
+    ///
+    /// The accuracy is limited by the underlying platform's capabilities. Longer timeouts will be
+    /// clamped to the maximum supported value which is expected to be in the magnitude of a few
+    /// days.
+    ///
+    /// </div>
     fn set_timeout(&mut self, timeout: Duration) -> Result<()>;
 
     // Functions for setting non-data control signal pins

src/posix/enumerate.rs

@@ -696,7 +696,7 @@ cfg_if! {
     not(target_env = "musl"),
     feature = "libudev"
 ))]
-mod test {
+mod tests {
     use super::*;
 
     use quickcheck_macros::quickcheck;

src/posix/poll.rs

@@ -5,11 +5,12 @@ use std::os::unix::io::RawFd;
 use std::slice;
 use std::time::Duration;
 
+use nix::libc::c_int;
 use nix::poll::{PollFd, PollFlags};
 #[cfg(target_os = "linux")]
 use nix::sys::signal::SigSet;
-#[cfg(target_os = "linux")]
-use nix::sys::time::{TimeSpec, TimeValLike};
+#[cfg(any(target_os = "linux", test))]
+use nix::sys::time::TimeSpec;
 
 pub fn wait_read_fd(fd: RawFd, timeout: Duration) -> io::Result<()> {
     wait_fd(fd, PollFlags::POLLIN, timeout)
@@ -24,23 +25,12 @@ fn wait_fd(fd: RawFd, events: PollFlags, timeout: Duration) -> io::Result<()> {
 
     let mut fd = PollFd::new(fd, events);
 
-    let milliseconds =
-        timeout.as_secs() as i64 * 1000 + i64::from(timeout.subsec_nanos()) / 1_000_000;
-    #[cfg(target_os = "linux")]
-    let wait_res = {
-        let timespec = TimeSpec::milliseconds(milliseconds);
-        nix::poll::ppoll(
-            slice::from_mut(&mut fd),
-            Some(timespec),
-            Some(SigSet::empty()),
-        )
-    };
-    #[cfg(not(target_os = "linux"))]
-    let wait_res = nix::poll::poll(slice::from_mut(&mut fd), milliseconds as nix::libc::c_int);
-
-    let wait = match wait_res {
+    let wait = match poll_clamped(&mut fd, timeout) {
         Ok(r) => r,
-        Err(e) => return Err(io::Error::from(crate::Error::from(e))),
+        Err(e) => {
+            dbg!(e);
+            return Err(io::Error::from(crate::Error::from(e)));
+        }
     };
     // All errors generated by poll or ppoll are already caught by the nix wrapper around libc, so
     // here we only need to check if there's at least 1 event
@@ -63,3 +53,102 @@ fn wait_fd(fd: RawFd, events: PollFlags, timeout: Duration) -> io::Result<()> {
 
     Err(io::Error::new(io::ErrorKind::Other, EIO.desc()))
 }
+
+/// Poll with a duration clamped to the maximum value representable by the `TimeSpec` used by
+/// `ppoll`.
+#[cfg(target_os = "linux")]
+fn poll_clamped(fd: &mut PollFd, timeout: Duration) -> nix::Result<c_int> {
+    let spec = clamped_time_spec(timeout);
+    nix::poll::ppoll(slice::from_mut(fd), Some(spec), Some(SigSet::empty()))
+}
+
+#[cfg(any(target_os = "linux", test))]
+// The type time_t is deprecaten on musl. The nix crate internally uses this type and makes an
+// exeption for the deprecation for musl. And so do we.
+//
+// See https://github.com/rust-lang/libc/issues/1848 which is referenced from every exemption used
+// in nix.
+#[cfg_attr(target_env = "musl", allow(deprecated))]
+fn clamped_time_spec(duration: Duration) -> TimeSpec {
+    use nix::libc::c_long;
+    use nix::sys::time::time_t;
+
+    // We need to clamp manually as TimeSpec::from_duration translates durations with more than
+    // i64::MAX seconds to negative timespans. This happens due to casting to i64 and is still the
+    // case as of nix 0.29.
+    let secs_limit = time_t::MAX as u64;
+    let secs = duration.as_secs();
+    if secs <= secs_limit {
+        TimeSpec::new(secs as time_t, duration.subsec_nanos() as c_long)
+    } else {
+        TimeSpec::new(time_t::MAX, 999_999_999)
+    }
+}
+
+// Poll with a duration clamped to the maximum millisecond value representable by the `c_int` used
+// by `poll`.
+#[cfg(not(target_os = "linux"))]
+fn poll_clamped(fd: &mut PollFd, timeout: Duration) -> nix::Result<c_int> {
+    let millis = clamped_millis_c_int(timeout);
+    nix::poll::poll(slice::from_mut(fd), millis)
+}
+
+#[cfg(any(not(target_os = "linux"), test))]
+fn clamped_millis_c_int(duration: Duration) -> c_int {
+    let secs_limit = (c_int::MAX as u64) / 1000;
+    let secs = duration.as_secs();
+
+    if secs <= secs_limit {
+        secs as c_int * 1000 + duration.subsec_millis() as c_int
+    } else {
+        c_int::MAX
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::tests::timeout::MONOTONIC_DURATIONS;
+
+    #[test]
+    fn clamped_millis_c_int_is_monotonic() {
+        let mut last = clamped_millis_c_int(Duration::ZERO);
+
+        for (i, d) in MONOTONIC_DURATIONS.iter().enumerate() {
+            let next = clamped_millis_c_int(*d);
+            dbg!((i, d));
+            assert!(
+                next >= last,
+                "{next} >= {last} failed for {d:?} at index {i}"
+            );
+            last = next;
+        }
+    }
+
+    #[test]
+    fn clamped_millis_c_int_zero_is_zero() {
+        assert_eq!(0, clamped_millis_c_int(Duration::ZERO));
+    }
+
+    #[test]
+    fn clamped_time_spec_is_monotonic() {
+        let mut last = clamped_time_spec(Duration::ZERO);
+
+        for (i, d) in MONOTONIC_DURATIONS.iter().enumerate() {
+            let next = clamped_time_spec(*d);
+            dbg!((i, d));
+            assert!(
+                next >= last,
+                "{next} >= {last} failed for {d:?} at index {i}"
+            );
+            last = next;
+        }
+    }
+
+    #[test]
+    fn clamped_time_spec_zero_is_zero() {
+        let spec = clamped_time_spec(Duration::ZERO);
+        assert_eq!(0, spec.tv_sec());
+        assert_eq!(0, spec.tv_nsec());
+    }
+}

src/tests/mod.rs

@@ -0,0 +1,7 @@
+use cfg_if::cfg_if;
+
+cfg_if! {
+    if #[cfg(test)] {
+        pub(crate) mod timeout;
+    }
+}

src/tests/timeout.rs

@@ -0,0 +1,41 @@
+use std::time::Duration;
+
+/// A sequence of strongly monotonic inrceasing durations. Introduced for testing conversions from
+/// `Duration` to platform-specific types.
+pub(crate) const MONOTONIC_DURATIONS: [Duration; 17] = [
+    Duration::ZERO,
+    Duration::from_nanos(1),
+    Duration::from_millis(1),
+    Duration::from_secs(1),
+    Duration::from_secs(i16::MAX as u64 - 1),
+    Duration::from_secs(i16::MAX as u64),
+    Duration::from_secs(i16::MAX as u64 + 1),
+    Duration::from_secs(i32::MAX as u64 - 1),
+    Duration::from_secs(i32::MAX as u64),
+    Duration::from_secs(i32::MAX as u64 + 1),
+    Duration::from_secs(i64::MAX as u64 - 1),
+    Duration::from_secs(i64::MAX as u64),
+    Duration::from_secs(i64::MAX as u64 + 1),
+    Duration::from_secs(u64::MAX - 1),
+    Duration::from_secs(u64::MAX),
+    Duration::new(u64::MAX, 1_000_000),
+    Duration::MAX,
+];
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn basic_durations_properties() {
+        assert_eq!(Duration::ZERO, *MONOTONIC_DURATIONS.first().unwrap());
+        assert_eq!(Duration::MAX, *MONOTONIC_DURATIONS.last().unwrap());
+
+        // Check that this array is monotonic.
+        let mut last = MONOTONIC_DURATIONS[0];
+        for next in MONOTONIC_DURATIONS {
+            assert!(last <= next);
+            last = next;
+        }
+    }
+}

src/windows/com.rs

@@ -154,6 +154,18 @@ impl COMPort {
             port_name: None,
         }
     }
+
+    fn timeout_constant(duration: Duration) -> DWORD {
+        let milliseconds = duration.as_millis();
+        // In the way we are setting up COMMTIMEOUTS, a timeout_constant of MAXDWORD gets rejected.
+        // Let's clamp the timeout constant for values of MAXDWORD and above. See remarks at
+        // https://learn.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-commtimeouts.
+        //
+        // This effectively throws away accuracy for really long timeouts but at least preserves a
+        // long-ish timeout. But just casting to DWORD would result in presumably unexpected short
+        // and non-monotonic timeouts from cutting off the higher bits.
+        u128::min(milliseconds, MAXDWORD as u128 - 1) as DWORD
+    }
 }
 
 impl Drop for COMPort {
@@ -248,14 +260,14 @@ impl SerialPort for COMPort {
     }
 
     fn set_timeout(&mut self, timeout: Duration) -> Result<()> {
-        let milliseconds = timeout.as_millis();
+        let timeout_constant = Self::timeout_constant(timeout);
 
         let mut timeouts = COMMTIMEOUTS {
             ReadIntervalTimeout: MAXDWORD,
             ReadTotalTimeoutMultiplier: MAXDWORD,
-            ReadTotalTimeoutConstant: milliseconds as DWORD,
+            ReadTotalTimeoutConstant: timeout_constant,
             WriteTotalTimeoutMultiplier: 0,
-            WriteTotalTimeoutConstant: milliseconds as DWORD,
+            WriteTotalTimeoutConstant: timeout_constant,
         };
 
         if unsafe { SetCommTimeouts(self.handle, &mut timeouts) } == 0 {
@@ -442,3 +454,29 @@ impl SerialPort for COMPort {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::tests::timeout::MONOTONIC_DURATIONS;
+
+    #[test]
+    fn timeout_constant_is_monotonic() {
+        let mut last = COMPort::timeout_constant(Duration::ZERO);
+
+        for (i, d) in MONOTONIC_DURATIONS.iter().enumerate() {
+            let next = COMPort::timeout_constant(*d);
+            dbg!((i, d));
+            assert!(
+                next >= last,
+                "{next} >= {last} failed for {d:?} at index {i}"
+            );
+            last = next;
+        }
+    }
+
+    #[test]
+    fn timeout_constant_zero_is_zero() {
+        assert_eq!(0, COMPort::timeout_constant(Duration::ZERO));
+    }
+}

src/windows/enumerate.rs

@@ -562,7 +562,7 @@ pub fn available_ports() -> Result<Vec<SerialPortInfo>> {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
     use super::*;
 
     use quickcheck_macros::quickcheck;