Keep track of absolute IDLE time for timeout#303
Conversation
jonhoo
left a comment
jonhoo
left a comment
There was a problem hiding this comment.
Thank you for this! Two nits, and then one thing that I think is wrong (but also easily fixed).
| // re-issue it at least every 29 minutes to avoid being logged off. | ||
| // This still allows a client to receive immediate mailbox updates even | ||
| // though it need only "poll" at half hour intervals. | ||
| let time_since_idle = Instant::now() - self.last_idle.map(Ok).unwrap_or_else(|| self.init())?; |
There was a problem hiding this comment.
| let time_since_idle = Instant::now() - self.last_idle.map(Ok).unwrap_or_else(|| self.init())?; | |
| let time_since_idle = self.last_idle.map(|t| Ok(t.elapsed())).unwrap_or_else(|| self.init())?; |
| // set_read_timeout() can fail if the argument is Some(0), which can never | ||
| // be the case here. | ||
| .unwrap(); |
There was a problem hiding this comment.
| // set_read_timeout() can fail if the argument is Some(0), which can never | |
| // be the case here. | |
| .unwrap(); | |
| .expect("cannot be Some(0) since time is monotonically increasing"); |
There was a problem hiding this comment.
Will it panic here on the first loop iteration when the timeout is set to Duration::ZERO?
If so, would it be better to return an error instead?
There was a problem hiding this comment.
An error is returned in that case
https://github.com/jonhoo/rust-imap/pull/303/files#diff-a11b127eccbbebaad79d0cfb2418302d979952456e70664a86161fc63b622de4R174
There was a problem hiding this comment.
It seems that .unwrap_or_else(|| self.init().map(|()| Duration::ZERO)) covers that Err(Error::Io(io::ErrorKind::TimedOut.into())) and leads to .set_read_timeout(Some(self.timeout - Duration::ZERO))
There was a problem hiding this comment.
Right, on the first iteration last_idle is None so the check is skipped. I'm moving it to fix this issue.
| // re-issue it at least every 29 minutes to avoid being logged off. | ||
| // This still allows a client to receive immediate mailbox updates even | ||
| // though it need only "poll" at half hour intervals. | ||
| let time_since_idle = Instant::now() - self.last_idle.map(Ok).unwrap_or_else(|| self.init())?; |
There was a problem hiding this comment.
I think this means that if init fails, we may fail to reset the read timeout to None if we've already gone around the loop at least one? Because the ? here will return from the function, not into the match.
There was a problem hiding this comment.
True. Also from self.terminate()? a couple of lines below. I'll change both to break on Err instead of using ?.
| if self.keepalive { | ||
| self.terminate()?; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Nice — this makes the flow much better.
z33ky
force-pushed
the
abs-idle-timeout
branch
3 times, most recently
from
a94dedb to
3df945a
Compare
| // set_read_timeout() can fail if the argument is Some(0), which can never be the | ||
| // case here. | ||
| self.session.stream.get_mut().set_read_timeout(None).unwrap(); |
There was a problem hiding this comment.
Is it possible to restore the original read_timeout ? When building a Client from custom stream, its timeout may have been customized.
There was a problem hiding this comment.
I don't see a reason why it wouldn't be possible.
The previous code overwrote the timeout without attempting to restore the original value as well, so that is a separate change that I'm not planning to implement.
The code used to apply the set timeout value to the TcpStream before entering the IDLE loop. This effectively resets the timeout after receiving and handling incoming messages, which nullifies the purpose of the timeout when messages are received. This change remembers when the IDLE command is sent initially and uses that value to set the remaining time for the TcpStream timeout. This allows the IDLE loop to reconnect the IDLE connection at the appropriate time. Fixes jonhoo#300.
| let result = loop { | ||
| match self.session.readline(&mut v) { | ||
| match { | ||
| // The server MAY consider a client inactive if it has an IDLE command | ||
| // running, and if such a server has an inactivity timeout it MAY log | ||
| // the client off implicitly at the end of its timeout period. Because | ||
| // of that, clients using IDLE are advised to terminate the IDLE and | ||
| // re-issue it at least every 29 minutes to avoid being logged off. | ||
| // This still allows a client to receive immediate mailbox updates even | ||
| // though it need only "poll" at half hour intervals. | ||
| self.last_idle | ||
| .map_or_else( | ||
| // If there's no self.last_idle, initialize the connection (and return a 0 time since idle). | ||
| || self.init().map(|()| Duration::ZERO), | ||
| |last_idle| Ok(last_idle.elapsed()), | ||
| ) | ||
| // If no error occurred, read from the stream. | ||
| .map(|time_since_idle| { | ||
| if self.timeout <= time_since_idle { | ||
| return Err(Error::Io(io::ErrorKind::TimedOut.into())); | ||
| } | ||
| self.session | ||
| .stream | ||
| .get_mut() | ||
| .set_read_timeout(Some(self.timeout - time_since_idle)) | ||
| .expect("cannot be Some(0) since that is guarded against"); | ||
| self.session.readline(&mut v) | ||
| }) | ||
| } { |
There was a problem hiding this comment.
How about changing to this flavor? The nesting is too deep.
let result = loop {
// The server MAY consider a client inactive if it has an IDLE command
// running, and if such a server has an inactivity timeout it MAY log
// the client off implicitly at the end of its timeout period. Because
// of that, clients using IDLE are advised to terminate the IDLE and
// re-issue it at least every 29 minutes to avoid being logged off.
// This still allows a client to receive immediate mailbox updates even
// though it need only "poll" at half hour intervals.
// If there's no self.last_idle, initialize the connection.
if self.last_idle.is_none() {
self.init()?;
}
let time_since_idle = last_idle.elapsed();
if self.timeout <= time_since_idle {
return Err(Error::Io(io::ErrorKind::TimedOut.into()));
}
self.session
.stream
.get_mut()
.set_read_timeout(Some(self.timeout - time_since_idle))
.expect("cannot be Some(0) since that is guarded against");
// If no error occurred, read from the stream.
match self.session.readline(&mut v) {
3 participants
Fixes #300.
This change is