[geary/wip/789924-network-transition-redux: 62/64] Improve IMAP client session pool management.

[Michael Gratton <mjog src gnome org>]

commit 3944884269fd0c757fee7f9ff0e99bd4fb149117
Author: Michael James Gratton <mike vee net>
Date:   Tue Feb 27 02:42:42 2018 +1100

    Improve IMAP client session pool management.
    
    Close released client sessions when there are too many, and try harder to
    not give out bad sessions when one is being claimed.
    
    * src/engine/imap/transport/imap-client-session-manager.vala
      (ClientSessionManager): Add a maximum free session tunable and release
      any client sessions returned if it would otherwise exceed this
      threshold. When claiming a session, ensure that an older session is
      still good by sending a NOOP before returning it.

 .../transport/imap-client-session-manager.vala     |   72 +++++++++++++++++---
 src/engine/imap/transport/imap-client-session.vala |   26 ++++++--
 2 files changed, 81 insertions(+), 17 deletions(-)
---
diff --git a/src/engine/imap/transport/imap-client-session-manager.vala 
b/src/engine/imap/transport/imap-client-session-manager.vala
index 61d6fbc..dc00891 100644
--- a/src/engine/imap/transport/imap-client-session-manager.vala
+++ b/src/engine/imap/transport/imap-client-session-manager.vala
@@ -23,8 +23,10 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
 
 
     private const int DEFAULT_MIN_POOL_SIZE = 1;
+    private const int DEFAULT_MAX_FREE_SIZE = 1;
     private const int POOL_START_TIMEOUT_SEC = 1;
     private const int POOL_STOP_TIMEOUT_SEC = 3;
+    private const int CHECK_NOOP_THRESHOLD_SEC = 5;
 
 
     /** Determines if the manager has been opened. */
@@ -65,15 +67,33 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
     public uint selected_with_idle_keepalive_sec { get; set; default = 
ClientSession.DEFAULT_SELECTED_WITH_IDLE_KEEPALIVE_SEC; }
 
     /**
-     * ClientSessionManager attempts to maintain a minimum number of open sessions with the server
-     * so they're immediately ready for use.
+     * Specifies the minimum number of sessions to keep open.
      *
-     * Setting this does not immediately adjust the pool size in either direction.  Adjustment will
-     * happen as connections are needed or closed.
+     * The manager will attempt to keep at least this number of
+     * connections open at all times.
+     *
+     * Setting this does not immediately adjust the pool size in
+     * either direction.  Adjustment will happen as connections are
+     * needed or closed.
      */
     public int min_pool_size { get; set; default = DEFAULT_MIN_POOL_SIZE; }
 
     /**
+     * Specifies the maximum number of free sessions to keep open.
+     *
+     * If there are already this number of free sessions available,
+     * the manager will close any additional sessions that are
+     * released, instead of keeping them for re-use. However it will
+     * not close sessions if doing so would reduce the size of the
+     * pool below {@link min_pool_size}.
+     *
+     * Setting this does not immediately adjust the pool size in
+     * either direction.  Adjustment will happen as connections are
+     * needed or closed.
+     */
+    public int max_free_size { get; set; default = DEFAULT_MAX_FREE_SIZE; }
+
+    /**
      * Determines if returned sessions should be kept or discarded.
      */
     public bool discard_returned_sessions = false;
@@ -245,7 +265,7 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
 
             // Connection may have gone bad sitting in the queue, so
             // check it before using it
-            if (!(yield check_session(claimed, false))) {
+            if (!(yield check_session(claimed, true))) {
                 claimed = null;
             }
         }
@@ -261,9 +281,14 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         debug("[%s] Returning session with %d of %d free",
               this.id, this.free_queue.size, this.all_sessions.size);
 
