[geary/mjog/imap-connection-fixes: 96/110] Rework Geary.Imap.ClientConnection signal and error handling

[Michael Gratton <mjog src gnome org>]

commit 2cbc1a1a3300be0d26d419d16fc477c748ebb067
Author: Michael Gratton <mike vee net>
Date:   Sun Dec 29 13:01:21 2019 +1030

    Rework Geary.Imap.ClientConnection signal and error handling
    
    Remove connect, disconnect and close_error signals, since they are
    implied by their respective methods completing and/or throwing an
    error. Remove Deserializer pass-through signals, treat all three kinds
    as generic receive errors instead.
    
    Make Deserializer emit end-of-stream signal only on EOS, not on EOS and
    on receive error, so it only signals an error condition once.

 .../imap/transport/imap-client-connection.vala     | 57 ++++++++--------------
 src/engine/imap/transport/imap-client-session.vala | 24 +--------
 src/engine/imap/transport/imap-deserializer.vala   | 30 ++++++------
 .../imap/transport/imap-deserializer-test.vala     |  4 +-
 4 files changed, 37 insertions(+), 78 deletions(-)
---
diff --git a/src/engine/imap/transport/imap-client-connection.vala 
b/src/engine/imap/transport/imap-client-connection.vala
index 6b742d7b..aa3c6dc1 100644
--- a/src/engine/imap/transport/imap-client-connection.vala
+++ b/src/engine/imap/transport/imap-client-connection.vala
@@ -89,14 +89,6 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
     private GLib.Cancellable? open_cancellable = null;
 
 
-    public virtual signal void connected() {
-        debug("Connected to %s", endpoint.to_string());
-    }
-
-    public virtual signal void disconnected() {
-        debug("Disconnected from %s", endpoint.to_string());
-    }
-
     public virtual signal void sent_command(Command cmd) {
         debug("SEND: %s", cmd.to_string());
     }
@@ -122,26 +114,14 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
         warning("Received bad response: %s", err.message);
     }
 
-    public virtual signal void received_eos() {
-        debug("Received eos");
-    }
-
     public virtual signal void send_failure(Error err) {
         warning("Send failure: %s", err.message);
     }
 
-    public virtual signal void receive_failure(Error err) {
+    public virtual signal void receive_failure(GLib.Error err) {
         warning("Receive failure: %s", err.message);
     }
 
-    public virtual signal void deserialize_failure(Error err) {
-        warning("Deserialize failure: %s", err.message);
-    }
-
-    public virtual signal void close_error(Error err) {
-        warning("Close error: %s", err.message);
-    }
-
 
     public ClientConnection(
         Geary.Endpoint endpoint,
@@ -216,8 +196,6 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
         this.pending_queue.clear();
         this.sent_queue.clear();
 
-        connected();
-
         try {
             yield open_channels_async();
         } catch (Error err) {
@@ -276,7 +254,6 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
                 close_error(close_err);
             }
 
-            disconnected();
         }
     }
 
@@ -359,20 +336,20 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
 
         this.open_cancellable = new GLib.Cancellable();
 
-        // Not buffering the Deserializer because it uses a DataInputStream, which is buffered
         ser_buffer = new BufferedOutputStream(ios.output_stream);
         ser_buffer.set_close_base_stream(false);
 
-        // Use ClientConnection cx_id for debugging aid with Serializer/Deserializer
         string id = "%04d".printf(cx_id);
         ser = new Serializer(id, ser_buffer);
-        des = new Deserializer(id, ios.input_stream);
 
-        des.parameters_ready.connect(on_parameters_ready);
+        // Not buffering the Deserializer because it uses a
+        // DataInputStream, which is already buffered
+        des = new Deserializer(id, this.cx.input_stream);
         des.bytes_received.connect(on_bytes_received);
-        des.receive_failure.connect(on_receive_failure);
         des.deserialize_failure.connect(on_deserialize_failure);
