[geary/mjog/account-command-stacks: 75/77] Geary.App.ConversationMonitor: Fix re-entrancy issues

[Michael Gratton <mjog src gnome org>]

commit ffb966530eaa94c658c8504cc25d41b1251655eb
Author: Michael Gratton <mike vee net>
Date:   Tue Nov 5 09:30:15 2019 +1100

    Geary.App.ConversationMonitor: Fix re-entrancy issues
    
    Avoid some issues if a monitor is closed as it is still being opened,
    clean up the API and implementation a bit.

 src/client/components/main-window.vala             | 10 +--
 src/engine/app/app-conversation-monitor.vala       | 75 +++++++++++++---------
 test/engine/app/app-conversation-monitor-test.vala | 37 +++++------
 3 files changed, 65 insertions(+), 57 deletions(-)
---
diff --git a/src/client/components/main-window.vala b/src/client/components/main-window.vala
index 51a963ca..3dc5ac57 100644
--- a/src/client/components/main-window.vala
+++ b/src/client/components/main-window.vala
@@ -429,7 +429,6 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
 
                 this.conversations = new Geary.App.ConversationMonitor(
                     to_select,
-                    NO_DELAY,
                     // Include fields for the conversation viewer as well so
                     // conversations can be displayed without having to go
                     // back to the db
@@ -1035,11 +1034,12 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
         to_open.conversations_added.connect(on_conversation_count_changed);
         to_open.conversations_removed.connect(on_conversation_count_changed);
 
-        to_open.start_monitoring_async.begin(
+        to_open.start_monitoring.begin(
+            NO_DELAY,
             cancellable,
             (obj, res) => {
                 try {
-                    to_open.start_monitoring_async.end(res);
+                    to_open.start_monitoring.end(res);
                 } catch (GLib.Error err) {
                     handle_error(to_open.base_folder.account.information, err);
                 }
@@ -1055,11 +1055,11 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
         to_close.conversations_added.disconnect(on_conversation_count_changed);
         to_close.conversations_removed.disconnect(on_conversation_count_changed);
 
-        to_close.stop_monitoring_async.begin(
+        to_close.stop_monitoring.begin(
             null,
             (obj, res) => {
                 try {
-                    to_close.stop_monitoring_async.end(res);
+                    to_close.stop_monitoring.end(res);
                 } catch (GLib.Error err) {
                     warning(
                         "Error closing conversation monitor %s: %s",
diff --git a/src/engine/app/app-conversation-monitor.vala b/src/engine/app/app-conversation-monitor.vala
index c3f522a1..8184da6b 100644
--- a/src/engine/app/app-conversation-monitor.vala
+++ b/src/engine/app/app-conversation-monitor.vala
@@ -20,7 +20,7 @@
  * conversations can be loaded afterwards as needed.
  *
  * When monitoring starts via a call to {@link
- * start_monitoring_async}, the folder will perform an initial
+ * start_monitoring}, the folder will perform an initial
  * //scan// of messages in the base folder and load conversation load
  * based on that. Increasing {@link min_window_count} will cause
  * additional scan operations to be executed as needed to fill the new
@@ -138,9 +138,8 @@ public class Geary.App.ConversationMonitor : BaseObject {
     internal bool fill_complete { get; set; default = false; }
 
     private Geary.Email.Field required_fields;
-    private Geary.Folder.OpenFlags open_flags;
-    private ConversationOperationQueue queue = null;
-    private Cancellable? operation_cancellable = null;
+    private ConversationOperationQueue queue;
+    private GLib.Cancellable operation_cancellable = new GLib.Cancellable();
 
     // Set of known, in-folder emails, explicitly loaded for the
     // monitor's window. This exists purely to support the window_size
@@ -276,19 +275,18 @@ public class Geary.App.ConversationMonitor : BaseObject {
      * Creates a conversation monitor for the given folder.
      *
      * @param base_folder a Folder to monitor for conversations
-     * @param open_flags See {@link Geary.Folder}
      * @param required_fields See {@link Geary.Folder}
      * @param min_window_count Minimum number of conversations that will be loaded
      */
     public ConversationMonitor(Folder base_folder,
-                               Folder.OpenFlags open_flags,
                                Email.Field required_fields,
                                int min_window_count) {
         this.base_folder = base_folder;
-        this.open_flags = open_flags;
         this.required_fields = required_fields | REQUIRED_FIELDS;
         this._min_window_count = min_window_count;
         this.conversations = new ConversationSet(base_folder);
+        this.operation_cancellable = new Cancellable();
+        this.queue = new ConversationOperationQueue(this.progress_monitor);
     }
 
     /**
@@ -301,16 +299,19 @@ public class Geary.App.ConversationMonitor : BaseObject {
      * The //cancellable// parameter will be used when opening the
      * folder, but not subsequently when scanning for new messages. To
      * cancel any such operations, simply close the monitor via {@link
-     * stop_monitoring_async}.
+     * stop_monitoring}.
+     *
+     * @param open_flags See {@link Geary.Folder}
+     * @param cancellable Passed to the folder open operation
      */
-    public async bool start_monitoring_async(Cancellable? cancellable = null)
-        throws Error {
+    public async bool start_monitoring(Folder.OpenFlags open_flags,
+                                       GLib.Cancellable? cancellable)
+        throws GLib.Error {
         if (this.is_monitoring)
             return false;
 
         // Set early yield to guard against reentrancy
         this.is_monitoring = true;
-        this.operation_cancellable = new Cancellable();
 
         this.base_folder.email_appended.connect(on_folder_email_appended);
         this.base_folder.email_inserted.connect(on_folder_email_inserted);
@@ -323,25 +324,37 @@ public class Geary.App.ConversationMonitor : BaseObject {
         this.base_folder.account.email_removed.connect(on_account_email_removed);
         this.base_folder.account.email_flags_changed.connect(on_account_email_flags_changed);
 
-        this.queue = new ConversationOperationQueue(this.progress_monitor);
         this.queue.operation_error.connect(on_operation_error);
         this.queue.add(new FillWindowOperation(this));
 
+        // Take the union of the two cancellables so that of the
+        // monitor is closed while it is opening, the folder open is
+        // also cancelled
+        GLib.Cancellable opening = new GLib.Cancellable();
+        if (cancellable != null) {
+            cancellable.cancelled.connect(() => opening.cancel());
+        }
+        this.operation_cancellable.cancelled.connect(() => opening.cancel());
+
         try {
-            yield this.base_folder.open_async(open_flags, cancellable);
+            yield this.base_folder.open_async(open_flags, opening);
         } catch (Error err) {
-            try {
-                yield stop_monitoring_internal(false, null);
-            } catch (Error stop_error) {
-                warning(
-                    "Error cleaning up after folder open error: %s", err.message
-                );
+            if (this.is_monitoring) {
+                try {
+                    yield stop_monitoring_internal(false, null);
+                } catch (Error stop_error) {
+                    warning(
+                        "Error cleaning up after folder open error: %s", err.message
+                    );
+                }
+                throw err;
             }
-            throw err;
         }
 
         // Now the folder is open, start the queue running
-        this.queue.run_process_async.begin();
+        if (this.is_monitoring) {
+            this.queue.run_process_async.begin();
+        }
 
         return true;
     }
@@ -356,11 +369,13 @@ public class Geary.App.ConversationMonitor : BaseObject {
      * internal monitor operations to complete, but will not prevent
      * attempts to close the base folder.
      */
-    public async bool stop_monitoring_async(Cancellable? cancellable) throws Error {
-        if (!is_monitoring)
-            return false;
-
-        return yield stop_monitoring_internal(true, cancellable);
+    public async bool stop_monitoring(GLib.Cancellable? cancellable)
+        throws GLib.Error {
+        bool is_closing = false;
+        if (this.is_monitoring) {
+            is_closing = yield stop_monitoring_internal(true, cancellable);
+        }
+        return is_closing;
     }
 
     /**
@@ -603,7 +618,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
         email_flags_changed(conversation, email);
     }
 
-    private async bool stop_monitoring_internal(bool close_folder,
+    private async bool stop_monitoring_internal(bool folder_was_opened,
                                                 Cancellable? cancellable)
         throws Error {
         // set now to prevent reentrancy during yield or signal
@@ -631,19 +646,15 @@ public class Geary.App.ConversationMonitor : BaseObject {
         } catch (Error err) {
             close_err = err;
         }
-        this.queue = null;
-
-        this.operation_cancellable = null;
 
         bool closing = false;
-        if (close_folder) {
+        if (folder_was_opened) {
             try {
                 // Always close the folder to prevent open leaks
                 closing = yield this.base_folder.close_async(null);
             } catch (Error err) {
                 warning("Unable to close monitored folder %s: %s",
                         this.base_folder.to_string(), err.message);
-                close_err = err;
             }
         }
 
diff --git a/test/engine/app/app-conversation-monitor-test.vala 
b/test/engine/app/app-conversation-monitor-test.vala
index a120e714..3d10684e 100644
--- a/test/engine/app/app-conversation-monitor-test.vala
+++ b/test/engine/app/app-conversation-monitor-test.vala
@@ -64,7 +64,7 @@ class Geary.App.ConversationMonitorTest : TestCase {
 
     public void start_stop_monitoring() throws Error {
         ConversationMonitor monitor = new ConversationMonitor(
-            this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10
+            this.base_folder, Email.Field.NONE, 10
         );
         Cancellable test_cancellable = new Cancellable();
 
@@ -73,27 +73,24 @@ class Geary.App.ConversationMonitorTest : TestCase {
         monitor.scan_started.connect(() => { saw_scan_started = true; });
         monitor.scan_completed.connect(() => { saw_scan_completed = true; });
 
-        this.base_folder.expect_call(
-            "open_async",
-            { MockObject.int_arg(Folder.OpenFlags.NONE), test_cancellable }
-        );
+        this.base_folder.expect_call("open_async");
         this.base_folder.expect_call("list_email_by_id_async");
         this.base_folder.expect_call("close_async");
 
-        monitor.start_monitoring_async.begin(
-            test_cancellable, (obj, res) => { async_complete(res); }
+        monitor.start_monitoring.begin(
+            NONE, test_cancellable, (obj, res) => { async_complete(res); }
         );
-        monitor.start_monitoring_async.end(async_result());
+        monitor.start_monitoring.end(async_result());
 
         // Process all of the async tasks arising from the open
         while (this.main_loop.pending()) {
             this.main_loop.iteration(true);
         }
 
-        monitor.stop_monitoring_async.begin(
+        monitor.stop_monitoring.begin(
             test_cancellable, (obj, res) => { async_complete(res); }
         );
-        monitor.stop_monitoring_async.end(async_result());
+        monitor.stop_monitoring.end(async_result());
 
         assert_true(saw_scan_started, "scan_started not fired");
         assert_true(saw_scan_completed, "scan_completed not fired");
@@ -103,18 +100,18 @@ class Geary.App.ConversationMonitorTest : TestCase {
 
     public void open_error() throws Error {
         ConversationMonitor monitor = new ConversationMonitor(
-            this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10
+            this.base_folder, Email.Field.NONE, 10
         );
 
         ExpectedCall open = this.base_folder
             .expect_call("open_async")
             .throws(new EngineError.SERVER_UNAVAILABLE("Mock error"));
 
-        monitor.start_monitoring_async.begin(
-            null, (obj, res) => { async_complete(res); }
+        monitor.start_monitoring.begin(
+            NONE, null, (obj, res) => { async_complete(res); }
         );
         try {
-            monitor.start_monitoring_async.end(async_result());
+            monitor.start_monitoring.end(async_result());
             assert_not_reached();
         } catch (Error err) {
             assert_error(open.throw_error, err);
@@ -248,10 +245,10 @@ class Geary.App.ConversationMonitorTest : TestCase {
         // Close the monitor to cancel the final load so it does not
         // error out during later tests
         this.base_folder.expect_call("close_async");
-        monitor.stop_monitoring_async.begin(
+        monitor.stop_monitoring.begin(
             null, (obj, res) => { async_complete(res); }
         );
-        monitor.stop_monitoring_async.end(async_result());
+        monitor.stop_monitoring.end(async_result());
     }
 
     public void external_folder_message_appended() throws Error {
@@ -387,7 +384,7 @@ class Geary.App.ConversationMonitorTest : TestCase {
                       Gee.MultiMap<Email,FolderPath>[] related_paths = {})
         throws Error {
         ConversationMonitor monitor = new ConversationMonitor(
-            this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10
+            this.base_folder, Email.Field.NONE, 10
         );
         Cancellable test_cancellable = new Cancellable();
 
@@ -479,10 +476,10 @@ class Geary.App.ConversationMonitorTest : TestCase {
             }
         }
 
-        monitor.start_monitoring_async.begin(
-            test_cancellable, (obj, res) => { async_complete(res); }
+        monitor.start_monitoring.begin(
+            NONE, test_cancellable, (obj, res) => { async_complete(res); }
         );
-        monitor.start_monitoring_async.end(async_result());
+        monitor.start_monitoring.end(async_result());
 
         if (base_folder_email.length == 0) {
             wait_for_call(list_call);