more flexible timeouts #26
Conversation
src/smtp/client/inner.rs
Outdated
| let read = with_timeout(this.timeout.as_ref(), reader.read_line(&mut buffer)).await?; | ||
| let read = with_timeout(timeout, reader.read_line(&mut buffer)).await?; |
There was a problem hiding this comment.
I guess that this is the important part?
src/smtp/client/inner.rs
Outdated
| async fn write(mut self: Pin<&mut Self>, string: &[u8]) -> Result<(), Error> { | ||
| async fn write( | ||
| mut self: Pin<&mut Self>, | ||
| string: &[u8], | ||
| timeout: Option<&Duration>, | ||
| ) -> Result<(), Error> { |
There was a problem hiding this comment.
Thinking about it, is it even necessary to change the timeout here?
There was a problem hiding this comment.
RFC 1123 recommends to use 3 minutes per TCP SEND call. We don't have such low-level access and these recommendations are outdated because TCP buffer size is variable now, but I think what they mean is:
- You do a UNIX write/send call asking to write 1MB to the socket, and wait for 3 minutes.
- Call returns, telling you that 100kb were written to the buffer.
- You do a send call again, asking to write 900kb to the socket, and wait for 3 minutes again.
...
Essentially the write timeout of this whole operation is file size divided by TCP buffer size and multiplied by 3 minutes, so it should grow linearly with file size.
|
@dignifiedquire I could fix all those Clippy warnings, but the ci here tests on Beta branch of Rust and DeltaChat runs on release branch, which one shall I take? |
|
hmm CI is pretty broken, we should fix master first independently |
src/smtp/client/inner.rs
Outdated
| self.as_mut() | ||
| .write(command.to_string().as_bytes(), timeout) | ||
| .await?; | ||
| self.read_response(timeout).await |
There was a problem hiding this comment.
this uses the timeout twice, I believe the "correct" way would be
let fut = async move {
self.as_mut().write(command.to_string().as_bytes()).await?;
self.read_response().await
};
fut.timeout(timeout).awaitThere was a problem hiding this comment.
No, write timeout and read timeouts are separate.
It was changed in https://github.com/deltachat/deltachat-core-rust/pull/1755/files#diff-80398c5faae3c069e4e6aa2ed11b28c0. Can be merged as soon as chatmail/async-smtp#26 is merged (and I maybe did cargo update once)
I'll do this, but the ci here runs on beta branch of Rust and DC on the release branch. They changed some naming and I have to decide for beta or release. Which one shall I take? |
dignifiedquire
force-pushed
the
smtptimeout
branch
from
0545405 to
9513152
Compare
dignifiedquire
changed the title
| let mut message_bytes = Vec::new(); | ||
| message_reader.read_to_end(&mut message_bytes).await?; | ||
| let mut message_bytes = Vec::new(); | ||
| message_reader.read_to_end(&mut message_bytes).await?; |
There was a problem hiding this comment.
Why is this wrapped into timeout now? It is a completely local operation.
Fix #22