[geary/wip/789924-network-transition-redux: 2/2] Don't let a task try to re-open a folder while it is being closed.



commit 675d7d3a9acb6263d0b8550511198c5ec4b3d983
Author: Michael James Gratton <mike vee net>
Date:   Sat Mar 3 11:06:35 2018 +1100

    Don't let a task try to re-open a folder while it is being closed.
    
    Really (for sure this time) ensure folder opening/closing is not racy by
    using the same mutex to guard the complete opening and closing processes.
    
    * src/engine/imap-engine/imap-engine-minimal-folder.vala (MinimalFolder):
      Rename open_mutex to remote_mutex since it's only actually used when
      opening a remote session. Rename close_mutex to lifecycle_mutex and
      ensure that has always been claimed when manipulating the
      open_count. In particular, ensure that all of open_async is executed
      under that mutex and ensure that both decrementing the close count, and
      the complete actual closing process is always protected by it as well.

 .../imap-engine/imap-engine-minimal-folder.vala    |  273 ++++++++++++--------
 .../replay-ops/imap-engine-user-close.vala         |   19 +-
 2 files changed, 173 insertions(+), 119 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala 
b/src/engine/imap-engine/imap-engine-minimal-folder.vala
index ec5a371..3808642 100644
--- a/src/engine/imap-engine/imap-engine-minimal-folder.vala
+++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala
@@ -90,11 +90,11 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     private int open_count = 0;
     private Folder.OpenFlags open_flags = OpenFlags.NONE;
     private Cancellable? open_cancellable = null;
-    private Nonblocking.Mutex open_mutex = new Nonblocking.Mutex();
-    private Nonblocking.Mutex close_mutex = new Nonblocking.Mutex();
+    private Nonblocking.Mutex lifecycle_mutex = new Nonblocking.Mutex();
     private Nonblocking.Semaphore closed_semaphore = new Nonblocking.Semaphore();
 
     private Imap.FolderSession? remote_session = null;
+    private Nonblocking.Mutex remote_mutex = new Nonblocking.Mutex();
     private Nonblocking.ReportingSemaphore<bool> remote_wait_semaphore =
         new Nonblocking.ReportingSemaphore<bool>(false);
     private TimeoutManager remote_open_timer;
@@ -161,7 +161,6 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     ~MinimalFolder() {
         if (open_count > 0)
             warning("Folder %s destroyed without closing", to_string());
-        this.local_folder.email_complete.disconnect(on_email_complete);
     }
 
     protected virtual void notify_closing(Gee.List<ReplayOperation> final_ops) {
@@ -200,6 +199,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
             notify_special_folder_type_changed(old_type, new_type);
     }
 
+    /** {@inheritDoc} */
     public override Geary.Folder.OpenState get_open_state() {
         if (this.open_count == 0)
             return Geary.Folder.OpenState.CLOSED;
@@ -210,61 +210,30 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     }
 
     /** {@inheritDoc} */
