From 46bb4d1b6cf90f120cd77ff1fcdae3596b4659fc Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Mon, 14 Jan 2019 11:29:27 +1100 Subject: [PATCH] Ensure MinimalFolder remote open forces closed on hard errors Minimal folder should force a folder closed when an error occurrs that it isn't going to be able to recover from. --- .../imap-engine-minimal-folder.vala | 18 +++-- .../imap-engine/imap-engine-replay-queue.vala | 8 +- src/engine/imap-engine/imap-engine.vala | 79 +++++++++++-------- 3 files changed, 61 insertions(+), 44 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala b/src/engine/imap-engine/imap-engine-minimal-folder.vala index ed3d9f09..e9204bba 100644 --- a/src/engine/imap-engine/imap-engine-minimal-folder.vala +++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala @@ -960,12 +960,18 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport ); return; } catch (Error err) { - debug("Other error: %s", err.message); - // Notify that there was a connection error, but don't - // force the folder closed, since it might come good again - // if the user fixes an auth problem or the network comes - // back or whatever. - notify_open_failed(Folder.OpenFailed.REMOTE_ERROR, err); + ErrorContext context = new ErrorContext(err); + if (is_unrecoverable_failure(err)) { + debug("Unrecoverable failure opening remote, forcing closed: %s", + context.format_full_error()); + yield force_close( + CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_ERROR + ); + } else { + debug("Recoverable error opening remote: %s", + context.format_full_error()); + notify_open_failed(Folder.OpenFailed.REMOTE_ERROR, err); + } return; } diff --git a/src/engine/imap-engine/imap-engine-replay-queue.vala b/src/engine/imap-engine/imap-engine-replay-queue.vala index 7238ffa0..8d5e2e61 100644 --- a/src/engine/imap-engine/imap-engine-replay-queue.vala +++ b/src/engine/imap-engine/imap-engine-replay-queue.vala @@ -519,11 +519,11 @@ private class Geary.ImapEngine.ReplayQueue : Geary.BaseObject { } catch (Error replay_err) { debug("Replay remote error for %s on %s: %s (%s)", op.to_string(), to_string(), replay_err.message, op.on_remote_error.to_string()); - - // If a hard failure and operation allows remote replay and not closing, - // re-schedule now + + // If a recoverable failure and operation allows + // remote replay and not closing, re-schedule now if ((op.on_remote_error == ReplayOperation.OnError.RETRY) - && is_hard_failure(replay_err) + && !is_unrecoverable_failure(replay_err) && state == State.OPEN) { debug("Schedule op retry %s on %s", op.to_string(), to_string()); diff --git a/src/engine/imap-engine/imap-engine.vala b/src/engine/imap-engine/imap-engine.vala index 5bad3088..998bb1b8 100644 --- a/src/engine/imap-engine/imap-engine.vala +++ b/src/engine/imap-engine/imap-engine.vala @@ -6,40 +6,51 @@ namespace Geary.ImapEngine { -/** - * A hard failure is defined as one due to hardware or connectivity issues, where a soft failure - * is due to software reasons, like credential failure or protocol violation. - */ -private static bool is_hard_failure(Error err) { - // CANCELLED is not a hard error - if (err is IOError.CANCELLED) - return false; + /** + * Determines if retrying an operation might succeed or not. + * + * A recoverable failure is defined as one that may not occur + * again if the operation that caused it is retried, without + * needing to make some change in the mean time. For example, + * recoverable failures may occur due to transient network + * connectivity issues or server rate limiting. On the other hand, + * an unrecoverable failure is due to some problem that will not + * succeed if tried again unless some action is taken, such as + * authentication failures, protocol parsing errors, and so on. + */ + private static bool is_unrecoverable_failure(GLib.Error err) { + return !( + err is EngineError.SERVER_UNAVAILABLE || + err is IOError.BROKEN_PIPE || + err is IOError.BUSY || + err is IOError.CONNECTION_CLOSED || + err is IOError.NOT_CONNECTED || + err is IOError.TIMED_OUT || + err is ImapError.NOT_CONNECTED || + err is ImapError.TIMED_OUT || + err is ImapError.UNAVAILABLE + ); + } - // Treat other errors -- most likely IOErrors -- as hard failures - if (!(err is ImapError) && !(err is EngineError)) - return true; - - return err is ImapError.NOT_CONNECTED - || err is ImapError.TIMED_OUT - || err is ImapError.SERVER_ERROR - || err is EngineError.SERVER_UNAVAILABLE; -} - -/** - * Determines if this IOError related to a remote host or not. - */ -private static bool is_remote_error(GLib.Error err) { - return err is ImapError - || err is IOError.CONNECTION_CLOSED - || err is IOError.CONNECTION_REFUSED - || err is IOError.HOST_UNREACHABLE - || err is IOError.MESSAGE_TOO_LARGE - || err is IOError.NETWORK_UNREACHABLE - || err is IOError.NOT_CONNECTED - || err is IOError.PROXY_AUTH_FAILED - || err is IOError.PROXY_FAILED - || err is IOError.PROXY_NEED_AUTH - || err is IOError.PROXY_NOT_ALLOWED; -} + /** + * Determines if an error was caused by the remote host or not. + */ + private static bool is_remote_error(GLib.Error err) { + return ( + err is EngineError.NOT_FOUND || + err is EngineError.SERVER_UNAVAILABLE || + err is IOError.CONNECTION_CLOSED || + err is IOError.CONNECTION_REFUSED || + err is IOError.HOST_UNREACHABLE || + err is IOError.MESSAGE_TOO_LARGE || + err is IOError.NETWORK_UNREACHABLE || + err is IOError.NOT_CONNECTED || + err is IOError.PROXY_AUTH_FAILED || + err is IOError.PROXY_FAILED || + err is IOError.PROXY_NEED_AUTH || + err is IOError.PROXY_NOT_ALLOWED || + err is ImapError + ); + } }