[geary/mjog/741-inbox-getting-closed: 3/3] Geary.App.ConversationMonitor: Fix more races when starting/stopping

[Michael Gratton <mjog src gnome org>]

commit a0afb5f83afe286afa502794eb3b8ad5992a4077
Author: Michael Gratton <mike vee net>
Date:   Fri Apr 10 14:55:18 2020 +1000

    Geary.App.ConversationMonitor: Fix more races when starting/stopping
    
    Ensure that if stopping the conversation monitor while it is waiting
    for the folder to open that the folder isn't closed, and if an error
    is thrown when opening the folder that it gets thrown by
    ::start_monitoring anyway.
    
    Hopefully fixes #741

 src/engine/app/app-conversation-monitor.vala       | 34 +++++++-----
 test/engine/app/app-conversation-monitor-test.vala | 62 ++++++++++++++++++++++
 2 files changed, 84 insertions(+), 12 deletions(-)
---
diff --git a/src/engine/app/app-conversation-monitor.vala b/src/engine/app/app-conversation-monitor.vala
index 1edf3313..68d1221e 100644
--- a/src/engine/app/app-conversation-monitor.vala
+++ b/src/engine/app/app-conversation-monitor.vala
@@ -140,6 +140,9 @@ public class Geary.App.ConversationMonitor : BaseObject {
     /** Determines if the fill operation can load more messages. */
     internal bool fill_complete { get; set; default = false; }
 
+    // Determines if the base folder was actually opened or not
+    private bool base_was_opened = false;
+
     private Geary.Email.Field required_fields;
     private ConversationOperationQueue queue;
     private GLib.Cancellable operation_cancellable = new GLib.Cancellable();
@@ -315,6 +318,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
 
         // Set early yield to guard against reentrancy
         this.is_monitoring = true;
+        this.base_was_opened = false;
 
         this.base_folder.email_appended.connect(on_folder_email_appended);
         this.base_folder.email_inserted.connect(on_folder_email_inserted);
@@ -341,20 +345,28 @@ public class Geary.App.ConversationMonitor : BaseObject {
 
         try {
             yield this.base_folder.open_async(open_flags, opening);
-        } catch (Error err) {
+            this.base_was_opened = true;
+        } catch (GLib.Error err) {
+            // This check is needed since ::stop_monitoring may have
+            // been called while this call is waiting for the folder
+            // to finish opening. If so, we're already disconnected
+            // and don't need to do it again.
             if (this.is_monitoring) {
                 try {
-                    yield stop_monitoring_internal(false, null);
-                } catch (Error stop_error) {
+                    yield stop_monitoring_internal(null);
+                } catch (GLib.Error stop_error) {
                     warning(
                         "Error cleaning up after folder open error: %s", err.message
                     );
                 }
-                throw err;
             }
+
+            this.is_monitoring = false;
+            throw err;
         }
 
-        // Now the folder is open, start the queue running
+        // Now the folder is open, start the queue running. The here
+        // is needed for the same reason as the one immediately above.
         if (this.is_monitoring) {
             this.queue.run_process_async.begin();
         }
@@ -376,7 +388,9 @@ public class Geary.App.ConversationMonitor : BaseObject {
         throws GLib.Error {
         bool is_closing = false;
         if (this.is_monitoring) {
-            is_closing = yield stop_monitoring_internal(true, cancellable);
+            // Set now to prevent reentrancy during yield or signal
+            this.is_monitoring = false;
+            is_closing = yield stop_monitoring_internal(cancellable);
         }
         return is_closing;
     }
@@ -643,12 +657,8 @@ public class Geary.App.ConversationMonitor : BaseObject {
         email_flags_changed(conversation, email);
     }
 
-    private async bool stop_monitoring_internal(bool folder_was_opened,
-                                                Cancellable? cancellable)
+    private async bool stop_monitoring_internal(GLib.Cancellable? cancellable)
         throws Error {
-        // set now to prevent reentrancy during yield or signal
-        is_monitoring = false;
-
         this.base_folder.email_appended.disconnect(on_folder_email_appended);
         this.base_folder.email_inserted.disconnect(on_folder_email_inserted);
         this.base_folder.email_locally_complete.disconnect(on_folder_email_complete);
@@ -673,7 +683,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
         }
 
         bool closing = false;
-        if (folder_was_opened) {
+        if (this.base_was_opened) {
             try {
                 closing = yield this.base_folder.close_async(null);
             } catch (GLib.Error err) {
diff --git a/test/engine/app/app-conversation-monitor-test.vala 
b/test/engine/app/app-conversation-monitor-test.vala
index 3bec1de7..0dec7655 100644
--- a/test/engine/app/app-conversation-monitor-test.vala
+++ b/test/engine/app/app-conversation-monitor-test.vala
@@ -20,6 +20,8 @@ class Geary.App.ConversationMonitorTest : TestCase {
         base("Geary.App.ConversationMonitorTest");
         add_test("start_stop_monitoring", start_stop_monitoring);
         add_test("open_error", open_error);
+        add_test("close_during_open_error", close_during_open_error);
+        add_test("close_after_open_error", close_after_open_error);
         add_test("load_single_message", load_single_message);
         add_test("load_multiple_messages", load_multiple_messages);
         add_test("load_related_message", load_related_message);
@@ -117,9 +119,69 @@ class Geary.App.ConversationMonitorTest : TestCase {
             assert_error(open.throw_error, err);
         }
 
+        assert_false(monitor.is_monitoring, "is monitoring");
+
         this.base_folder.assert_expectations();
     }
 
+    public void close_during_open_error() throws GLib.Error {
+        ConversationMonitor monitor = new ConversationMonitor(
+            this.base_folder, Email.Field.NONE, 10
+        );
+
+        ExpectedCall open = this.base_folder
+            .expect_call("open_async")
+            .async_call(PAUSE)
+            .throws(new GLib.IOError.CANCELLED("Mock error"));
+        this.base_folder
+            .expect_call("close_async")
+            .throws(new EngineError.ALREADY_CLOSED("Mock error"));
+
+        var start_waiter = new AsyncResultWaiter(this.main_loop);
+        monitor.start_monitoring.begin(NONE, null, start_waiter.async_completion);
+
+        var stop_waiter = new AsyncResultWaiter(this.main_loop);
+        monitor.stop_monitoring.begin(null, stop_waiter.async_completion);
+
+        open.async_resume();
+        try {
+            monitor.start_monitoring.end(start_waiter.async_result());
+            assert_not_reached();
+        } catch (GLib.Error err) {
+            assert_error(open.throw_error, err);
+        }
+
+        // base_folder.close_async should not be called, so should not
+        // throw an error
+        monitor.stop_monitoring.end(stop_waiter.async_result());
+    }
+
+    public void close_after_open_error() throws GLib.Error {
+        ConversationMonitor monitor = new ConversationMonitor(
+            this.base_folder, Email.Field.NONE, 10
+        );
+
+        ExpectedCall open = this.base_folder
+            .expect_call("open_async")
+            .throws(new EngineError.SERVER_UNAVAILABLE("Mock error"));
+        this.base_folder
+            .expect_call("close_async")
+            .throws(new EngineError.ALREADY_CLOSED("Mock error"));
+
+        monitor.start_monitoring.begin(NONE, null, this.async_completion);
+        try {
+            monitor.start_monitoring.end(async_result());
+            assert_not_reached();
+        } catch (GLib.Error err) {
+            assert_error(open.throw_error, err);
+        }
+
+        // base_folder.close_async should not be called, so should not
+        // throw an error
+        monitor.stop_monitoring.begin(null, this.async_completion);
+        monitor.stop_monitoring.end(async_result());
+    }
+
     public void load_single_message() throws Error {
         Email e1 = setup_email(1);