-    public override async bool open_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable = null)
+    public override async bool open_async(Folder.OpenFlags open_flags,
+                                          Cancellable? cancellable = null)
         throws Error {
-        if (open_count++ > 0) {
-            // even if opened or opening, or if forcing a re-open, respect the NO_DELAY flag
-            if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
-                // add NO_DELAY flag if it forces an open
-                if (this.remote_session == null)
-                    this.open_flags |= OpenFlags.NO_DELAY;
-
-                this.open_remote_session.begin();
+        // Claim the lifecycle_mutex here so we don't try to re-open when
+        // the folder is in the middle of being closed.
+        bool opening = false;
+        Error? open_err = null;
+        try {
+            int token = yield this.lifecycle_mutex.claim_async(cancellable);
+            try {
+                opening = yield open_locked(open_flags, cancellable);
+            } catch (Error err) {
+                open_err = err;
             }
-            return false;
+            this.lifecycle_mutex.release(ref token);
+        } catch (Error err) {
+            // oh well
         }
 
-        // first open gets to name the flags, but see note above
-        this.open_flags = open_flags;
-
-        // reset to force waiting in wait_for_close_async()
-        this.closed_semaphore.reset();
-
-        // reset unseen count refresh since it will be updated when
-        // the remote opens
-        this.refresh_unseen_timer.reset();
-
-        // Construct objects needed when open
-        this.open_cancellable = new Cancellable();
-        this.replay_queue = new ReplayQueue(this);
-
-        // Notify the email prefetcher
-        this.email_prefetcher.open();
-
-        // notify about the local open
-        notify_opened(
-            Geary.Folder.OpenState.LOCAL,
-            this.local_folder.get_properties().email_total
-        );
-
-        // Unless NO_DELAY is set, do NOT open the remote side here; wait for the ReplayQueue to
-        // require a remote connection or wait_for_remote_async() to be called ... this allows for
-        // fast local-only operations to occur, local-only either because (a) the folder has all
-        // the information required (for a list or fetch operation), or (b) the operation was de
-        // facto local-only.  In particular, EmailStore will open and close lots of folders,
-        // causing a lot of connection setup and teardown
-        //
-        // However, want to eventually open, otherwise if there's no user interaction (i.e. a
-        // second account Inbox they don't manipulate), no remote connection will ever be made,
-        // meaning that folder normalization never happens and unsolicited notifications never
-        // arrive
-        this._account.session_pool.ready.connect(on_remote_ready);
-        if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
-            this.open_remote_session.begin();
-        } else {
-            this.remote_open_timer.start();
+        if (open_err != null) {
+            throw open_err;
         }
-        return true;
+
+        return opening;
     }
 
     /** {@inheritDoc} */
@@ -305,36 +274,17 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     /** {@inheritDoc} */
     public override async bool close_async(Cancellable? cancellable = null)
         throws Error {
-        bool is_closing = false;
-        if (open_count > 0) {
-            UserClose user_close = new UserClose(
-                () => {
-                    // Decrement the open count only if we are not
-                    // going to be fully closed here, since if so we
-                    // want close_internal_locked to be able to manage
-                    // when it is actually set to zero so it can clean
-                    // up beforehand as needed.
-                    if (this.open_count == 1) {
-                        is_closing = true;
-                        // Call close_internal in the background since
-                        // it recursively causes replay operations to
-                        // be scheduled, which since this is being
-                        // called from the replay queue would
-                        // otherwise deadlock.
-                        this.close_internal.begin(
-                            CloseReason.LOCAL_CLOSE,
-                            CloseReason.REMOTE_CLOSE,
-                            cancellable
-                        );
-                    } else if (this.open_count > 1) {
-                        this.open_count -= 1;
-                    }
-                    return is_closing;
-                });
-            this.replay_queue.schedule(user_close);
-            yield user_close.wait_for_ready_async(cancellable);
-        }
-        return is_closing;
+        // Although it's inefficient in the case of just decrementing
+        // the open count, pass all requests to close via the replay
+        // queue so that other operations queued are interleaved in an
+        // expected way, the same code path can be used to both test
+        // and decrement the open count, and that the decrement can be
+        // used under the same lock as actually closing the folder,
+        // making it essentially an atomic operation.
+        UserClose op = new UserClose(this, cancellable);
+        this.replay_queue.schedule(op);
+        yield op.wait_for_ready_async(cancellable);
+        return op.is_closing.is_certain();
     }
 
     /** {@inheritDoc} */
@@ -715,9 +665,54 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
     }
 
     /**
+     * Closes the folder and the remote session.
+     *
+     * This should only be called from the replay queue.
+     */
+    internal async bool close_internal(Folder.CloseReason local_reason,
+                                       Folder.CloseReason remote_reason,
+                                       Cancellable? cancellable) {
+        bool is_closing = false;
+        try {
+            int token = yield this.lifecycle_mutex.claim_async(cancellable);
+            // Don't ever decrement to zero here,
+            // close_internal_locked will do that later when it's
+            // appropriate to do so, after having flushed the replay
+            // queue. For the same reason, if we're actually going to
+            // do the close here, we need to hold the lock until it's
+            // done so that it's not possible to re-open half way
+            // through.
+            if (this.open_count == 1) {
+                is_closing = true;
+                this.close_internal_locked.begin(
+                    local_reason, remote_reason, cancellable,
+                    (obj, res) => {
+                        this.close_internal_locked.end(res);
+                        try {
+                            this.lifecycle_mutex.release(ref token);
+                        } catch (Error err) {
+                            // oh well
+                        }
+                    }
+                );
+            } else {
+                if (this.open_count > 1) {
+                    this.open_count -= 1;
+                } else {
+                    is_closing = true;
+                }
+                this.lifecycle_mutex.release(ref token);
+            }
+        } catch (Error err) {
+            // oh well
+        }
+        return is_closing;
+    }
+
+    /**
      * Unhooks the IMAP folder session and returns it to the account.
      */
