From eea2b31a73f8130dae3ac7bf1891a56685e4e1c4 Mon Sep 17 00:00:00 2001 From: Jim Nelson Date: Fri, 6 Jan 2012 11:32:20 -0800 Subject: [PATCH] Smarter keepalives and a small Conversations fix. The NOOP keepalives were being every n seconds no matter what the traffic was like. Now only being sent if no traffic has been received after n seconds, meaning less traffic overall. Also, there are now two different timers, one for selected mailbox and one for authorized state, since receiving unsolicited responses only happens when a mailbox is selected. Also includes a small fix in Conversations where a nullable ref was being treated as un-nullable. --- src/engine/api/geary-conversations.vala | 3 +- .../imap/decoders/imap-noop-results.vala | 70 ------------ .../imap-client-session-manager.vala | 20 ++-- .../imap/transport/imap-client-session.vala | 108 ++++++++++++------ src/engine/util/util-numeric.vala | 18 +++ src/wscript | 2 +- 6 files changed, 99 insertions(+), 122 deletions(-) delete mode 100644 src/engine/imap/decoders/imap-noop-results.vala create mode 100755 src/engine/util/util-numeric.vala diff --git a/src/engine/api/geary-conversations.vala b/src/engine/api/geary-conversations.vala index 682ebcbc..79f05bfc 100644 --- a/src/engine/api/geary-conversations.vala +++ b/src/engine/api/geary-conversations.vala @@ -644,7 +644,8 @@ public class Geary.Conversations : Object { if (conversation.get_usable_count() == 0) { // prune all Nodes in the conversation tree - prune_nodes((Node) conversation.get_origin()); + if (conversation.get_origin() != null) + prune_nodes((Node) conversation.get_origin()); // prune all orphan Nodes Gee.Collection? orphans = conversation.get_orphans(); diff --git a/src/engine/imap/decoders/imap-noop-results.vala b/src/engine/imap/decoders/imap-noop-results.vala deleted file mode 100644 index 01033ad5..00000000 --- a/src/engine/imap/decoders/imap-noop-results.vala +++ /dev/null @@ -1,70 +0,0 @@ -/* Copyright 2011 Yorba Foundation - * - * This software is licensed under the GNU Lesser General Public License - * (version 2.1 or later). See the COPYING file in this distribution. - */ - -public class Geary.Imap.NoopResults : Geary.Imap.CommandResults { - public Gee.List? expunged { get; private set; } - /** - * -1 if "exists" result not returned by server. - */ - public int exists { get; private set; } - public MailboxAttributes? flags { get; private set; } - /** - * -1 if "recent" result not returned by server. - */ - public int recent { get; private set; } - - public NoopResults(StatusResponse status_response, Gee.List? expunged, int exists, - MailboxAttributes? flags, int recent) { - base (status_response); - - this.expunged = expunged; - this.exists = exists; - this.flags = flags; - this.recent = recent; - } - - public static NoopResults decode(CommandResponse response) { - assert(response.is_sealed()); - - Gee.List expunged = new Gee.ArrayList(); - MailboxAttributes? flags = null; - int exists = -1; - int recent = -1; - - foreach (ServerData data in response.server_data) { - UnsolicitedServerData? unsolicited = UnsolicitedServerData.from_server_data(data); - if (unsolicited == null) { - message("NOOP server data \"%s\" unrecognized", data.to_string()); - - continue; - } - - if (unsolicited.exists >= 0) - exists = unsolicited.exists; - - if (unsolicited.recent >= 0) - recent = unsolicited.recent; - - if (unsolicited.flags != null) - flags = unsolicited.flags; - - if (unsolicited.expunge != null) - expunged.add(unsolicited.expunge); - } - - return new NoopResults(response.status_response, (expunged.size > 0) ? expunged : null, - exists, (flags != null && flags.size > 0) ? flags : null, recent); - } - - public bool has_exists() { - return exists >= 0; - } - - public bool has_recent() { - return recent >= 0; - } -} - diff --git a/src/engine/imap/transport/imap-client-session-manager.vala b/src/engine/imap/transport/imap-client-session-manager.vala index 165b2315..0c2808b8 100644 --- a/src/engine/imap/transport/imap-client-session-manager.vala +++ b/src/engine/imap/transport/imap-client-session-manager.vala @@ -6,7 +6,6 @@ public class Geary.Imap.ClientSessionManager { private const int MIN_POOL_SIZE = 2; - private const int SELECTED_KEEPALIVE_SEC = 30; private Endpoint endpoint; private Credentials credentials; @@ -14,8 +13,8 @@ public class Geary.Imap.ClientSessionManager { private Geary.NonblockingMutex sessions_mutex = new Geary.NonblockingMutex(); private Gee.HashSet examined_contexts = new Gee.HashSet(); private Gee.HashSet selected_contexts = new Gee.HashSet(); - private int keepalive_sec = ClientSession.DEFAULT_KEEPALIVE_SEC; - private int selected_keepalive_sec = SELECTED_KEEPALIVE_SEC; + private int unselected_keepalive_sec = ClientSession.DEFAULT_UNSELECTED_KEEPALIVE_SEC; + private int selected_keepalive_sec = ClientSession.DEFAULT_SELECTED_KEEPALIVE_SEC; public signal void login_failed(); @@ -39,14 +38,15 @@ public class Geary.Imap.ClientSessionManager { } /** - * Set to zero or negative value if keepalives should be disabled. (This is not recommended.) + * Set to zero or negative value if keepalives should be disabled when a connection has not + * selected a mailbox. (This is not recommended.) * * This only affects newly created sessions or sessions leaving the selected/examined state * and returning to an authorized state. */ - public void set_keepalive(int keepalive_sec) { + public void set_unselected_keepalive(int unselected_keepalive_sec) { // set for future connections - this.keepalive_sec = keepalive_sec; + this.unselected_keepalive_sec = unselected_keepalive_sec; } /** @@ -179,10 +179,8 @@ public class Geary.Imap.ClientSessionManager { bool removed = contexts.remove(context); assert(removed); - if (context.session != null) { + if (context.session != null) context.session.close_mailbox_async.begin(); - context.session.enable_keepalives(keepalive_sec); - } } // This should only be called when sessions_mutex is locked. @@ -195,7 +193,7 @@ public class Geary.Imap.ClientSessionManager { yield new_session.login_async(credentials, cancellable); // do this after logging in - new_session.enable_keepalives(keepalive_sec); + new_session.enable_keepalives(selected_keepalive_sec, unselected_keepalive_sec); // since "disconnected" is used to remove the ClientSession from the sessions list, want // to only connect to the signal once the object has been added to the list; otherwise it's @@ -241,8 +239,6 @@ public class Geary.Imap.ClientSessionManager { ClientSession authd = yield get_authorized_session(cancellable); - authd.enable_keepalives(selected_keepalive_sec); - results = yield authd.select_examine_async(folder, is_select, cancellable); return authd; diff --git a/src/engine/imap/transport/imap-client-session.vala b/src/engine/imap/transport/imap-client-session.vala index c8fd2c20..2a3c4b73 100644 --- a/src/engine/imap/transport/imap-client-session.vala +++ b/src/engine/imap/transport/imap-client-session.vala @@ -5,9 +5,13 @@ */ public class Geary.Imap.ClientSession { - // 30 min keepalive required to maintain session; back off by 5 min for breathing room - public const int MIN_KEEPALIVE_SEC = 25 * 60; - public const int DEFAULT_KEEPALIVE_SEC = 3 * 60; + // 30 min keepalive required to maintain session; back off by 1 min for breathing room + public const int MIN_KEEPALIVE_SEC = 29 * 60; + + // NOOP is only sent after this amount of time has passed since the last received + // message on the connection dependant on connection state (selected/examined vs. authorized) + public const int DEFAULT_SELECTED_KEEPALIVE_SEC = 45; + public const int DEFAULT_UNSELECTED_KEEPALIVE_SEC = MIN_KEEPALIVE_SEC; public enum Context { UNCONNECTED, @@ -172,6 +176,8 @@ public class Geary.Imap.ClientSession { Hashable.hash_func, Equalable.equal_func); private CommandResponse current_cmd_response = new CommandResponse(); private uint keepalive_id = 0; + private int selected_keepalive_secs = 0; + private int unselected_keepalive_secs = 0; // state used only during connect and disconnect private bool awaiting_connect_response = false; @@ -528,17 +534,13 @@ public class Geary.Imap.ClientSession { * Although keepalives can be enabled at any time, if they're enabled and trigger sending * a command prior to connection, error signals may be fired. */ - public void enable_keepalives(int seconds = DEFAULT_KEEPALIVE_SEC) { - if (seconds <= 0) { - disable_keepalives(); - - return; - } + public void enable_keepalives(int seconds_while_selected = DEFAULT_SELECTED_KEEPALIVE_SEC, + int seconds_while_unselected = DEFAULT_UNSELECTED_KEEPALIVE_SEC) { + selected_keepalive_secs = seconds_while_selected; + unselected_keepalive_secs = seconds_while_unselected; - if (keepalive_id != 0) - Source.remove(keepalive_id); - - keepalive_id = Timeout.add_seconds(seconds, on_keepalive); + // schedule one now, although will be rescheduled if traffic is received before it fires + schedule_keepalive(); } /** @@ -554,41 +556,59 @@ public class Geary.Imap.ClientSession { return true; } + private void schedule_keepalive() { + // if old one was scheduled, unschedule and schedule anew + if (keepalive_id != 0) + Source.remove(keepalive_id); + + int seconds; + switch (get_context(null)) { + case Context.UNCONNECTED: + return; + + case Context.IN_PROGRESS: + case Context.EXAMINED: + case Context.SELECTED: + seconds = selected_keepalive_secs; + break; + + case Context.UNAUTHORIZED: + case Context.AUTHORIZED: + default: + seconds = unselected_keepalive_secs; + break; + } + + // Possible to not have keepalives in one state but in another, or for neither + // + // Yes, we allow keepalive to be set to 1 second. It's their dime. + if (seconds > 0) + keepalive_id = Timeout.add_seconds(seconds, on_keepalive); + } + private bool on_keepalive() { send_command_async.begin(new NoopCommand(), null, on_keepalive_completed); - return true; + // Reschedule to reflect current connection state, although will be rescheduled again if + // traffic is received + keepalive_id = 0; + schedule_keepalive(); + + return false; } private void on_keepalive_completed(Object? source, AsyncResult result) { - NoopResults results; + CommandResponse response; try { - results = NoopResults.decode(send_command_async.end(result)); + response = send_command_async.end(result); } catch (Error err) { - message("Keepalive error: %s", err.message); + debug("Keepalive error: %s", err.message); return; } - if (results.status_response.status != Status.OK) { - debug("Keepalive failed: %s", results.status_response.to_string()); - - return; - } - - if (results.expunged != null) { - foreach (MessageNumber msg in results.expunged) - unsolicited_expunged(msg); - } - - if (results.has_exists()) - unsolicited_exists(results.exists); - - if (results.has_recent()) - unsolicited_recent(results.recent); - - if (results.flags != null) - unsolicited_flags(results.flags); + if (response.status_response.status != Status.OK) + debug("Keepalive failed: %s", response.status_response.to_string()); } // @@ -616,9 +636,12 @@ public class Geary.Imap.ClientSession { if (params.err != null) throw params.err; - // look for unsolicited server data and signal all that are found ... since SELECT/EXAMINE + // Look for unsolicited server data and signal all that are found ... since SELECT/EXAMINE // aren't allowed here, don't need to check for them (because their fields aren't considered - // unsolicited) + // unsolicited). Note that this also captures NOOP's responses; although not exactly + // unsolicited in one sense, the NOOP command is merely the transport vehicle to allow the + // server to deliver unsolicited messages when no other response is available to piggyback + // upon. // // Note that EXPUNGE returns *EXPUNGED* results, not *EXPUNGE*, which is what the unsolicited // version is. @@ -1093,6 +1116,9 @@ public class Geary.Imap.ClientSession { } private void on_received_status_response(StatusResponse status_response) { + // reschedule keepalive, now that traffic has been seen + schedule_keepalive(); + assert(!current_cmd_response.is_sealed()); current_cmd_response.seal(status_response); assert(current_cmd_response.is_sealed()); @@ -1114,6 +1140,9 @@ public class Geary.Imap.ClientSession { } private void on_received_server_data(ServerData server_data) { + // reschedule keepalive, now that traffic has been seen + schedule_keepalive(); + // The first response from the server is an untagged status response, which is considered // ServerData in our model. This captures that and treats it as such. if (awaiting_connect_response) { @@ -1129,6 +1158,9 @@ public class Geary.Imap.ClientSession { } private void on_received_bad_response(RootParameters root, ImapError err) { + // reschedule keepalive, now that traffic has been seen + schedule_keepalive(); + debug("Received bad response %s: %s", root.to_string(), err.message); } diff --git a/src/engine/util/util-numeric.vala b/src/engine/util/util-numeric.vala new file mode 100755 index 00000000..9ce56e92 --- /dev/null +++ b/src/engine/util/util-numeric.vala @@ -0,0 +1,18 @@ +/* Copyright 2012 Yorba Foundation + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +namespace Geary.Numeric { + +public inline int int_floor(int value, int floor) { + return (value >= floor) ? value : floor; +} + +public inline int int_ceiling(int value, int ceiling) { + return (value <= ceiling) ? value : ceiling; +} + +} + diff --git a/src/wscript b/src/wscript index ebbf1050..b8b7ffce 100644 --- a/src/wscript +++ b/src/wscript @@ -53,7 +53,6 @@ def build(bld): '../engine/imap/decoders/imap-fetch-data-decoder.vala', '../engine/imap/decoders/imap-fetch-results.vala', '../engine/imap/decoders/imap-list-results.vala', - '../engine/imap/decoders/imap-noop-results.vala', '../engine/imap/decoders/imap-select-examine-results.vala', '../engine/imap/decoders/imap-status-results.vala', '../engine/imap/imap-error.vala', @@ -146,6 +145,7 @@ def build(bld): '../engine/util/util-inet.vala', '../engine/util/util-interfaces.vala', '../engine/util/util-memory.vala', + '../engine/util/util-numeric.vala', '../engine/util/util-reference-semantics.vala', '../engine/util/util-scheduler.vala', '../engine/util/util-stream.vala',