-        if (!this.is_open || this.discard_returned_sessions) {
+        bool too_many_free = (
+            this.free_queue.size >= this.max_free_size &&
+            this.all_sessions.size > this.min_pool_size
+        );
+
+        if (!this.is_open || this.discard_returned_sessions || too_many_free) {
             yield force_disconnect(session);
-        } else if (yield check_session(session, true)) {
+        } else if (yield check_session(session, false)) {
             bool free = true;
             MailboxSpecifier? mailbox = null;
             ClientSession.ProtocolState proto = session.get_protocol_state(out mailbox);
@@ -347,7 +372,7 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
     }
 
     /** Determines if a session is valid, disposing of it if not. */
-    private async bool check_session(ClientSession target, bool allow_selected) {
+    private async bool check_session(ClientSession target, bool claiming) {
         bool valid = false;
         switch (target.get_protocol_state(null)) {
         case ClientSession.ProtocolState.AUTHORIZED:
@@ -357,10 +382,10 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
 
         case ClientSession.ProtocolState.SELECTED:
         case ClientSession.ProtocolState.SELECTING:
-            if (allow_selected) {
-                valid = true;
-            } else {
+            if (claiming) {
                 yield force_disconnect(target);
+            } else {
+                valid = true;
             }
             break;
 
@@ -379,6 +404,31 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
             break;
         }
 
+        // We now know if the session /thinks/ it is in a reasonable
+        // state, but if we're claiming a new session it *needs* to be
+        // good, and in particular we want to ensure the connection
+        // hasn't timed out or otherwise been dropped. So send a NOOP
+        // and wait for it to find out. Only do this if we haven't
+        // seen a response from the server in a little while, however.
+        //
+        // XXX This is problematic since the server may send untagged
+        // responses to the NOOP, but at least since it won't be in
+        // the Selected state, we won't lose notifications of new
+        // messages, etc, only folder status, which should eventually
+        // get picked up by UpdateRemoteFolders. :/
+        if (claiming &&
+            target.last_seen + (CHECK_NOOP_THRESHOLD_SEC * 1000000) < GLib.get_real_time()) {
+            try {
+                debug("Sending NOOP when claiming a session");
+                yield target.send_command_async(
+                    new NoopCommand(), this.pool_cancellable
+                );
+            } catch (Error err) {
+                debug("Error sending NOOP: %s", err.message);
+                valid = false;
+            }
+        }
+
         return valid;
     }
 
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index ff3b036..cf303e8 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -234,6 +234,14 @@ public class Geary.Imap.ClientSession : BaseObject {
         get { return this.capabilities.has_capability(Capabilities.IDLE); }
     }
 
+    /**
+     * Determines when the last successful command response was received.
+     *
+     * Returns the system wall clock time the last successful command
+     * response was received, in microseconds since the UNIX epoch.
+     */
+    public int64 last_seen = 0;
+
 
     // While the following inbox and namespace data should be server
     // specific, there is a small chance they will differ between
@@ -1740,11 +1748,13 @@ public class Geary.Imap.ClientSession : BaseObject {
         
         fsm.issue(Event.SEND_ERROR, null, null, err);
     }
-    
+
     private void on_received_status_response(StatusResponse status_response) {
+        this.last_seen = GLib.get_real_time();
+
         // reschedule keepalive (traffic seen on channel)
         schedule_keepalive();
-        
+
         // If a CAPABILITIES ResponseCode, decode and update capabilities ...
         // some servers do this to prevent a second round-trip
         ResponseCode? response_code = status_response.response_code;
@@ -1843,11 +1853,13 @@ public class Geary.Imap.ClientSession : BaseObject {
         
         server_data_received(server_data);
     }
-    
+
     private void on_received_server_data(ServerData server_data) {
+        this.last_seen = GLib.get_real_time();
+
         // reschedule keepalive (traffic seen on channel)
         schedule_keepalive();
-        
+
         // send ServerData to upper layers for processing and storage
         try {
             notify_received_data(server_data);
@@ -1856,12 +1868,14 @@ public class Geary.Imap.ClientSession : BaseObject {
                 ierr.message);
         }
     }
-    
+
     private void on_received_bytes(size_t bytes) {
+        this.last_seen = GLib.get_real_time();
+
         // reschedule keepalive
         schedule_keepalive();
     }
-    
+
     private void on_received_bad_response(RootParameters root, ImapError err) {
         debug("[%s] Received bad response %s: %s", to_string(), root.to_string(), err.message);
     }