From 9d44f0e06232661259681d64002dd53c7c43099d Mon Sep 17 00:00:00 2001 From: cketti Date: Mon, 22 Jun 2015 00:37:48 +0200 Subject: [PATCH] Improve send failure handling We now no longer parse the exception message in MessagingController to find out if it was a permanent SMTP failure. Overall the code is still a mess, but the error handling should be a little bit better and more readable now. --- .../fsck/k9/mail/transport/SmtpTransport.java | 27 +++---- .../k9/controller/MessagingController.java | 72 +++++++++++++------ 2 files changed, 63 insertions(+), 36 deletions(-) diff --git a/k9mail-library/src/main/java/com/fsck/k9/mail/transport/SmtpTransport.java b/k9mail-library/src/main/java/com/fsck/k9/mail/transport/SmtpTransport.java index c6292a569..1e2ccec6a 100644 --- a/k9mail-library/src/main/java/com/fsck/k9/mail/transport/SmtpTransport.java +++ b/k9mail-library/src/main/java/com/fsck/k9/mail/transport/SmtpTransport.java @@ -491,7 +491,6 @@ public class SmtpTransport extends Transport { private void sendMessageTo(List addresses, Message message) throws MessagingException { - boolean possibleSend = false; close(); open(); @@ -503,13 +502,11 @@ public class SmtpTransport extends Transport { // the size of messages, count the message's size before sending it if (mLargestAcceptableMessage > 0 && message.hasAttachments()) { if (message.calculateSize() > mLargestAcceptableMessage) { - MessagingException me = new MessagingException("Message too large for server"); - //TODO this looks rather suspicious... shouldn't it be true? - me.setPermanentFailure(possibleSend); - throw me; + throw new MessagingException("Message too large for server", true); } } + boolean entireMessageSent = false; Address[] from = message.getFrom(); try { executeSimpleCommand("MAIL FROM:" + "<" + from[0].getAddress() + ">" @@ -527,20 +524,14 @@ public class SmtpTransport extends Transport { // We use BufferedOutputStream. So make sure to call flush() ! msgOut.flush(); - possibleSend = true; // After the "\r\n." is attempted, we may have sent the message + entireMessageSent = true; // After the "\r\n." is attempted, we may have sent the message executeSimpleCommand("\r\n."); + } catch (NegativeSmtpReplyException e) { + throw e; } catch (Exception e) { MessagingException me = new MessagingException("Unable to send message", e); + me.setPermanentFailure(entireMessageSent); - // "5xx text" -responses are permanent failures - String msg = e.getMessage(); - if (msg != null && msg.startsWith("5")) { - Log.w(LOG_TAG, "handling 5xx SMTP error code as a permanent failure"); - possibleSend = false; - } - - //TODO this looks rather suspicious... why is possibleSend used, and why are 5xx NOT permanent (in contrast to the log text) - me.setPermanentFailure(possibleSend); throw me; } finally { close(); @@ -775,11 +766,15 @@ public class SmtpTransport extends Transport { private final String mReplyText; public NegativeSmtpReplyException(int replyCode, String replyText) { - super("Negative SMTP reply: " + replyCode + " " + replyText); + super("Negative SMTP reply: " + replyCode + " " + replyText, isPermanentSmtpError(replyCode)); mReplyCode = replyCode; mReplyText = replyText; } + private static boolean isPermanentSmtpError(int replyCode) { + return replyCode >= 500 && replyCode <= 599; + } + public int getReplyCode() { return mReplyCode; } diff --git a/k9mail/src/main/java/com/fsck/k9/controller/MessagingController.java b/k9mail/src/main/java/com/fsck/k9/controller/MessagingController.java index bc2f79108..8aa648861 100644 --- a/k9mail/src/main/java/com/fsck/k9/controller/MessagingController.java +++ b/k9mail/src/main/java/com/fsck/k9/controller/MessagingController.java @@ -3367,6 +3367,7 @@ public class MessagingController implements Runnable { public void sendPendingMessagesSynchronous(final Account account) { Folder localFolder = null; Exception lastFailure = null; + boolean wasPermanentFailure = false; try { Store localStore = account.getLocalStore(); localFolder = localStore.getFolder( @@ -3463,38 +3464,40 @@ public class MessagingController implements Runnable { processPendingCommands(account); } - } catch (Exception e) { - // 5.x.x errors from the SMTP server are "PERMFAIL" - // move the message over to drafts rather than leaving it in the outbox - // This is a complete hack, but is worlds better than the previous - // "don't even bother" functionality - if (getRootCauseMessage(e).startsWith("5")) { - localFolder.moveMessages(Collections.singletonList(message), (LocalFolder) localStore.getFolder(account.getDraftsFolderName())); - } + } catch (CertificateValidationException e) { + lastFailure = e; + wasPermanentFailure = false; notifyUserIfCertificateProblem(context, e, account, false); - addErrorMessage(account, "Failed to send message", e); - message.setFlag(Flag.X_SEND_FAILED, true); - Log.e(K9.LOG_TAG, "Failed to send message", e); - for (MessagingListener l : getListeners()) { - l.synchronizeMailboxFailed(account, localFolder.getName(), getRootCauseMessage(e)); - } + handleSendFailure(account, localStore, localFolder, message, e, wasPermanentFailure); + } catch (MessagingException e) { lastFailure = e; + wasPermanentFailure = e.isPermanentFailure(); + + handleSendFailure(account, localStore, localFolder, message, e, wasPermanentFailure); + } catch (Exception e) { + lastFailure = e; + wasPermanentFailure = true; + + handleSendFailure(account, localStore, localFolder, message, e, wasPermanentFailure); } } catch (Exception e) { - Log.e(K9.LOG_TAG, "Failed to fetch message for sending", e); - for (MessagingListener l : getListeners()) { - l.synchronizeMailboxFailed(account, localFolder.getName(), getRootCauseMessage(e)); - } - addErrorMessage(account, "Failed to fetch message for sending", e); lastFailure = e; + wasPermanentFailure = false; + + Log.e(K9.LOG_TAG, "Failed to fetch message for sending", e); + + addErrorMessage(account, "Failed to fetch message for sending", e); + notifySynchronizeMailboxFailed(account, localFolder, e); } } + for (MessagingListener l : getListeners()) { l.sendPendingMessagesCompleted(account); } + if (lastFailure != null) { - if (getRootCauseMessage(lastFailure).startsWith("5")) { + if (wasPermanentFailure) { notifySendPermFailed(account, lastFailure); } else { notifySendTempFailed(account, lastFailure); @@ -3517,6 +3520,35 @@ public class MessagingController implements Runnable { } } + private void handleSendFailure(Account account, Store localStore, Folder localFolder, Message message, + Exception exception, boolean permanentFailure) throws MessagingException { + + Log.e(K9.LOG_TAG, "Failed to send message", exception); + + if (permanentFailure) { + moveMessageToDraftsFolder(account, localFolder, localStore, message); + } + + addErrorMessage(account, "Failed to send message", exception); + message.setFlag(Flag.X_SEND_FAILED, true); + + notifySynchronizeMailboxFailed(account, localFolder, exception); + } + + private void moveMessageToDraftsFolder(Account account, Folder localFolder, Store localStore, Message message) + throws MessagingException { + LocalFolder draftsFolder = (LocalFolder) localStore.getFolder(account.getDraftsFolderName()); + localFolder.moveMessages(Collections.singletonList(message), draftsFolder); + } + + private void notifySynchronizeMailboxFailed(Account account, Folder localFolder, Exception exception) { + String folderName = localFolder.getName(); + String errorMessage = getRootCauseMessage(exception); + for (MessagingListener listener : getListeners()) { + listener.synchronizeMailboxFailed(account, folderName, errorMessage); + } + } + public void getAccountStats(final Context context, final Account account, final MessagingListener listener) {