-        des.eos.connect(on_eos);
+        des.end_of_stream.connect(on_eos);
+        des.parameters_ready.connect(on_parameters_ready);
+        des.receive_failure.connect(on_receive_failure);
 
         // Start this running in the "background", it will stop when
         // open_cancellable is cancelled
@@ -394,11 +371,11 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
 
         // disconnect from Deserializer before yielding to stop it
         if (des != null) {
-            des.parameters_ready.disconnect(on_parameters_ready);
             des.bytes_received.disconnect(on_bytes_received);
-            des.receive_failure.disconnect(on_receive_failure);
             des.deserialize_failure.disconnect(on_deserialize_failure);
-            des.eos.disconnect(on_eos);
+            des.end_of_stream.disconnect(on_eos);
+            des.parameters_ready.disconnect(on_parameters_ready);
+            des.receive_failure.disconnect(on_receive_failure);
 
             yield des.stop_async();
         }
@@ -586,22 +563,26 @@ public class Geary.Imap.ClientConnection : BaseObject, Logging.Source {
         received_bytes(bytes);
     }
 
+    private void on_eos() {
+        receive_failure(
+            new ImapError.NOT_CONNECTED(
+                "End of stream reading from %s", to_string()
+            )
+        );
+    }
+
     private void on_receive_failure(Error err) {
         receive_failure(err);
     }
 
     private void on_deserialize_failure() {
-        deserialize_failure(
+        receive_failure(
             new ImapError.PARSE_ERROR(
                 "Unable to deserialize from %s", to_string()
             )
         );
     }
 
-    private void on_eos() {
-        received_eos();
-    }
-
     private void on_command_timeout(Command command) {
         this.sent_queue.remove(command);
         command.response_timed_out.disconnect(on_command_timeout);
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index a40c5986..bc2b28e4 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -739,8 +739,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         assert(cx == null);
         cx = new ClientConnection(imap_endpoint);
         cx.set_logging_parent(this);
-        cx.connected.connect(on_network_connected);
-        cx.disconnected.connect(on_network_disconnected);
         cx.sent_command.connect(on_network_sent_command);
         cx.send_failure.connect(on_network_send_error);
         cx.received_status_response.connect(on_received_status_response);
@@ -748,9 +746,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         cx.received_continuation_response.connect(on_received_continuation_response);
         cx.received_bytes.connect(on_received_bytes);
         cx.received_bad_response.connect(on_received_bad_response);
-        cx.received_eos.connect(on_received_eos);
         cx.receive_failure.connect(on_network_receive_failure);
-        cx.deserialize_failure.connect(on_network_receive_failure);
 
         assert(connect_waiter == null);
         connect_waiter = new Nonblocking.Semaphore();
@@ -760,14 +756,10 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         return State.CONNECTING;
     }
 
-    // this is used internally to tear-down the ClientConnection object and unhook it from
-    // ClientSession
     private void drop_connection() {
         unschedule_keepalive();
 
         if (cx != null) {
-            cx.connected.disconnect(on_network_connected);
-            cx.disconnected.disconnect(on_network_disconnected);
             cx.sent_command.disconnect(on_network_sent_command);
             cx.send_failure.disconnect(on_network_send_error);
             cx.received_status_response.disconnect(on_received_status_response);
@@ -775,9 +767,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
             cx.received_continuation_response.disconnect(on_received_continuation_response);
             cx.received_bytes.disconnect(on_received_bytes);
             cx.received_bad_response.disconnect(on_received_bad_response);
-            cx.received_eos.connect(on_received_eos);
             cx.receive_failure.disconnect(on_network_receive_failure);
-            cx.deserialize_failure.disconnect(on_network_receive_failure);
 
             cx = null;
         }
@@ -1807,14 +1797,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     // network connection event handlers
     //
 
-    private void on_network_connected() {
-        fsm.issue(Event.CONNECTED);
-    }
-
-    private void on_network_disconnected() {
-        fsm.issue(Event.DISCONNECTED);
-    }
-
     private void on_network_sent_command(Command cmd) {
         // resechedule keepalive
         schedule_keepalive();
@@ -1968,11 +1950,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         fsm.issue(Event.RECV_ERROR, null, null, err);
     }
 
-    private void on_received_eos(ClientConnection cx) {
-        fsm.issue(Event.RECV_ERROR, null, null, null);
-    }
-
-    private void on_network_receive_failure(Error err) {
+    private void on_network_receive_failure(GLib.Error err) {
         fsm.issue(Event.RECV_ERROR, null, null, err);
     }
 
diff --git a/src/engine/imap/transport/imap-deserializer.vala 
b/src/engine/imap/transport/imap-deserializer.vala
index 6959b09d..a9042265 100644
--- a/src/engine/imap/transport/imap-deserializer.vala
+++ b/src/engine/imap/transport/imap-deserializer.vala
@@ -117,26 +117,28 @@ public class Geary.Imap.Deserializer : BaseObject {
     public signal void bytes_received(size_t bytes);
 
     /**
-     * Fired when the underlying InputStream is closed, whether due to normal EOS or input error.
+     * Fired when a syntax error has occurred.
      *
-     * @see receive_failure
+     * This generally means the data looks like garbage and further
+     * deserialization is unlikely or impossible.
      */
-    public signal void eos();
+    public signal void deserialize_failure();
 
     /**
-     * Fired when a syntax error has occurred.
+     * Fired when an Error is trapped on the input stream.
      *
-     * This generally means the data looks like garbage and further deserialization is unlikely
-     * or impossible.
+     * This is nonrecoverable and means the stream should be closed
+     * and this Deserializer destroyed.
      */
-    public signal void deserialize_failure();
+    public signal void receive_failure(GLib.Error err);
 
     /**
-     * Fired when an Error is trapped on the input stream.
+     * Fired when the underlying InputStream is closed.
      *
-     * This is nonrecoverable and means the stream should be closed and this Deserializer destroyed.
+     * This is nonrecoverable and means the stream should be closed
+     * and this Deserializer destroyed.
      */
-    public signal void receive_failure(Error err);
+    public signal void end_of_stream();
 
 
     public Deserializer(string identifier, InputStream ins) {
@@ -816,8 +818,8 @@ public class Geary.Imap.Deserializer : BaseObject {
         flush_params();
 
         // always signal as closed and notify subscribers
-        closed_semaphore.blind_notify();
-        eos();
+        this.closed_semaphore.blind_notify();
+        end_of_stream();
 
         return State.CLOSED;
     }
@@ -833,9 +835,7 @@ public class Geary.Imap.Deserializer : BaseObject {
         }
 
         // always signal as closed and notify
-        closed_semaphore.blind_notify();
-        eos();
-
+        this.closed_semaphore.blind_notify();
         return State.CLOSED;
     }
 
diff --git a/test/engine/imap/transport/imap-deserializer-test.vala 
b/test/engine/imap/transport/imap-deserializer-test.vala
index 0960331d..f090057e 100644
--- a/test/engine/imap/transport/imap-deserializer-test.vala
+++ b/test/engine/imap/transport/imap-deserializer-test.vala
@@ -265,7 +265,7 @@ class Geary.Imap.DeserializerTest : TestCase {
         this.stream.add_data(bye.data);
 
         bool eos = false;
-        this.deser.eos.connect(() => { eos = true; });
+        this.deser.end_of_stream.connect(() => { eos = true; });
 
         this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
         RootParameters? message = this.process.end(async_result());
@@ -283,7 +283,7 @@ class Geary.Imap.DeserializerTest : TestCase {
 
         this.deser.parameters_ready.connect((param) => { message = param; });
         this.deser.bytes_received.connect((count) => { bytes_received += count; });
-        this.deser.eos.connect((param) => { eos = true; });
+        this.deser.end_of_stream.connect((param) => { eos = true; });
         this.deser.deserialize_failure.connect(() => { deserialize_failure = true; });
         this.deser.receive_failure.connect((err) => { receive_failure = true;});