[geary/wip/17-noisy-problem-reports: 10/12] Fix failed IMAP commands being treated as an invalid server response

[Michael Gratton <mjog src gnome org>]

commit 34ce1dc1aa420af02a53d961845c1df4a03fc2b0
Author: Michael Gratton <mike vee net>
Date:   Tue Jan 1 22:07:18 2019 +1100

    Fix failed IMAP commands being treated as an invalid server response
    
    By checking for a NO/BAD status reponse and throwing an error if found
    when parsing the server's response, the responses were treated as
    invalid rather than being simply rejected by the server.
    
    This updates Imap.Command so that when chcking its status response,
    it only throws an error on NO/BAD from the high-level API used by
    ClientSession, not ClientConnection, and fixes a minor cleanup issue.

 src/engine/imap/command/imap-command.vala             | 14 ++++++++++----
 src/engine/imap/transport/imap-client-connection.vala |  3 ++-
 2 files changed, 12 insertions(+), 5 deletions(-)
---
diff --git a/src/engine/imap/command/imap-command.vala b/src/engine/imap/command/imap-command.vala
index a3481905..5df2d357 100644
--- a/src/engine/imap/command/imap-command.vala
+++ b/src/engine/imap/command/imap-command.vala
@@ -226,7 +226,9 @@ public class Geary.Imap.Command : BaseObject {
     public async void wait_until_complete(GLib.Cancellable cancellable)
         throws GLib.Error {
         yield this.complete_lock.wait_async(cancellable);
-        check_status();
+        // Since this is part of the public API, perform a strict
+        // check on the status code.
+        check_status(true);
     }
 
     /**
@@ -250,7 +252,9 @@ public class Geary.Imap.Command : BaseObject {
         this.response_timer.reset();
         this.complete_lock.blind_notify();
         cancel_send();
-        check_status();
+        // Since this gets called by the client connection only check
+        // for an expected server response, good or bad
+        check_status(false);
     }
 
     /**
@@ -319,7 +323,7 @@ public class Geary.Imap.Command : BaseObject {
         }
     }
 
-    private void check_status() throws ImapError {
+    private void check_status(bool require_okay) throws ImapError {
         if (this.status == null) {
             throw new ImapError.SERVER_ERROR(
                 "%s: No command response was received",
@@ -335,7 +339,9 @@ public class Geary.Imap.Command : BaseObject {
             );
         }
 
-        if (this.status.status != Status.OK) {
+        // XXX should we be distinguishing between NO and BAD
+        // responses here?
+        if (require_okay && this.status.status != Status.OK) {
             throw new ImapError.SERVER_ERROR(
                 "%s: Command failed: %s",
                 to_brief_string(),
diff --git a/src/engine/imap/transport/imap-client-connection.vala 
b/src/engine/imap/transport/imap-client-connection.vala
index 1d4d0d16..b333bd60 100644
--- a/src/engine/imap/transport/imap-client-connection.vala
+++ b/src/engine/imap/transport/imap-client-connection.vala
@@ -547,8 +547,9 @@ public class Geary.Imap.ClientConnection : BaseObject {
                 );
             }
             this.sent_queue.remove(sent);
-            sent.completed(status);
             sent.response_timed_out.disconnect(on_command_timeout);
+            // This could throw an error so call it after cleaning up
+            sent.completed(status);
         }
 
         received_status_response(status);