[geary] Don't add IMAP connections back to the pool when the account is closing.

[Michael Gratton <mjog src gnome org>]

commit 07bc4410855f243e8b41ffbc5da515aa056b48b6
Author: Michael James Gratton <mike vee net>
Date:   Wed Jan 17 15:23:56 2018 +1100

    Don't add IMAP connections back to the pool when the account is closing.
    
    This updates the client session manager to close IMAP connections without
    waiting when they are released and the account is closing, rather than
    adding them back to the pool again. Sessions that are in the Selected
    state due to being used by a folder are also simply closed, rather than
    attempting to return them to Authenticated (i.e. not selected) state
    first.
    
    This means when a folder is closed and releases its session that it does
    not block if its session has hung, making application shutdown less
    likely to hang as well, helping Bug 745561 along a bit further.

 .../imap-engine/imap-engine-generic-account.vala   |    1 +
 src/engine/imap/api/imap-account.vala              |   35 +++++-
 .../transport/imap-client-session-manager.vala     |  133 ++++++++++---------
 3 files changed, 100 insertions(+), 69 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala 
b/src/engine/imap-engine/imap-engine-generic-account.vala
index 7ed3612..38bd71d 100644
--- a/src/engine/imap-engine/imap-engine-generic-account.vala
+++ b/src/engine/imap-engine/imap-engine-generic-account.vala
@@ -221,6 +221,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
         if (!open)
             return;
 
+        this.remote.prepare_to_close();
         this.remote.ready.disconnect(on_remote_ready);
 
         // Halt internal tasks early so they stop using local and
