From a8ab7c9c04a10f764f0874b78118048bad429ebb Mon Sep 17 00:00:00 2001 From: link2xt Date: Sat, 2 Apr 2022 13:26:02 +0000 Subject: [PATCH] smtp: do not try to use stale connections Previously first try used an old connection and. If using stale connection, an immediate retry was performed using a new connection. Now if connection is stale, it is closed immediately without trying to use it. This should reduce the delay in cases when old connection is unusable. --- CHANGELOG.md | 1 + src/job.rs | 19 +++++--- src/smtp.rs | 119 ++++++++++++++++++++++++--------------------------- 3 files changed, 70 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0398bf25..7c63a8439 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ the last copy #3138 - speed up loading of chat messages #3171 - clear more columns when message expires due to `delete_device_after` setting #3181 +- do not try to use stale SMTP connections #3180 ### Changes - add more SMTP logging #3093 diff --git a/src/job.rs b/src/job.rs index 71e4de87a..2f2650286 100644 --- a/src/job.rs +++ b/src/job.rs @@ -20,7 +20,7 @@ use crate::message::{Message, MsgId}; use crate::mimefactory::MimeFactory; use crate::param::{Param, Params}; use crate::scheduler::InterruptInfo; -use crate::smtp::{smtp_send, Smtp}; +use crate::smtp::{smtp_send, SendResult, Smtp}; use crate::sql; // results in ~3 weeks for the last backoff timespan @@ -318,13 +318,18 @@ impl Job { return Status::RetryLater; } - let status = smtp_send(context, &recipients, &body, smtp, msg_id, 0).await; - if matches!(status, Status::Finished(Ok(_))) { - // Remove additional SendMdn jobs we have aggregated into this one. - job_try!(kill_ids(context, &additional_job_ids).await); + match smtp_send(context, &recipients, &body, smtp, msg_id, 0).await { + SendResult::Success => { + // Remove additional SendMdn jobs we have aggregated into this one. + job_try!(kill_ids(context, &additional_job_ids).await); + Status::Finished(Ok(())) + } + SendResult::Retry => { + info!(context, "Temporary SMTP failure while sending an MDN"); + Status::RetryLater + } + SendResult::Failure(err) => Status::Finished(Err(err)), } - - status } /// Read the recipients from old emails sent by the user and add them as contacts. diff --git a/src/smtp.rs b/src/smtp.rs index 45a03106d..a1c1bfb15 100644 --- a/src/smtp.rs +++ b/src/smtp.rs @@ -4,14 +4,14 @@ pub mod send; use std::time::{Duration, SystemTime}; -use anyhow::{bail, format_err, Context as _, Result}; +use anyhow::{bail, format_err, Context as _, Error, Result}; use async_smtp::smtp::client::net::ClientTlsParameters; use async_smtp::smtp::response::{Category, Code, Detail}; use async_smtp::{smtp, EmailAddress, ServerAddress}; +use async_std::task; use crate::constants::DC_LP_AUTH_OAUTH2; use crate::events::EventType; -use crate::job::Status; use crate::login_param::{ dc_build_tls, CertificateChecks, LoginParam, ServerLoginParam, Socks5Config, }; @@ -50,7 +50,10 @@ impl Smtp { /// Disconnect the SMTP transport and drop it entirely. pub async fn disconnect(&mut self) { if let Some(mut transport) = self.transport.take() { - transport.close().await.ok(); + // Closing connection with a QUIT command may take some time, especially if it's a + // stale connection and an attempt to send the command times out. Send a command in a + // separate task to avoid waiting for reply or timeout. + task::spawn(async move { transport.close().await }); } self.last_success = None; } @@ -196,12 +199,18 @@ impl Smtp { } } +pub(crate) enum SendResult { + /// Message was sent successfully. + Success, + + /// Permanent error, message sending has failed. + Failure(Error), + + /// Temporary error, the message should be retried later. + Retry, +} + /// Tries to send a message. -/// -/// Returns Status::Finished if sending the message should not be retried anymore, -/// Status::RetryLater if sending should be postponed and Status::RetryNow if it is suspected that -/// temporary failure is caused by stale connection, in which case a second attempt to send the -/// same message may be done immediately. pub(crate) async fn smtp_send( context: &Context, recipients: &[async_smtp::EmailAddress], @@ -209,7 +218,7 @@ pub(crate) async fn smtp_send( smtp: &mut Smtp, msg_id: MsgId, rowid: i64, -) -> Status { +) -> SendResult { if std::env::var(crate::DCC_MIME_DEBUG).is_ok() { info!(context, "smtp-sending out mime message:"); println!("{}", message); @@ -217,6 +226,20 @@ pub(crate) async fn smtp_send( smtp.connectivity.set_working(context).await; + if smtp.has_maybe_stale_connection().await { + info!(context, "Closing stale connection"); + smtp.disconnect().await; + + if let Err(err) = smtp + .connect_configured(context) + .await + .context("failed to reopen stale SMTP connection") + { + smtp.last_send_error = Some(format!("{:#}", err)); + return SendResult::Retry; + } + } + let send_result = smtp .send(context, recipients, message.as_bytes(), rowid) .await; @@ -253,14 +276,14 @@ pub(crate) async fn smtp_send( if maybe_transient { info!(context, "Permanent error that is likely to actually be transient, postponing retry for later"); - Status::RetryLater + SendResult::Retry } else { info!(context, "Permanent error, message sending failed"); // If we do not retry, add an info message to the chat. // Yandex error "554 5.7.1 [2] Message rejected under suspicion of SPAM; https://ya.cc/..." // should definitely go here, because user has to open the link to // resume message sending. - Status::Finished(Err(format_err!("Permanent SMTP error: {}", err))) + SendResult::Failure(format_err!("Permanent SMTP error: {}", err)) } } async_smtp::smtp::error::Error::Transient(ref response) => { @@ -277,35 +300,29 @@ pub(crate) async fn smtp_send( // receive as a transient error are misconfigurations of the smtp server. // See info!(context, "Received extended status code {} for a transient error. This looks like a misconfigured smtp server, let's fail immediatly", first_word); - Status::Finished(Err(format_err!("Permanent SMTP error: {}", err))) + SendResult::Failure(format_err!("Permanent SMTP error: {}", err)) } else { info!( context, "Transient error with status code {}, postponing retry for later", first_word ); - Status::RetryLater + SendResult::Retry } } else { info!( context, "Transient error without status code, postponing retry for later" ); - Status::RetryLater + SendResult::Retry } } _ => { info!( context, - "Message sending failed without error returned by the server" + "Message sending failed without error returned by the server, retry later" ); - if smtp.has_maybe_stale_connection().await { - info!(context, "Connection is probably stale, retry immediately"); - Status::RetryNow - } else { - info!(context, "Connection is not stale, retry later"); - Status::RetryLater - } + SendResult::Retry } }; @@ -319,24 +336,24 @@ pub(crate) async fn smtp_send( // Local error, job is invalid, do not retry. smtp.disconnect().await; warn!(context, "SMTP job is invalid: {}", err); - Status::Finished(Err(err.into())) + SendResult::Failure(err.into()) } Err(crate::smtp::send::Error::NoTransport) => { // Should never happen. // It does not even make sense to disconnect here. error!(context, "SMTP job failed because SMTP has no transport"); - Status::Finished(Err(format_err!("SMTP has not transport"))) + SendResult::Failure(format_err!("SMTP has not transport")) } Err(crate::smtp::send::Error::Other(err)) => { // Local error, job is invalid, do not retry. smtp.disconnect().await; warn!(context, "unable to load job: {}", err); - Status::Finished(Err(err)) + SendResult::Failure(err) } - Ok(()) => Status::Finished(Ok(())), + Ok(()) => SendResult::Success, }; - if let Status::Finished(Err(err)) = &status { + if let SendResult::Failure(err) = &status { // We couldn't send the message, so mark it as failed message::set_msg_failed(context, msg_id, Some(err.to_string())).await; } @@ -432,7 +449,7 @@ pub(crate) async fn send_msg_to_smtp( return Ok(()); } - let status = match smtp_send( + let status = smtp_send( context, &recipients_list, body.as_str(), @@ -440,47 +457,25 @@ pub(crate) async fn send_msg_to_smtp( msg_id, rowid, ) - .await - { - Status::RetryNow => { - // Do a single retry immediately without increasing retry counter in case of stale - // connection. - info!(context, "Doing immediate retry to send message."); + .await; - // smtp_send just closed stale SMTP connection, reconnect and try again. - if let Err(err) = smtp - .connect_configured(context) - .await - .context("failed to reopen stale SMTP connection") - { - smtp.last_send_error = Some(format!("{:#}", err)); - return Err(err); - } - - smtp_send( - context, - &recipients_list, - body.as_str(), - smtp, - msg_id, - rowid, - ) - .await - } - status => status, - }; match status { - Status::Finished(res) => { + SendResult::Retry => {} + SendResult::Success | SendResult::Failure(_) => { context .sql .execute("DELETE FROM smtp WHERE id=?", paramsv![rowid]) .await?; - if res.is_ok() { - msg_id.set_delivered(context).await?; - } - res } - Status::RetryNow | Status::RetryLater => Err(format_err!("Retry")), + }; + + match status { + SendResult::Retry => Err(format_err!("Retry")), + SendResult::Success => { + msg_id.set_delivered(context).await?; + Ok(()) + } + SendResult::Failure(err) => Err(format_err!("{}", err)), } }