[geary/wip/789924-network-transition-redux: 60/64] Tidy up IMAP folder session management.

[Michael Gratton <mjog src gnome org>]

commit 9070380fc8961b6d91116108147d135fa5ddb84f
Author: Michael James Gratton <mike vee net>
Date:   Tue Feb 27 01:17:35 2018 +1100

    Tidy up IMAP folder session management.
    
    * src/engine/imap-engine/imap-engine-generic-account.vala
      (GenericAccount): Don't attempt to claim two client sessions when
      opening a folder, rather claim one and use as both an account session
      and a folder session. Rename open_folder_session to
      claim_folder_session to be consistent with account session API, update
      call sites.
    
    * src/engine/imap-engine/imap-engine-minimal-folder.vala (MinimalFolder):
      Ensure the remote session is automatically re-established if
      appropriate on disconnect. Tidy up debug output and some comments.

 .../gmail/imap-engine-gmail-folder.vala            |    2 +-
 .../imap-engine/imap-engine-generic-account.vala   |   43 ++++++++++++++------
 .../imap-engine/imap-engine-minimal-folder.vala    |   32 +++++++++-----
 .../imap-engine-revokable-committed-move.vala      |    2 +-
 4 files changed, 52 insertions(+), 27 deletions(-)
---
diff --git a/src/engine/imap-engine/gmail/imap-engine-gmail-folder.vala 
b/src/engine/imap-engine/gmail/imap-engine-gmail-folder.vala
index fb2d925..c4ab545 100644
--- a/src/engine/imap-engine/gmail/imap-engine-gmail-folder.vala
+++ b/src/engine/imap-engine/gmail/imap-engine-gmail-folder.vala
@@ -70,7 +70,7 @@ private class Geary.ImapEngine.GmailFolder : MinimalFolder, FolderSupport.Archiv
         // separate connection and is not synchronized with the database, but also avoids a full
         // folder normalization, which can be a heavyweight operation
         GenericAccount account = (GenericAccount) folder.account;