-    internal async void close_remote_session(Folder.CloseReason remote_reason) {
+    private async void close_remote_session(Folder.CloseReason remote_reason) {
         // Since the remote session has is/has gone away, we need to
         // let waiters know. In the case of the folder being closed,
         // notify that no more remotes will ever come back, otherwise
@@ -749,32 +744,97 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
         }
     }
 
+    // Must be called when lifecycle_mutex is locked, i.e. from
+    // open_async().
+    private async bool open_locked(Folder.OpenFlags open_flags,
+                                   Cancellable cancellable)
+        throws Error {
+        if (this.open_count++ > 0) {
+            // even if opened or opening, or if forcing a re-open,
+            // respect the NO_DELAY flag
+            if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
+                // add NO_DELAY flag if it forces an open
+                if (this.remote_session == null)
+                    this.open_flags |= OpenFlags.NO_DELAY;
+
+                this.open_remote_session.begin();
+            }
+            return false;
+        }
+
+        // first open gets to name the flags, but see note above
+        this.open_flags = open_flags;
+
+        // reset to force waiting in wait_for_close_async()
+        this.closed_semaphore.reset();
+
+        // reset unseen count refresh since it will be updated when
+        // the remote opens
+        this.refresh_unseen_timer.reset();
+
+        // Construct objects needed when open
+        this.open_cancellable = new Cancellable();
+        this.replay_queue = new ReplayQueue(this);
+
+        // Notify the email prefetcher
+        this.email_prefetcher.open();
+
+        // notify about the local open
+        notify_opened(
+            Geary.Folder.OpenState.LOCAL,
+            this.local_folder.get_properties().email_total
+        );
+
+        // Unless NO_DELAY is set, do NOT open the remote side here; wait for the ReplayQueue to
+        // require a remote connection or wait_for_remote_async() to be called ... this allows for
+        // fast local-only operations to occur, local-only either because (a) the folder has all
+        // the information required (for a list or fetch operation), or (b) the operation was de
+        // facto local-only.  In particular, EmailStore will open and close lots of folders,
+        // causing a lot of connection setup and teardown
+        //
+        // However, want to eventually open, otherwise if there's no user interaction (i.e. a
+        // second account Inbox they don't manipulate), no remote connection will ever be made,
+        // meaning that folder normalization never happens and unsolicited notifications never
+        // arrive
+        this._account.session_pool.ready.connect(on_remote_ready);
+        if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
+            this.open_remote_session.begin();
+        } else {
+            this.remote_open_timer.start();
+        }
+
+        debug("%s: Folder opened", to_string());
+        return true;
+    }
+
     /**
-     * Closes the folder and the remote session.
+     * Closes the folder regardless of the open count.
+     *
+     * This only useful when an unrecoverable error has occurred. No
+     * cancellable argument is provided since the close must complete.
      */
