[geary/mjog/imap-connection-fixes: 23/34] Fix Geary.Imap.ClientService sometimes not closing sessions

[Michael Gratton <mjog src gnome org>]

commit 31260ab1e0411034d208c273b999ddc8a4ddb577
Author: Michael Gratton <mike vee net>
Date:   Mon Dec 30 15:11:15 2019 +1030

    Fix Geary.Imap.ClientService sometimes not closing sessions
    
    Update to ensure sessions are always closed and dropped as needed.

 src/engine/imap/api/imap-client-service.vala | 105 ++++++++++-----------------
 1 file changed, 40 insertions(+), 65 deletions(-)
---
diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala
index 1697ec73..7a8a7f84 100644
--- a/src/engine/imap/api/imap-client-service.vala
+++ b/src/engine/imap/api/imap-client-service.vala
@@ -222,14 +222,17 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
             this.all_sessions.size > this.min_pool_size
         );
 
-        if (!this.is_running || this.discard_returned_sessions || too_many_free) {
-            yield disconnect_session(session);
-        } else if (yield check_session(session, false)) {
-            bool free = true;
-            MailboxSpecifier? mailbox = null;
-            ClientSession.ProtocolState proto = session.get_protocol_state();
+        bool disconnect = (
+            too_many_free ||
+            this.discard_returned_sessions ||
+            !this.is_running ||
+            !yield check_session(session, false)
+        );
+
+        if (!disconnect) {
             // If the session has a mailbox selected, close it before
             // adding it back to the pool
+            ClientSession.ProtocolState proto = session.get_protocol_state();
             if (proto == ClientSession.ProtocolState.SELECTED ||
                 proto == ClientSession.ProtocolState.SELECTING) {
                 // always close mailbox to return to authorized state
@@ -238,32 +241,20 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
                 } catch (ImapError imap_error) {
                     debug("Error attempting to close released session %s: %s",
                           session.to_string(), imap_error.message);
-                    free = false;
+                    disconnect = true;
                 }
-
-                // Double check the session after closing it
-                switch (session.get_protocol_state()) {
-                case AUTHORIZED:
-                    // This is the desired state, so all good
-                    break;
-
-                case NOT_CONNECTED:
-                    // No longer connected, so just drop it
-                    free = false;
-                    break;
-
-                default:
+                if (session.get_protocol_state() != AUTHORIZED) {
                     // Closing it didn't leave it in the desired
-                    // state, so log out and drop it
-                    yield disconnect_session(session);
-                    free = false;
-                    break;
+                    // state, so drop it
+                    disconnect = true;
                 }
             }
 
-            if (free) {
+            if (!disconnect) {
                 debug("Unreserving session %s", session.to_string());
                 this.free_queue.send(session);
+            } else {
+                yield disconnect_session(session);
             }
         }
     }
@@ -396,15 +387,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
             }
             break;
 
-        case ClientSession.ProtocolState.NOT_CONNECTED:
-            // Already disconnected, so drop it on the floor
-            try {
-                yield remove_session_async(target);
-            } catch (Error err) {
-                debug("Error removing unconnected session: %s", err.message);
-            }
-            break;
-
         default:
             yield disconnect_session(target);
             break;
@@ -506,44 +488,43 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
     }
 
     private async void disconnect_session(ClientSession session) {
-        debug("Logging out session: %s", session.to_string());
-
-        // Log out before removing the session since close() only
-        // hangs around until all sessions have been removed before
-        // exiting.
-        try {
-            yield session.logout_async(this.close_cancellable);
+        if (session.get_protocol_state() != NOT_CONNECTED) {
+            debug("Logging out session: %s", session.to_string());
+            // No need to remove it after logging out, the
+            // disconnected handler will do that for us.
+            try {
+                yield session.logout_async(this.close_cancellable);
+            } catch (GLib.Error err) {
+                debug("Error logging out of session: %s", err.message);
+                yield force_disconnect_session(session);
+                }
+        } else {
             yield remove_session_async(session);
-        } catch (GLib.Error err) {
-            debug("Error logging out of session: %s", err.message);
-            yield force_disconnect_session(session);
         }
-
     }
 
     private async void force_disconnect_session(ClientSession session) {
         debug("Dropping session: %s", session.to_string());
-
-        try {
-            yield remove_session_async(session);
-        } catch (Error err) {
-            debug("Error removing session: %s", err.message);
-        }
+        yield remove_session_async(session);
 
         // Don't wait for this to finish because we don't want to
         // block claiming a new session, shutdown, etc.
         session.disconnect_async.begin(null);
     }
 
-    private async bool remove_session_async(ClientSession session) throws Error {
+    private async bool remove_session_async(ClientSession session) {
         // Ensure the session isn't held on to, anywhere
 
         this.free_queue.revoke(session);
 
         bool removed = false;
-        yield this.sessions_mutex.execute_locked(() => {
-                removed = this.all_sessions.remove(session);
-            });
+        try {
+            yield this.sessions_mutex.execute_locked(() => {
+                    removed = this.all_sessions.remove(session);
+                });
+        } catch (GLib.Error err) {
+            debug("Error removing session: %s", err.message);
+        }
 
         if (removed) {
             session.disconnected.disconnect(on_disconnected);
@@ -551,21 +532,15 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
         return removed;
     }
 
-    private void on_disconnected(ClientSession session, ClientSession.DisconnectReason reason) {
+    private void on_disconnected(ClientSession session,
+                                 ClientSession.DisconnectReason reason) {
         debug(
-            "Session unexpected disconnect: %s: %s",
+            "Session disconnected: %s: %s",
             session.to_string(), reason.to_string()
         );
         this.remove_session_async.begin(
             session,
-            (obj, res) => {
-                try {
-                    this.remove_session_async.end(res);
-                } catch (Error err) {
-                    debug("Error removing disconnected session: %s",
-                          err.message);
-                }
-            }
+            (obj, res) => { this.remove_session_async.end(res); }
         );
     }