From 62f1df12cb0e8641b7163cb197e56e047386da60 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Sun, 29 Dec 2019 13:01:21 +1030 Subject: [PATCH] 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. --- .../transport/imap-client-connection.vala | 57 +++++++------------ .../imap/transport/imap-client-session.vala | 24 +------- .../imap/transport/imap-deserializer.vala | 32 +++++------ .../transport/imap-deserializer-test.vala | 4 +- 4 files changed, 38 insertions(+), 79 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 @@ -116,27 +116,29 @@ 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. - * - * @see receive_failure - */ - public signal void eos(); - /** * Fired when a syntax error has occurred. * - * This generally means the data looks like garbage and further deserialization is unlikely - * or impossible. + * This generally means the data looks like garbage and further + * deserialization is unlikely or impossible. */ public signal void deserialize_failure(); /** * Fired when an Error is trapped on the input stream. * - * 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 receive_failure(GLib.Error err); + + /** + * Fired when the underlying InputStream is closed. + * + * This is nonrecoverable and means the stream should be closed + * and this Deserializer destroyed. + */ + 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;});