-    private async void close_internal(Folder.CloseReason local_reason,
-                                      Folder.CloseReason remote_reason,
-                                      Cancellable? cancellable) {
+    private async void force_close(Folder.CloseReason local_reason,
+                                   Folder.CloseReason remote_reason) {
         try {
-            int token = yield this.close_mutex.claim_async(cancellable);
-            // Only actually close if we are still open. This guards
-            // against e.g. multiple callers calling when the open
-            // count is 1.
+            int token = yield this.lifecycle_mutex.claim_async(null);
+            // Check we actually need to do the close in case the
+            // folder was in the process of closing anyway
             if (this.open_count > 0) {
-                yield close_internal_locked(
-                    local_reason, remote_reason, cancellable
-                );
+                yield close_internal_locked(local_reason, remote_reason, null);
             }
-            this.close_mutex.release(ref token);
+            this.lifecycle_mutex.release(ref token);
         } catch (Error err) {
             // oh well
         }
     }
 
-    // Should only be called when close_mutex is locked, i.e. use close_internal()
+    // Must be called when lifecycle_mutex is locked, i.e. from
+    // close_internal() or force_close().
     private async void close_internal_locked(Folder.CloseReason local_reason,
                                              Folder.CloseReason remote_reason,
                                              Cancellable? cancellable) {
+        debug("%s: Folder closing", to_string());
+
         // Ensure we don't attempt to start opening a remote while
         // closing
         this._account.session_pool.ready.disconnect(on_remote_ready);
@@ -856,7 +916,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
      */
     private async void open_remote_session() {
         try {
-            int token = yield this.open_mutex.claim_async(this.open_cancellable);
+            int token = yield this.remote_mutex.claim_async(this.open_cancellable);
 
             // Ensure we are open already and guard against someone
             // else having called this just before we did.
@@ -869,13 +929,13 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
                 this.opening_monitor.notify_finish();
             }
 
-            this.open_mutex.release(ref token);
+            this.remote_mutex.release(ref token);
         } catch (Error err) {
             // Lock error
         }
     }
 
-    // Should only be called when open_mutex is locked, i.e. use open_remote_session()
+    // Should only be called when remote_mutex is locked, i.e. use open_remote_session()
     private async void open_remote_session_locked(Cancellable? cancellable) {
         debug("%s: Opening remote session", to_string());
 
@@ -941,12 +1001,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
                     local_reason =  CloseReason.LOCAL_CLOSE;
                     remote_reason = CloseReason.REMOTE_ERROR;
                 }
-
-                yield close_internal(
-                    local_reason,
-                    remote_reason,
-                    null // Don't pass cancellable, close must complete
-                );
+                yield force_close(local_reason, remote_reason);
             }
             return;
         }
@@ -964,11 +1019,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
             yield this._account.release_folder_session(session);
             if (!(err is IOError.CANCELLED)) {
                 notify_open_failed(Folder.OpenFailed.LOCAL_ERROR, err);
-                yield close_internal(
-                    CloseReason.LOCAL_ERROR,
-                    CloseReason.REMOTE_CLOSE,
-                    null // Don't pass cancellable, close must complete
-                );
+                yield force_close(CloseReason.LOCAL_ERROR, CloseReason.REMOTE_CLOSE);
             }
             return;
         }
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala 
b/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
index d023c53..48814ba 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
@@ -6,18 +6,17 @@
 
 private class Geary.ImapEngine.UserClose : Geary.ImapEngine.ReplayOperation {
 
-    /** A function that this operation can call to close the folder. */
-    public delegate bool CloseFolder();
-
     /** Determines the state of the close operation. */
     public Trillian is_closing = Trillian.UNKNOWN;
 
-    private CloseFolder close;
+    private MinimalFolder owner;
+    private Cancellable? cancellable;
 
 
-    public UserClose(owned CloseFolder close) {
-        base ("UserClose", Scope.LOCAL_ONLY);
-        this.close = (owned) close;
+    public UserClose(MinimalFolder owner, Cancellable? cancellable) {
+        base("UserClose", Scope.LOCAL_ONLY);
+        this.owner = owner;
+        this.cancellable = cancellable;
     }
 
     public override void notify_remote_removed_position(Imap.SequenceNumber removed) {
@@ -30,7 +29,11 @@ private class Geary.ImapEngine.UserClose : Geary.ImapEngine.ReplayOperation {
     }
 
     public override async ReplayOperation.Status replay_local_async() throws Error {
-        bool closing = this.close();
+        bool closing = yield this.owner.close_internal(
+            Folder.CloseReason.LOCAL_CLOSE,
+            Folder.CloseReason.REMOTE_CLOSE,
+            this.cancellable
+        );
         this.is_closing = Trillian.from_boolean(closing);
         return ReplayOperation.Status.COMPLETED;
     }


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]