[geary/wip/789924-network-transition: 5/15] Make SmtpOutboxFolder postman more robust.
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/789924-network-transition: 5/15] Make SmtpOutboxFolder postman more robust.
- Date: Sun, 12 Nov 2017 11:11:04 +0000 (UTC)
commit 3fb723731ee75f980046dbadffe0fe63dd6cf50f
Author: Michael James Gratton <mike vee net>
Date: Tue Nov 7 11:15:15 2017 +1100
Make SmtpOutboxFolder postman more robust.
Currently there are a bunch of weird edge cases when things go wrong
sending mail. This commit cleans up the class futher to make sure no
dataloss occurs.
* src/engine/imap-db/outbox/smtp-outbox-folder.vala
(SmtpOutboxFolder::start_postman_async): Move all code related to
sending and saving mail out to a seperate postman_send() method. Ensure
that if any hard or soft error occurs, the row is always enqueued and a
nap is always had.
(SmtpOutboxFolder::postman_send): Rework how SMTP auth failures are
handled so we don't need to work around the napping code. Don't catch
hard errors, let them go through to the caller.
* src/engine/api/geary-account.vala (Account.Problem): Make send error
names consistent. Update uses.
src/client/application/geary-controller.vala | 10 +-
src/engine/api/geary-account.vala | 15 +-
src/engine/imap-db/outbox/smtp-outbox-folder.vala | 247 +++++++++++----------
3 files changed, 142 insertions(+), 130 deletions(-)
---
diff --git a/src/client/application/geary-controller.vala b/src/client/application/geary-controller.vala
index 8b541c1..451218f 100644
--- a/src/client/application/geary-controller.vala
+++ b/src/client/application/geary-controller.vala
@@ -889,15 +889,15 @@ public class GearyController : Geary.BaseObject {
// they've hit cancel, so there's not much for us to do here.
close_account(account);
break;
-
- case Geary.Account.Problem.EMAIL_DELIVERY_FAILURE:
+
+ case Geary.Account.Problem.SEND_EMAIL_DELIVERY_FAILURE:
handle_outbox_failure(StatusBar.Message.OUTBOX_SEND_FAILURE);
break;
-
- case Geary.Account.Problem.SAVE_SENT_MAIL_FAILED:
+
+ case Geary.Account.Problem.SEND_EMAIL_SAVE_FAILED:
handle_outbox_failure(StatusBar.Message.OUTBOX_SAVE_SENT_MAIL_FAILED);
break;
-
+
case Geary.Account.Problem.CONNECTION_FAILURE:
ErrorDialog dialog = new ErrorDialog(main_window,
_("Error connecting to the server"),
diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala
index b6f9f41..148127f 100644
--- a/src/engine/api/geary-account.vala
+++ b/src/engine/api/geary-account.vala
@@ -22,16 +22,17 @@
public abstract class Geary.Account : BaseObject {
public enum Problem {
- RECV_EMAIL_LOGIN_FAILED,
- SEND_EMAIL_LOGIN_FAILED,
+ CONNECTION_FAILURE,
+ DATABASE_FAILURE,
HOST_UNREACHABLE,
NETWORK_UNAVAILABLE,
- DATABASE_FAILURE,
- EMAIL_DELIVERY_FAILURE,
- SAVE_SENT_MAIL_FAILED,
- CONNECTION_FAILURE,
+ RECV_EMAIL_LOGIN_FAILED,
+ SEND_EMAIL_DELIVERY_FAILURE,
+ SEND_EMAIL_ERROR,
+ SEND_EMAIL_LOGIN_FAILED,
+ SEND_EMAIL_SAVE_FAILED,
}
-
+
public Geary.AccountInformation information { get; protected set; }
public Geary.ProgressMonitor search_upgrade_monitor { get; protected set; }
diff --git a/src/engine/imap-db/outbox/smtp-outbox-folder.vala
b/src/engine/imap-db/outbox/smtp-outbox-folder.vala
index 785a92e..910e8ee 100644
--- a/src/engine/imap-db/outbox/smtp-outbox-folder.vala
+++ b/src/engine/imap-db/outbox/smtp-outbox-folder.vala
@@ -1,4 +1,6 @@
-/* Copyright 2016 Software Freedom Conservancy Inc.
+/*
+ * Copyright 2017 Michael Gratton <mike vee net>
+ * Copyright 2016 Software Freedom Conservancy Inc.
*
* This software is licensed under the GNU Lesser General Public License
* (version 2.1 or later). See the COPYING file in this distribution.
@@ -23,6 +25,9 @@ private class Geary.SmtpOutboxFolder :
// loaded, connections to settle, pigs to fly, etc.
private const uint START_TIMEOUT = 4;
+ // Number of times to retry sending after auth failures
+ private const uint AUTH_ERROR_MAX_RETRY = 3;
+
private class OutboxRow {
public int64 id;
@@ -389,160 +394,166 @@ private class Geary.SmtpOutboxFolder :
// Start the send queue.
while (!cancellable.is_cancelled()) {
// yield until a message is ready
- OutboxRow row;
- bool mail_sent;
+ OutboxRow? row = null;
try {
row = yield this.outbox_queue.recv_async(cancellable);
- mail_sent = !yield is_unsent_async(row.ordering, cancellable);
- } catch (Error wait_err) {
- if (!(wait_err is IOError.CANCELLED)) {
- debug("Outbox postman queue error: %s", wait_err.message);
+ if (yield postman_send(row, cancellable)) {
+ // send was good, reset nap length
+ send_retry_seconds = MIN_SEND_RETRY_INTERVAL_SEC;
+ } else {
+ // send was bad, try sending again later
+ this.outbox_queue.send(row);
+
+ if (!cancellable.is_cancelled()) {
+ debug("Outbox napping for %u seconds...", send_retry_seconds);
+ // Take a brief nap before continuing to allow
+ // connection problems to resolve.
+ yield Geary.Scheduler.sleep_async(send_retry_seconds);
+ send_retry_seconds = Geary.Numeric.uint_ceiling(
+ send_retry_seconds * 2,
+ MAX_SEND_RETRY_INTERVAL_SEC
+ );
+ }
+ }
+ } catch (Error err) {
+ // A hard error occurred. This will cause the postman
+ // to exit but we still want to re-queue the row in
+ // case it restarts.
+ if (row != null) {
+ this.outbox_queue.send(row);
}
- break;
+ debug("Outbox postman error: %s", err.message);
+ if (err is SmtpError.AUTHENTICATION_FAILED) {
+ report_problem(Geary.Account.Problem.SEND_EMAIL_LOGIN_FAILED, err);
+ } else if (!(err is IOError.CANCELLED)) {
+ report_problem(Geary.Account.Problem.SEND_EMAIL_ERROR, err);
+ }
+ // Get out of here
+ cancellable.cancel();
}
+ }
- // Convert row into RFC822 message suitable for sending or framing
- RFC822.Message message;
- try {
- message = new RFC822.Message.from_buffer(row.message);
- } catch (RFC822Error msg_err) {
- // TODO: This needs to be reported to the user
- debug("Outbox postman message error: %s", msg_err.message);
+ this.queue_cancellable = null;
+ debug("Exiting outbox postman");
+ }
- continue;
- }
+ // Returns true if row was successfully processed, else false
+ private async bool postman_send(OutboxRow row, Cancellable cancellable)
+ throws Error {
+ AccountInformation account = this.account.information;
+ bool mail_sent = !yield is_unsent_async(row.ordering, cancellable);
- if (!mail_sent) {
- // Get SMTP password if we haven't loaded it yet and the account needs credentials.
- // If the account needs a password but it's not set or incorrect in the keyring, we'll
- // prompt below after getting an AUTHENTICATION_FAILED error.
- if (_account.information.smtp_credentials != null &&
- !_account.information.smtp_credentials.is_complete()) {
- try {
- yield _account.information.get_passwords_async(ServiceFlag.SMTP);
- } catch (Error e) {
- debug("SMTP password fetch error: %s", e.message);
- }
- }
+ // Convert row into RFC822 message suitable for sending or framing
+ RFC822.Message message;
+ try {
+ message = new RFC822.Message.from_buffer(row.message);
+ } catch (RFC822Error msg_err) {
+ // TODO: This needs to be reported to the user
+ debug("Outbox postman message error: %s", msg_err.message);
+ return false;
+ }
- // Send the message, but only remove from database once sent
- bool should_nap = false;
+ if (!mail_sent) {
+ // Get SMTP password if we haven't loaded it yet and the account needs credentials.
+ // If the account needs a password but it's not set or incorrect in the keyring, we'll
+ // prompt below after getting an AUTHENTICATION_FAILED error.
+ if (account.smtp_credentials != null &&
+ !account.smtp_credentials.is_complete()) {
try {
- // only try if (a) no TLS issues or (b) user has acknowledged them and says to
- // continue
- if (this.smtp_endpoint.is_trusted_or_never_connected) {
- debug("Outbox postman: Sending \"%s\" (ID:%s)...",
- message_subject(message), row.outbox_id.to_string());
- yield send_email_async(message, cancellable);
- mail_sent = true;
- } else {
- // user was warned via Geary.Engine signal, need to wait for that to be cleared
- // befor sending
- outbox_queue.send(row);
- should_nap = true;
- }
- } catch (Error send_err) {
- debug("Outbox postman send error, retrying: %s", send_err.message);
+ yield account.get_passwords_async(ServiceFlag.SMTP);
+ } catch (Error e) {
+ debug("SMTP password fetch error: %s", e.message);
+ }
+ }
- should_nap = true;
+ // only try sending if (a) no TLS issues or (b) user has
+ // acknowledged them and says to continue
+ if (!this.smtp_endpoint.is_trusted_or_never_connected) {
+ return false;
+ }
+ // We immediately retry auth errors after the prompting
+ // the user, but if they get it wrong enough times or
+ // cancel we have no choice other than to stop the postman
+ int attempts = 0;
+ while (!mail_sent && ++attempts <= AUTH_ERROR_MAX_RETRY) {
+ try {
+ debug("Outbox postman: Sending \"%s\" (ID:%s)...",
+ message_subject(message), row.outbox_id.to_string());
+ yield send_email_async(message, cancellable);
+ mail_sent = true;
+ } catch (Error send_err) {
+ debug("Outbox postman send error: %s", send_err.message);
if (send_err is SmtpError.AUTHENTICATION_FAILED) {
- bool report = true;
-
- // Retry immediately (bug 6387)
- should_nap = false;
-
// At this point we may already have a password in memory -- but it's incorrect.
// Delete the current password, prompt the user for a new one, and try again.
+ bool user_confirmed = false;
try {
- if (yield _account.information.fetch_passwords_async(ServiceFlag.SMTP, true))
- report = false;
+ user_confirmed = yield account.fetch_passwords_async(
+ ServiceFlag.SMTP, true
+ );
} catch (Error e) {
debug("Error prompting for SMTP password: %s", e.message);
}
- if (report)
- report_problem(Geary.Account.Problem.SEND_EMAIL_LOGIN_FAILED, send_err);
+ if (!user_confirmed) {
+ // The user cancelled hence they don't
+ // want to be prompted again, so report it
+ // and bail out.
+ throw send_err;
+ }
} else if (send_err is TlsError) {
// up to application to be aware of problem via Geary.Engine, but do nap and
// try later
debug("TLS connection warnings connecting to %s, user must confirm connection to
continue",
this.smtp_endpoint.to_string());
} else {
- report_problem(Geary.Account.Problem.EMAIL_DELIVERY_FAILURE, send_err);
- }
- }
-
- if (should_nap && !cancellable.is_cancelled()) {
- debug("Outbox napping for %u seconds...", send_retry_seconds);
-
- // Take a brief nap before continuing to allow connection problems to resolve.
- yield Geary.Scheduler.sleep_async(send_retry_seconds);
- send_retry_seconds = Geary.Numeric.uint_ceiling(send_retry_seconds * 2,
MAX_SEND_RETRY_INTERVAL_SEC);
- }
-
- // If we got this far the send was successful, so reset the send retry interval.
- send_retry_seconds = MIN_SEND_RETRY_INTERVAL_SEC;
- // Mark as sent, so if there's a problem pushing up to Sent Mail,
- // we don't retry sending.
- if (mail_sent) {
- try {
- debug("Outbox postman: Marking %s as sent", row.outbox_id.to_string());
- // Don't observe the cancellable here - if
- // it's been sent but not saved, we want to
- // try to update the sent flag anyway
- yield mark_email_as_sent_async(row.outbox_id, null);
- } catch (Error e) {
- debug("Outbox postman: Unable to mark row as sent: %s", e.message);
+ report_problem(Geary.Account.Problem.SEND_EMAIL_DELIVERY_FAILURE, send_err);
}
}
}
- if (cancellable.is_cancelled()) {
- break;
- }
-
- if (!mail_sent) {
- // try again later
- outbox_queue.send(row);
- continue;
- }
-
- bool mail_saved = true;
- if (_account.information.allow_save_sent_mail() && _account.information.save_sent_mail) {
- mail_saved = false;
- try {
- debug("Outbox postman: Saving %s to sent mail", row.outbox_id.to_string());
- yield save_sent_mail_async(message, cancellable);
- mail_saved = true;
- } catch (Error e) {
- debug("Outbox postman: Error saving sent mail: %s", e.message);
- report_problem(Geary.Account.Problem.SAVE_SENT_MAIL_FAILED, e);
- }
+ // Mark as sent, so if there's a problem pushing up to
+ // Sent, we don't retry sending. Don't observe the
+ // cancellable here - if it's been sent we want to try to
+ // update the sent flag anyway
+ if (mail_sent) {
+ debug("Outbox postman: Marking %s as sent", row.outbox_id.to_string());
+ yield mark_email_as_sent_async(row.outbox_id, null);
}
- if (!mail_saved) {
+ if (!mail_sent || cancellable.is_cancelled()) {
// try again later
- outbox_queue.send(row);
- continue;
+ return false;
}
+ }
- // Remove from database ... can't use remove_email_async() because this runs even if
- // the outbox is closed as a Geary.Folder.
+ // If we get to this point, the message has either been just
+ // sent, or previously sent but not saved. So now try saving
+ // if needed.
+ if (account.allow_save_sent_mail() &&
+ account.save_sent_mail) {
try {
- debug("Outbox postman: Deleting row %s", row.outbox_id.to_string());
- Gee.ArrayList<SmtpOutboxEmailIdentifier> list = new
Gee.ArrayList<SmtpOutboxEmailIdentifier>();
- list.add(row.outbox_id);
- // Don't observe the cancellable here - if it's been
- // saved we want to try to remove it anyway
- yield internal_remove_email_async(list, null);
- } catch (Error e) {
- debug("Outbox postman: Unable to delete row: %s", e.message);
+ debug("Outbox postman: Saving %s to sent mail", row.outbox_id.to_string());
+ yield save_sent_mail_async(message, cancellable);
+ } catch (Error err) {
+ debug("Outbox postman: Error saving sent mail: %s", err.message);
+ report_problem(Geary.Account.Problem.SEND_EMAIL_SAVE_FAILED, err);
+ return false;
}
}
- this.queue_cancellable = null;
- debug("Exiting outbox postman");
+ // Remove from database ... can't use remove_email_async()
+ // because this runs even if the outbox is closed as a
+ // Geary.Folder. Again, don't observe the cancellable here -
+ // if it's been send and saved we want to try to remove it
+ // anyway.
+ debug("Outbox postman: Deleting row %s", row.outbox_id.to_string());
+ Gee.ArrayList<SmtpOutboxEmailIdentifier> list = new Gee.ArrayList<SmtpOutboxEmailIdentifier>();
+ list.add(row.outbox_id);
+ yield internal_remove_email_async(list, null);
+
+ return true;
}
private void stop_postman() {
@@ -648,7 +659,7 @@ private class Geary.SmtpOutboxFolder :
}
try {
- // no always logout
+ // always logout
yield smtp.logout_async(false, null);
} catch (Error err) {
debug("Unable to disconnect from SMTP server %s: %s", smtp.to_string(), err.message);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
