[geary/wip/cx-reestablish] Improved connection reestablishment for IMAP folders
- From: Jim Nelson <jnelson src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/cx-reestablish] Improved connection reestablishment for IMAP folders
- Date: Wed, 14 Jan 2015 03:05:19 +0000 (UTC)
commit 9b850b646de3fd4d5fe63bbf7816d53408912792
Author: Jim Nelson <jim yorba org>
Date: Tue Jan 13 19:02:21 2015 -0800
Improved connection reestablishment for IMAP folders
This fixes a bug where the folder's open_count could underflow,
potentially blocking session reestablishment. This also doesn't allow
for error-handling calls to decide whether or not to force re-
establishment (sometimes needlessly), but to rely solely on the
open_count, which should be the final determinant (but can change btwn
async calls).
Finally, this adds some retry logic to AccountSynchronizer and a
back-off algorithm to ClientSessionManager.
.../imap-engine-account-synchronizer.vala | 17 ++-
.../imap-engine/imap-engine-minimal-folder.vala | 156 +++++++++++---------
.../replay-ops/imap-engine-replay-disconnect.vala | 5 +-
.../transport/imap-client-session-manager.vala | 20 ++-
4 files changed, 113 insertions(+), 85 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala
b/src/engine/imap-engine/imap-engine-account-synchronizer.vala
index 66d6aa3..3bcbaac 100644
--- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala
+++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala
@@ -7,6 +7,7 @@
private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
private const int FETCH_DATE_RECEIVED_CHUNK_COUNT = 25;
private const int SYNC_DELAY_SEC = 10;
+ private const int RETRY_SYNC_DELAY_SEC = 60;
public GenericAccount account { get; private set; }
@@ -80,7 +81,7 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
// treat as an availability check (i.e. as if the account had just opened) because
// just because this value has changed doesn't mean the contents in the folders
// have changed
- delayed_send_all(account.list_folders(), true);
+ delayed_send_all(account.list_folders(), true, SYNC_DELAY_SEC);
} catch (Error err) {
debug("Unable to schedule re-sync for %s due to prefetch time changing: %s",
account.to_string(), err.message);
@@ -96,7 +97,7 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
foreach (Folder folder in available)
unavailable_paths.remove(folder.path);
- delayed_send_all(available, true);
+ delayed_send_all(available, true, SYNC_DELAY_SEC);
}
if (unavailable != null) {
@@ -108,7 +109,7 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
}
private void on_folders_contents_altered(Gee.Collection<Folder> altered) {
- delayed_send_all(altered, false);
+ delayed_send_all(altered, false, SYNC_DELAY_SEC);
}
private void on_email_sent() {
@@ -121,8 +122,8 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
}
}
- private void delayed_send_all(Gee.Collection<Folder> folders, bool reason_available) {
- Timeout.add_seconds(SYNC_DELAY_SEC, () => {
+ private void delayed_send_all(Gee.Collection<Folder> folders, bool reason_available, int sec) {
+ Timeout.add_seconds(sec, () => {
// remove any unavailable folders
Gee.ArrayList<Folder> trimmed_folders = new Gee.ArrayList<Folder>();
foreach (Folder folder in folders) {
@@ -334,6 +335,9 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
debug("Unable to open %s: %s", folder.to_string(), err.message);
+ // retry later
+ delayed_send_all(iterate<Folder>(folder).to_array_list(), false, RETRY_SYNC_DELAY_SEC);
+
return true;
}
@@ -345,6 +349,9 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
debug("Error background syncing folder %s: %s", folder.to_string(), err.message);
+ // retry later
+ delayed_send_all(iterate<Folder>(folder).to_array_list(), false, RETRY_SYNC_DELAY_SEC);
+
// fallthrough and close
}
diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala
b/src/engine/imap-engine/imap-engine-minimal-folder.vala
index 0f5213d..2407a5f 100644
--- a/src/engine/imap-engine/imap-engine-minimal-folder.vala
+++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala
@@ -7,7 +7,7 @@
private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport.Copy,
Geary.FolderSupport.Mark, Geary.FolderSupport.Move {
private const int FORCE_OPEN_REMOTE_TIMEOUT_SEC = 10;
- private const int DEFAULT_REESTABLISH_DELAY_MSEC = 10;
+ private const int DEFAULT_REESTABLISH_DELAY_MSEC = 100;
private const int MAX_REESTABLISH_DELAY_MSEC = 30000;
public override Account account { get { return _account; } }
@@ -70,6 +70,8 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
if (open_count > 0)
warning("Folder %s destroyed without closing", to_string());
+ cancel_remote_open_timer();
+
local_folder.email_complete.disconnect(on_email_complete);
}
@@ -144,8 +146,8 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
}
}
- private async bool normalize_folders(Geary.Imap.Folder remote_folder, Geary.Folder.OpenFlags open_flags,
- Cancellable? cancellable) throws Error {
+ private async bool normalize_folders(Geary.Imap.Folder remote_folder, Cancellable? cancellable)
+ throws Error {
debug("%s: Begin normalizing remote and local folders", to_string());
Geary.Imap.FolderProperties local_properties = local_folder.get_properties();
@@ -488,8 +490,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
if (!remote_opened) {
debug("wait_for_open_async %s: opening remote on demand...", to_string());
- remote_opened = true;
- open_remote_async.begin(open_flags, null);
+ start_remote_open_now();
}
if (!yield remote_semaphore.wait_for_result_async(cancellable))
@@ -499,25 +500,23 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
public override async bool open_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable = null)
throws Error {
if (open_count++ > 0) {
- // even if opened or opening, respect the NO_DELAY flag
+ // even if opened or opening, or if forcing a re-open, respect the NO_DELAY flag
if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
- cancel_remote_open_timer();
- wait_for_open_async.begin();
+ // add NO_DELAY flag if it forces an open
+ if (!remote_opened)
+ this.open_flags |= OpenFlags.NO_DELAY;
+
+ start_remote_open_now();
}
- debug("Not opening %s: already open (open_count=%d)", to_string(), open_count);
+ debug("Not opening %s: already open", to_string());
return false;
}
+ // first open gets to name the flags, but see note above
this.open_flags = open_flags;
- open_internal(open_flags, cancellable);
-
- return true;
- }
-
- private void open_internal(Folder.OpenFlags open_flags, Cancellable? cancellable) {
remote_semaphore = new Geary.Nonblocking.ReportingSemaphore<bool>(false);
// start the replay queue
@@ -535,16 +534,32 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// meaning that folder normalization never happens and unsolicited notifications never
// arrive
if (open_flags.is_all_set(OpenFlags.NO_DELAY))
- wait_for_open_async.begin();
+ start_remote_open_now();
else
start_remote_open_timer();
+
+ return true;
}
private void start_remote_open_timer() {
if (open_remote_timer_id != 0)
Source.remove(open_remote_timer_id);
- open_remote_timer_id = Timeout.add_seconds(FORCE_OPEN_REMOTE_TIMEOUT_SEC, on_open_remote_timeout);
+ open_remote_timer_id = Timeout.add_seconds(FORCE_OPEN_REMOTE_TIMEOUT_SEC, () => {
+ start_remote_open_now();
+
+ return false;
+ });
+ }
+
+ private void start_remote_open_now() {
+ if (remote_opened)
+ return;
+
+ cancel_remote_open_timer();
+ remote_opened = true;
+
+ open_remote_async.begin(null);
}
private void cancel_remote_open_timer() {
@@ -555,18 +570,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
open_remote_timer_id = 0;
}
- private bool on_open_remote_timeout() {
- open_remote_timer_id = 0;
-
- // remote was not forced open due to caller, so open now
- wait_for_open_async.begin();
-
- return false;
- }
-
- private async void open_remote_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable) {
- cancel_remote_open_timer();
-
+ private async void open_remote_async(Cancellable? cancellable) {
// watch for folder closing before this call got a chance to execute
if (open_count == 0)
return;
@@ -589,7 +593,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// allow subclasses to examine the opened folder and resolve any vital
// inconsistencies
- if (yield normalize_folders(opening_folder, open_flags, cancellable)) {
+ if (yield normalize_folders(opening_folder, cancellable)) {
// update flags, properties, etc.
yield local.update_folder_select_examine_async(opening_folder, cancellable);
@@ -620,7 +624,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// schedule immediate close
close_internal_async.begin(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false,
- false, cancellable);
+ cancellable);
return;
}
@@ -645,14 +649,12 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
}
Folder.CloseReason remote_reason;
- bool force_reestablishment;
if (hard_failure) {
// hard failure, retry
debug("Hard failure opening or preparing remote folder %s, retrying: %s", to_string(),
open_err.message);
remote_reason = CloseReason.REMOTE_ERROR;
- force_reestablishment = true;
} else {
// soft failure, treat as failure to open
debug("Soft failure opening or preparing remote folder %s: %s", to_string(),
@@ -662,7 +664,6 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
open_err);
remote_reason = CloseReason.REMOTE_CLOSE;
- force_reestablishment = false;
}
// be sure to close opening_folder if it was fetched or opened
@@ -676,9 +677,8 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// stop before starting the close
opening_monitor.notify_finish();
- // schedule immediate close and force reestablishment
- close_internal_async.begin(CloseReason.LOCAL_CLOSE, remote_reason, force_reestablishment,
- false, null);
+ // schedule immediate close and connection reestablishment
+ close_internal_async.begin(CloseReason.LOCAL_CLOSE, remote_reason, false, null);
return;
}
@@ -715,7 +715,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// schedule immediate close
close_internal_async.begin(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false,
- false, cancellable);
+ cancellable);
return;
}
@@ -735,13 +735,13 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
if (remote_folder != null)
_properties.remove(remote_folder.properties);
- yield close_internal_async(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false, true,
+ yield close_internal_async(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, true,
cancellable);
}
// NOTE: This bypasses open_count and forces the Folder closed.
internal async void close_internal_async(Folder.CloseReason local_reason, Folder.CloseReason
remote_reason,
- bool force_reestablish, bool flush_pending, Cancellable? cancellable) {
+ bool flush_pending, Cancellable? cancellable) {
cancel_remote_open_timer();
// only flushing pending ReplayOperations if this is a "clean" close, not forced due to
@@ -780,7 +780,14 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
if (flush_pending)
closing_remote_folder = clear_remote_folder();
- if (closing_remote_folder != null || force_reestablish) {
+ // now treat remote as closed, i.e. a call to open_async() will reinitiate opening and not fall
+ // through (unless open_count is > 0) ... do this before close_remote_folder_async() since
+ // it's *possible* for it to loop back to open_async() before returning
+ remote_opened = false;
+
+ // use background call to (a) close remote if necessary and/or (b) reestablish connection if
+ // necessary
+ if (closing_remote_folder != null || open_count > 0) {
// to avoid keeping the caller waiting while the remote end closes (i.e. drops the
// connection or performs an IMAP CLOSE operation), close it in the background and
// reestablish connection there, if necessary
@@ -793,19 +800,15 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// closed. Also not a problem, as GenericAccount does that internally. However, this
// might be an issue if GenericAccount removes this folder due to a user command or
// detection on the server, so this background op keeps a reference to the Folder
- close_remote_folder_async.begin(this, closing_remote_folder, remote_reason,
- force_reestablish);
+ close_remote_folder_async.begin(this, closing_remote_folder);
}
- remote_opened = false;
-
- // if remote reason is an error, then close_remote_folder_async() will be performing
- // reestablishment, so go no further
- if ((remote_reason.is_error() && closing_remote_folder != null) || force_reestablish)
+ // if reestablishing in close_remote_folder_async(), go no further
+ if (open_count > 0)
return;
// forced closed one way or another, so reset state
- open_count = 0;
+ open_flags = OpenFlags.NONE;
reestablish_delay_msec = DEFAULT_REESTABLISH_DELAY_MSEC;
// use remote_reason even if remote_folder was null; it could be that the error occurred
@@ -847,7 +850,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// See note in close_async() for why this method is static and uses an owned ref
private static async void close_remote_folder_async(owned MinimalFolder folder,
- owned Imap.Folder? remote_folder, Folder.CloseReason remote_reason, bool force_reestablish) {
+ owned Imap.Folder? remote_folder) {
// force the remote closed; if due to a remote disconnect and plan on reopening, *still*
// need to do this ... don't set remote_folder to null, as that will make some code paths
// think the folder is closing or closed when in fact it will be re-opening in a moment
@@ -860,31 +863,37 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// fallthrough
}
+ if (folder.open_count <= 0) {
+ debug("Not reestablishing connection to %s: closed", folder.to_string());
+
+ return;
+ }
+
// reestablish connection (which requires renormalizing the remote with the local) if
// close was in error
- if (remote_reason.is_error() || force_reestablish) {
- debug("Reestablishing broken connection to %s in %dms", folder.to_string(),
- folder.reestablish_delay_msec);
+ debug("Reestablishing broken connection to %s in %dms", folder.to_string(),
+ folder.reestablish_delay_msec);
+
+ yield Scheduler.sleep_ms_async(folder.reestablish_delay_msec);
+
+ if (folder.open_count <= 0) {
+ debug("Not reestablishing connection to %s: closed after delay", folder.to_string());
- yield Scheduler.sleep_ms_async(folder.reestablish_delay_msec);
+ return;
+ }
+
+ try {
+ // double timer now, reset to init value when cleanly opened
+ folder.reestablish_delay_msec = (folder.reestablish_delay_msec * 2).clamp(
+ DEFAULT_REESTABLISH_DELAY_MSEC, MAX_REESTABLISH_DELAY_MSEC);
- try {
- if (folder.open_count > 0) {
- // double now, reset to init value when cleanly opened
- folder.reestablish_delay_msec = (folder.reestablish_delay_msec * 2).clamp(
- DEFAULT_REESTABLISH_DELAY_MSEC, MAX_REESTABLISH_DELAY_MSEC);
-
- // since open_async() increments open_count, artificially decrement here to
- // prevent driving the value up
- folder.open_count--;
-
- yield folder.open_async(OpenFlags.NO_DELAY, null);
- } else {
- debug("%s: Not reestablishing broken connection, folder was closed", folder.to_string());
- }
- } catch (Error err) {
- debug("Error reestablishing broken connection to %s: %s", folder.to_string(), err.message);
- }
+ // since open_async() increments open_count, artificially decrement here to
+ // prevent driving the value up
+ folder.open_count = Numeric.int_floor(folder.open_count - 1, 0);
+
+ yield folder.open_async(OpenFlags.NO_DELAY, null);
+ } catch (Error err) {
+ debug("Error reestablishing broken connection to %s: %s", folder.to_string(), err.message);
}
}
@@ -1405,5 +1414,10 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
return ret;
}
+
+ public override string to_string() {
+ return "%s (open_count=%d remote_opened=%s)".printf(base.to_string(), open_count,
+ remote_opened.to_string());
+ }
}
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
b/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
index d277b9c..2a20990 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
@@ -35,10 +35,9 @@ private class Geary.ImapEngine.ReplayDisconnect : Geary.ImapEngine.ReplayOperati
// we want to encourage, so use the Idle queue to schedule close_internal_async
Idle.add(() => {
// ReplayDisconnect is only used when remote disconnects, so never flush pending, the
- // connection is down or going down, but always force reestablish connection, since
- // it wasn't our idea
+ // connection is down or going down
owner.close_internal_async.begin(Geary.Folder.CloseReason.LOCAL_CLOSE, remote_reason,
- true, false, null);
+ false, null);
return false;
});
diff --git a/src/engine/imap/transport/imap-client-session-manager.vala
b/src/engine/imap/transport/imap-client-session-manager.vala
index c821d64..9b1b363 100644
--- a/src/engine/imap/transport/imap-client-session-manager.vala
+++ b/src/engine/imap/transport/imap-client-session-manager.vala
@@ -5,8 +5,9 @@
*/
public class Geary.Imap.ClientSessionManager : BaseObject {
- public const int DEFAULT_MIN_POOL_SIZE = 1;
- private const int AUTHORIZED_SESSION_ERROR_RETRY_TIMEOUT_MSEC = 1000;
+ private const int DEFAULT_MIN_POOL_SIZE = 1;
+ private const int AUTHORIZED_SESSION_ERROR_MIN_RETRY_TIMEOUT_SEC = 1;
+ private const int AUTHORIZED_SESSION_ERROR_MAX_RETRY_TIMEOUT_SEC = 10;
public bool is_open { get; private set; default = false; }
@@ -52,6 +53,7 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
private bool authentication_failed = false;
private bool untrusted_host = false;
private uint authorized_session_error_retry_timeout_id = 0;
+ private int authorized_session_retry_sec = AUTHORIZED_SESSION_ERROR_MIN_RETRY_TIMEOUT_SEC;
public signal void login_failed();
@@ -170,12 +172,15 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
debug("Unable to create authorized session to %s: %s",
account_information.get_imap_endpoint().to_string(), err.message);
- // try again after a slight delay
+ // try again after a slight delay and bump up delay
if (authorized_session_error_retry_timeout_id != 0)
Source.remove(authorized_session_error_retry_timeout_id);
- authorized_session_error_retry_timeout_id
- = Timeout.add(AUTHORIZED_SESSION_ERROR_RETRY_TIMEOUT_MSEC,
- on_authorized_session_error_retry_timeout);
+
+ authorized_session_error_retry_timeout_id = Timeout.add_seconds(
+ authorized_session_retry_sec, on_authorized_session_error_retry_timeout);
+
+ authorized_session_retry_sec = (authorized_session_retry_sec * 2).clamp(
+ AUTHORIZED_SESSION_ERROR_MIN_RETRY_TIMEOUT_SEC,
AUTHORIZED_SESSION_ERROR_MAX_RETRY_TIMEOUT_SEC);
}
}
@@ -244,6 +249,9 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
throw err;
}
+ // reset delay
+ authorized_session_retry_sec = AUTHORIZED_SESSION_ERROR_MIN_RETRY_TIMEOUT_SEC;
+
// do this after logging in
new_session.enable_keepalives(selected_keepalive_sec, unselected_keepalive_sec,
selected_with_idle_keepalive_sec);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