-        Imap.FolderSession imap_trash = yield account.open_folder_session(
+        Imap.FolderSession imap_trash = yield account.claim_folder_session(
             trash.path, cancellable
         );
         try {
diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala 
b/src/engine/imap-engine/imap-engine-generic-account.vala
index 86acee9..48103ea 100644
--- a/src/engine/imap-engine/imap-engine-generic-account.vala
+++ b/src/engine/imap-engine/imap-engine-generic-account.vala
@@ -339,34 +339,51 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
     }
 
     /**
-     * Establishes a new IMAP folder session.
+     * Claims a new IMAP folder session from the pool.
      *
      * A new IMAP client session will be retrieved from the pool,
      * connecting if needed, and used for a new folder session. This
      * call will wait until the pool is ready to provide sessions. The
      * session must be returned via {@link release_folder_session}
      * after use.
+     *
+     * The account must have been opened before calling this method.
      */
-    public async Imap.FolderSession open_folder_session(Geary.FolderPath path,
-                                                        Cancellable cancellable)
+    public async Imap.FolderSession claim_folder_session(Geary.FolderPath path,
+                                                         Cancellable cancellable)
         throws Error {
         check_open();
-        debug("%s: Opening account session", this.to_string());
-        Imap.ClientSession? client = null;
+        debug("%s: Acquiring folder session", this.to_string());
+        yield this.remote_ready_lock.wait_async(cancellable);
+
+        // We manually construct an account session here and then
+        // reuse it for the folder session so we only need to claim as
+        // single session from the pool, not two.
+
+        Imap.ClientSession? client =
+            yield this.session_pool.claim_authorized_session_async(cancellable);
+        Imap.AccountSession account = new Imap.AccountSession(
+            this.information.id, client
+        );
+
         Imap.Folder? folder = null;
+        GLib.Error? folder_err = null;
         try {
-            // Do the claim_account_session first ensure the pool is
-            // ready.
-            Imap.AccountSession account = yield claim_account_session();
             folder = yield account.fetch_folder_async(path, cancellable);
-            client = yield this.session_pool.claim_authorized_session_async(
-                cancellable
-            );
         } catch (Error err) {
-            if (client != null) {
+            folder_err = err;
+        }
+
+        account.close();
+
+        if (folder_err != null) {
+            try {
                 yield this.session_pool.release_session_async(client);
+            } catch (Error release_err) {
+                debug("Error releasing folder session: %s", release_err.message);
             }
-            throw err;
+
+            throw folder_err;
         }
 
         return yield new Imap.FolderSession(
diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala 
b/src/engine/imap-engine/imap-engine-minimal-folder.vala
index 66cd17f..e7c2ced 100644
--- a/src/engine/imap-engine/imap-engine-minimal-folder.vala
+++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala
@@ -297,7 +297,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     public async Imap.FolderSession claim_remote_session(Cancellable? cancellable = null)
         throws Error {
         check_open("claim_remote_session");
-        debug("%s: Acquiring folder session", this.to_string());
+        debug("%s: Claiming folder session", this.to_string());
         yield this.wait_for_remote_async(cancellable);
         return this.remote_session;
     }
@@ -776,7 +776,6 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     private async void close_internal_locked(Folder.CloseReason local_reason,
                                              Folder.CloseReason remote_reason,
                                              Cancellable? cancellable) {
-        debug("%s: Closing", this.to_string());
         // Ensure we don't attempt to start opening a remote while
         // closing
         this._account.session_pool.ready.disconnect(on_remote_ready);
@@ -894,7 +893,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
 
         Imap.FolderSession? session = null;
         try {
-            session = yield this._account.open_folder_session(this.path, cancellable);
+            session = yield this._account.claim_folder_session(this.path, cancellable);
         } catch (Error err) {
             if (!(err is IOError.CANCELLED)) {
                 // Notify that there was a connection error, but don't
@@ -928,9 +927,10 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
         try {
             yield normalize_folders(session, cancellable);
         } catch (Error err) {
-            // Normalisation failed, which is also a serious problem
-            // so treat as in the error case above, after resolving if
-            // the issue was local or remote.
+            // Normalisation failed, so we have a pretty serious
+            // problem and should not try to use the folder further,
+            // unless the open was simply cancelled. So clean up, and
+            // force the folder closed.
             this._account.release_folder_session(session);
             if (!(err is IOError.CANCELLED)) {
                 Folder.CloseReason local_reason = CloseReason.LOCAL_ERROR;
@@ -960,10 +960,8 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
                 session.folder.properties, cancellable
             );
         } catch (Error err) {
-            // Database failed, so we have a pretty serious problem
-            // and should not try to use the folder further, unless
-            // the open was simply cancelled. So clean up, and force
-            // the folder closed if needed.
+            // Database failed, which is also a pretty serious
+            // problem, so handle as per above.
             this._account.release_folder_session(session);
             if (!(err is IOError.CANCELLED)) {
                 notify_open_failed(Folder.OpenFailed.LOCAL_ERROR, err);
@@ -999,7 +997,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
         // folder to open that the result of that operation is ready
         notify_remote_waiters(true);
 
-        // Update flags once the folder has opened. We will receive
+        // Update flags once the remote has opened. We will receive
         // notifications of changes as long as the session remains
         // open, so only need to do this once
         this.update_flags_timer.start();
@@ -1510,12 +1508,22 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     }
 
     private void on_remote_disconnected(Imap.ClientSession.DisconnectReason reason) {
+        bool is_error = reason.is_error();
+
         // Need to close the remote session immediately to avoid a
         // race with it opening again
-        Geary.Folder.CloseReason remote_reason = reason.is_error()
+        Geary.Folder.CloseReason remote_reason = is_error
             ? Geary.Folder.CloseReason.REMOTE_ERROR
             : Geary.Folder.CloseReason.REMOTE_CLOSE;
         close_remote_session(remote_reason);
+
+        // If an error occurred, but the folder is still open and so
+        // is the pool, try re-establishing the connection.
+        if (is_error &&
+            this._account.session_pool.is_ready &&
+            !this.open_cancellable.is_cancelled()) {
+            this.open_remote_session.begin();
+        }
     }
 
 }
diff --git a/src/engine/imap-engine/imap-engine-revokable-committed-move.vala 
b/src/engine/imap-engine/imap-engine-revokable-committed-move.vala
index daed441..a7ab922 100644
--- a/src/engine/imap-engine/imap-engine-revokable-committed-move.vala
+++ b/src/engine/imap-engine/imap-engine-revokable-committed-move.vala
@@ -28,7 +28,7 @@ private class Geary.ImapEngine.RevokableCommittedMove : Revokable {
         try {
             // use a detached folder to quickly open, issue command, and leave, without full
             // normalization that MinimalFolder requires
-            session = yield this.account.open_folder_session(destination, cancellable);
+            session = yield this.account.claim_folder_session(destination, cancellable);
             foreach (Imap.MessageSet msg_set in Imap.MessageSet.uid_sparse(destination_uids)) {
                 // don't use Cancellable to try to make operations atomic
                 yield session.copy_email_async(msg_set, source, null);