Fix hang on TTY read #192
Fix hang on TTY read #192
Conversation
In some cases, when a underlying error happens to a TTY port between poll::wait_read_fd and unistd::read, the read function would hang waiting for some data that is never received. This commit sets the port to non-canonical mode, with VMIN = VTIME = 0. With this change, it has the effect of making reads non-blocking, returning right away. The timeout behaviour is maintained, as prior to reading we call unix::poll through poll::wait_read_fd. Fixes: serialport#7
There was a problem hiding this comment.
Thank you for looking into the dusty corners of our issue list @danielstuart14 and creating this PR!
In some cases, when a underlying error happens to a TTY port between poll::wait_read_fd and unistd::read, the read function would hang waiting for some data that is never received.
Yes, at a first glance this looks like something the actual code does not consider. Do you have an example to reproduce such a situation?
This commit sets the port to non-canonical mode, with VMIN = VTIME = 0. With this change, it has the effect of making reads non-blocking, returning right away.
Setting VMIN and VTIME to zero looks like the right thing for not blocking on the read after successfully polling for new data to read. But as annotated below - is VTIME set to `timeout unintentionally then?
And isn't there some explicit handling required in case reading returns 0, eventually with errno set?
The timeout behaviour is maintained, as prior to reading we call unix::poll through poll::wait_read_fd.
Wouldn't VTIME == timeout change the timeout behavior in when read is callen after polling?
| pub(crate) fn set_timeout(termios: &mut Termios, timeout: Duration) { | ||
| let timeout = u128::min(timeout.as_millis() / 100, u8::MAX as u128) as u8; | ||
| termios.c_cc[libc::VMIN as usize] = 0; | ||
| termios.c_cc[libc::VTIME as usize] = timeout; |
There was a problem hiding this comment.
The description (and the documentation of termios as well) mention setting VTIME to 0 as well. What's the reason for setting it to timeout here?
There was a problem hiding this comment.
I made this utils function to bridge how timeouts are set on termios (by setting VTIME to a value with 100ms steps) with the Duration type on rust.
If you check at
serialport-rs/src/posix/tty.rs
Line 181 in 8961365
Hey there! About VTIME, I'm setting it to zero through the bridge function I created, check my reply above. Any internal errors should be returned by the unistd "read" implementation. What maybe we could do is return an error if the poll function returned successfully but the read didn't return any data, but it would need some thinking regarding the behavior when timeout = 0 or buf length = 0. |
In some cases, when a underlying error happens to a TTY port between poll::wait_read_fd and unistd::read, the read function would hang waiting for some data that is never received.
This commit sets the port to non-canonical mode, with VMIN = VTIME = 0. With this change, it has the effect of making reads non-blocking, returning right away.
The timeout behaviour is maintained, as prior to reading we call unix::poll through poll::wait_read_fd.
Fixes: #7