diff --git a/src/engine/imap/api/imap-account.vala b/src/engine/imap/api/imap-account.vala
index d862ad4..7e77b0a 100644
--- a/src/engine/imap/api/imap-account.vala
+++ b/src/engine/imap/api/imap-account.vala
@@ -1,5 +1,6 @@
 /*
  * Copyright 2016 Software Freedom Conservancy Inc.
+ * Copyright 2018 Michael Gratton <mike vee net>.
  *
  * This software is licensed under the GNU Lesser General Public License
  * (version 2.1 or later).  See the COPYING file in this distribution.
@@ -8,16 +9,22 @@
 /**
  * An interface between the high-level engine API and the IMAP stack.
  *
- * Because of the complexities of the IMAP protocol, this private
- * class takes common operations that a Geary.Account implementation
- * would need (in particular, {@link Geary.ImapEngine.GenericAccount}
- * and makes them into simple async calls.
+ * Because of the complexities of the IMAP protocol, class takes
+ * common operations that a Geary.Account implementation would need
+ * (in particular, {@link Geary.ImapEngine.GenericAccount}) and makes
+ * them into simple async calls.
+ *
+ * This class maintains an {@link ClientSessionManager} instance to
+ * maintain a pool of connections to an account's IMAP endpoint. On
+ * opening, it will open the pool, then claim an IMAP session and
+ * maintain it in a non-selected state for executing
+ * non-mailbox-specific operations.
  *
  * Geary.Imap.Account manages the {@link Imap.Folder} objects it
  * returns, but only in the sense that it will not create new
  * instances repeatedly.  Otherwise, it does not refresh or update the
  * Imap.Folders themselves (such as update their {@link
- * Imap.StatusData} periodically).  That's the responsibility of the
+ * Imap.StatusData} periodically). That's the responsibility of the
  * higher layers of the stack.
  */
 private class Geary.Imap.Account : BaseObject {
@@ -66,6 +73,12 @@ private class Geary.Imap.Account : BaseObject {
         this.session_mgr.login_failed.connect(on_login_failed);
     }
 
+    /**
+     * Prepares the account for use.
+     *
+     * Opening the account will kick off at establishing least one
+     * connection to the IMAP server, if accessible.
+     */
     public async void open_async(Cancellable? cancellable = null) throws Error {
         if (is_open)
             throw new EngineError.ALREADY_OPEN("Imap.Account already open");
@@ -80,7 +93,17 @@ private class Geary.Imap.Account : BaseObject {
 
         is_open = true;
     }
-    
+
+    /**
+     * Notifies the account that the engine is preparing to exit.
+     */
+    public void prepare_to_close() {
+        this.session_mgr.discard_returned_sessions = true;
+    }
+
+    /**
+     * Closes the account, releasing its IMAP session and session pool.
+     */
     public async void close_async(Cancellable? cancellable = null) throws Error {
         if (!is_open)
             return;
diff --git a/src/engine/imap/transport/imap-client-session-manager.vala 
b/src/engine/imap/transport/imap-client-session-manager.vala
index 302b914..4734db9 100644
--- a/src/engine/imap/transport/imap-client-session-manager.vala
+++ b/src/engine/imap/transport/imap-client-session-manager.vala
@@ -58,6 +58,11 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
      */
     public int min_pool_size { get; set; default = DEFAULT_MIN_POOL_SIZE; }
 
+    /**
+     * Determines if returned sessions should be kept or discarded.
+     */
+    public bool discard_returned_sessions = false;
+
     private AccountInformation account_information;
     private Endpoint endpoint;
     private Gee.HashSet<ClientSession> sessions = new Gee.HashSet<ClientSession>();
@@ -345,79 +350,84 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         
         return found_session;
     }
-    
+
     public async void release_session_async(ClientSession session, Cancellable? cancellable)
         throws Error {
-        // Don't check_open(), it's valid for this to be called when is_open is false, that happens
-        // during mop-up
-        
-        MailboxSpecifier? mailbox;
+        // Don't check_open(), it's valid for this to be called when
+        // is_open is false, that happens during mop-up
+        MailboxSpecifier? mailbox = null;
         ClientSession.ProtocolState context = session.get_protocol_state(out mailbox);
-        
-        bool unreserve = false;
-        switch (context) {
+
+        if (context == ClientSession.ProtocolState.UNCONNECTED) {
+            // Already disconnected, so drop it on the floor
+            try {
+                yield unlocked_remove_session_async(session);
+            } catch (Error err) {
+                debug("[%s] Error removing unconnected session: %s",
+                      to_string(), err.message);
+            }
+        } else if (this.is_open && !this.discard_returned_sessions) {
+            bool free = false;
+            switch (context) {
             case ClientSession.ProtocolState.AUTHORIZED:
             case ClientSession.ProtocolState.CLOSING_MAILBOX:
-                // keep as-is, but remove from the reserved list
-                unreserve = true;
-            break;
-            
-            // ClientSessionManager is tasked with holding onto a pool of authorized connections,
-            // so if one is released outside that state, pessimistically drop it
-            case ClientSession.ProtocolState.CONNECTING:
-            case ClientSession.ProtocolState.AUTHORIZING:
-            case ClientSession.ProtocolState.UNAUTHORIZED:
-                yield force_disconnect(session, true);
-            break;
-            
-            case ClientSession.ProtocolState.UNCONNECTED:
-                yield force_disconnect(session, false);
-            break;
-            
+                // keep as-is, but add back to the free list
+                free = true;
+                break;
+
             case ClientSession.ProtocolState.SELECTED:
             case ClientSession.ProtocolState.SELECTING:
-                debug("[%s] Closing mailbox for released session %s", to_string(), session.to_string());
-                
+                debug("[%s] Closing %s for released session %s",
+                      to_string(),
+                      mailbox != null ? mailbox.to_string() : "(unknown)",
+                      session.to_string());
+
                 // always close mailbox to return to authorized state
                 try {
                     yield session.close_mailbox_async(cancellable);
                 } catch (ImapError imap_error) {
-                    debug("Error attempting to close released session %s: %s", session.to_string(),
-                        imap_error.message);
+                    debug("[%s] Error attempting to close released session %s: %s",
+                          to_string(), session.to_string(), imap_error.message);
                 }
-                
-                // if not in authorized state now, drop it, otherwise remove from reserved list
-                if (session.get_protocol_state(out mailbox) == ClientSession.ProtocolState.AUTHORIZED)
-                    unreserve = true;
-                else
+
+                if (session.get_protocol_state(out mailbox) == ClientSession.ProtocolState.AUTHORIZED) {
+                    // Now in authorized state, free it up for re-use
+                    free = true;
+                } else {
+                    // Closing it didn't work, so drop it
                     yield force_disconnect(session, true);
-            break;
-            
+                }
+                break;
+
             default:
-                assert_not_reached();
-        }
-        
-        if (!unreserve)
-            return;
-        
-        // if not open, disconnect, which will remove from the reserved pool anyway
-        if (!is_open) {
-            yield force_disconnect(session, true);
-        } else {
-            debug("[%s] Unreserving session %s", to_string(), session.to_string());
-            
-            try {
-                // don't respect Cancellable because this *must* happen; don't want this lingering
-                // on the reserved list forever
-                int token = yield sessions_mutex.claim_async();
-                
-                bool removed = reserved_sessions.remove(session);
-                assert(removed);
-                
-                sessions_mutex.release(ref token);
-            } catch (Error err) {
-                message("Unable to remove %s from reserved list: %s", session.to_string(), err.message);
+                // This class is tasked with holding onto a pool of
+                // authorized connections, so if one is released
+                // outside that state, pessimistically drop it
+                yield force_disconnect(session, true);
+                break;
             }
+
+            if (free) {
+                debug("[%s] Unreserving session %s",
+                      to_string(), session.to_string());
+                try {
+                    int token = yield sessions_mutex.claim_async(cancellable);
+                    this.reserved_sessions.remove(session);
+                    this.sessions_mutex.release(ref token);
+                } catch (Error err) {
+                    message("[%s] Unable to add %s to the free list: %s",
+                            to_string(), session.to_string(), err.message);
+                }
+            }
+        } else {
+            // Not open, or we are discarding sessions, so just close it.
+            yield force_disconnect(session, true);
+        }
+
+        // If we're discarding returned sessions, we don't want to
+        // create any more, so only twiddle the pool if not.
+        if (!this.discard_returned_sessions) {
+            this.adjust_session_pool.begin();
         }
     }
 
@@ -466,12 +476,9 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         }
 
         locked_remove_session(session);
+
         if (do_disconnect) {
-            try {
-                yield session.disconnect_async();
-            } catch (Error err) {
-                // ignored
-            }
+            session.disconnect_async.begin();
         }
 
         try {