Repository: serialport/serialport-rs (language: rust)

Issue:
SerialPortBuilder.timeout(Duration::MAX) gives "Invalid argument"
When serial port is created through `SerialPortBuilder` and `.timeout()` is called with the argument set to `Duration::MAX`, any attempt to read from the port returns an error `Custom { kind: Other, error: "Invalid argument" }`.
Apparently there is no other way to set blocking read without a timeout.

Code to reproduce:
```rust
use std::{time::Duration, error::Error};

fn main() -> Result<(), Box<dyn Error>> {
    let mut serial = serialport::new("/dev/ttyUSB0", 9600)
        .timeout(Duration::MAX)
        .open()?;
    let mut tmp = vec![0];
    serial.read(&mut tmp)?;
    dbg!(tmp);
    Ok(())
}
```

Environment:
* Fedora 40
* 6.9.11-200.fc40.x86_64
* rustc 1.79.0 (129f3b996 2024-06-10)
* serialport = "4.4.0"
* `/dev/ttyUSB0`: ID 1a86:7523 QinHeng Electronics CH340 serial converter 
* also reproducible with `/dev/tty`


Discussion / hints:

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e2f3b57f..1550890d 100644
--- a/CHANGELOG.md
+++ b/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
 
 
diff --git a/Cargo.toml b/Cargo.toml
index 1608f758..32bbad0f 100644
--- a/Cargo.toml
+++ b/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 }
 
diff --git a/src/lib.rs b/src/lib.rs
index 200374e8..25242d1b 100644
--- a/src/lib.rs
+++ b/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
diff --git a/src/posix/enumerate.rs b/src/posix/enumerate.rs
index 44c44a33..209e7cd0 100644
--- a/src/posix/enumerate.rs
+++ b/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;
diff --git a/src/posix/poll.rs b/src/posix/poll.rs
index e45226ea..e193ac0e 100644
--- a/src/posix/poll.rs
+++ b/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());
+    }
+}
diff --git a/src/windows/com.rs b/src/windows/com.rs
index 20e243fb..21ed0486 100644
--- a/src/windows/com.rs
+++ b/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));
+    }
+}
diff --git a/src/windows/enumerate.rs b/src/windows/enumerate.rs
index 57333ec2..c303e246 100644
--- a/src/windows/enumerate.rs
+++ b/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;


Test patch:
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
new file mode 100644
index 00000000..deb6b6ae
--- /dev/null
+++ b/src/tests/mod.rs
@@ -0,0 +1,7 @@
+use cfg_if::cfg_if;
+
+cfg_if! {
+    if #[cfg(test)] {
+        pub(crate) mod timeout;
+    }
+}
diff --git a/src/tests/timeout.rs b/src/tests/timeout.rs
new file mode 100644
index 00000000..dd53c172
--- /dev/null
+++ b/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;
+        }
+    }
+}
diff --git a/tests/test_timeout.rs b/tests/test_timeout.rs
index fc51c9aa..a0627d29 100644
--- a/tests/test_timeout.rs
+++ b/tests/test_timeout.rs
@@ -59,7 +59,7 @@ fn test_read_returns_available_data_before_timeout(
                     );
                     received_message.extend_from_slice(&buffer[..read]);
                 }
-                _ => assert!(false),
+                e => panic!("unexpected error {:?}", e),
             }
 
             if received_message.len() >= expected_message.len() {
@@ -79,7 +79,7 @@ fn test_read_returns_available_data_before_timeout(
 
             println!("send: {} bytes", chunk.len());
 
-            next = next + send_period;
+            next += send_period;
             thread::sleep(next - Instant::now());
         }
     });
@@ -178,3 +178,50 @@ fn test_timeout_greater_zero(hw_config: HardwareConfig, #[case] timeout: Duratio
     assert!(flushed_at + timeout + margin > read_at);
     assert_eq!(buffer[..read], message[..read]);
 }
+
+/// Checks that reading data with a timeout of `Duration::MAX` returns some data and no error. It
+/// does not check the actual timeout for obvious reason.
+#[rstest]
+#[cfg_attr(feature = "ignore-hardware-tests", ignore)]
+fn test_timeout_max(hw_config: HardwareConfig) {
+    let sleep = Duration::from_millis(3000);
+    let margin = Duration::from_millis(500);
+    let mut sender = serialport::new(hw_config.port_1, 115200).open().unwrap();
+    let mut receiver = serialport::new(hw_config.port_2, 115200)
+        .timeout(Duration::MAX)
+        .open()
+        .unwrap();
+
+    let message =
+        b"0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~";
+    let mut buffer: [u8; 1024] = [0xff; 1024];
+
+    sender.clear(ClearBuffer::All).unwrap();
+    receiver.clear(ClearBuffer::All).unwrap();
+
+    let started_at = Instant::now();
+
+    let sender_thread = thread::spawn(move || {
+        thread::sleep(sleep);
+
+        sender.write_all(message).unwrap();
+        sender.flush().unwrap();
+    });
+
+    let read = receiver.read(&mut buffer).unwrap();
+    let read_at = Instant::now();
+
+    println!(
+        "read: {} bytes of {} after {} ms",
+        read,
+        message.len(),
+        (Instant::now() - started_at).as_millis()
+    );
+
+    assert!(read > 0);
+    assert!(read_at > started_at + sleep);
+    assert!(read_at < started_at + sleep + margin);
+    assert_eq!(buffer[..read], message[..read]);
+
+    sender_thread.join().unwrap();
+}