-
-
Notifications
You must be signed in to change notification settings - Fork 104
Flexible timeouts for larger emails #1755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is ready to be tested, can someone help me with this? (I have no idea how to do this) |
as discussed with @adbenitez -- how about using a 10 minute timeout on waiting for the "ok" after sendmail? see https://tools.ietf.org/html/rfc5321#section-4.5.3.2.6 --- seems @adbenitez thinks this would solve the problem. |
I know too few about networking to have a strong opinion on this.
|
@dignifiedquire or @link2xt will have to decide this. |
I would just set it to 600 s and see how it works. It is possible that there will be problems with lost connections on mobile devices, in which case OK will never arrive. In this case it is impossible to determine if the message was delivered and it will be resent, it is impossible to do anything about it. To avoid email sending being stuck for the whole 10 minutes in this case, a new connection should be opened. Eventually core needs a connection pool for SMTP instead of just one connection, but let's see how common this problem is with 10 minute timeout first. |
@@ -47,11 +48,16 @@ impl Smtp { | |||
); | |||
|
|||
if let Some(ref mut transport) = self.transport { | |||
transport.send(mail).await.map_err(Error::SendError)?; | |||
// The timeout is 1min + 3min per MB. | |||
let timeout = 60 + (180 * message_len_bytes / 1_000_000) as u64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I proposed is to just set it to 600 seconds. Maybe actually it is not such a good idea, because there is a chance to get stuck for 10 minutes while sending a small message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think it is good as-is
@hpk42 please don't merge PRs that depend on feature branches, this has broken current master |
Fix #1383, depends on chatmail/async-smtp#26.
The timeout is 60s + 180 extra seconds per MB now. This is about twice the amounts @hpk42 reported in the original issue.