[geary/mjog/imap-connection-fixes: 15/21] Update Geary.Imap.ClientSession connect timeout handling

[Michael Gratton <mjog src gnome org>]

commit 28602fc1e218287ea7fa454fb00102925cca4542
Author: Michael Gratton <mike vee net>
Date:   Sun Dec 29 17:01:28 2019 +1030

    Update Geary.Imap.ClientSession connect timeout handling
    
    Allow specifying the connect greeting timeout length, ensure that
    any connect errors are in place before releasing the connect waiter,
    add unit test to ensure it works properly.

 src/engine/api/geary-engine.vala                   |  6 +-
 src/engine/imap/api/imap-client-service.vala       |  6 +-
 src/engine/imap/transport/imap-client-session.vala | 80 ++++++++++++----------
 .../imap/transport/imap-client-session-test.vala   | 41 +++++++++--
 test/integration/imap/client-session.vala          |  4 +-
 5 files changed, 90 insertions(+), 47 deletions(-)
---
diff --git a/src/engine/api/geary-engine.vala b/src/engine/api/geary-engine.vala
index 3fee1063..157eb2b1 100644
--- a/src/engine/api/geary-engine.vala
+++ b/src/engine/api/geary-engine.vala
@@ -284,10 +284,12 @@ public class Geary.Engine : BaseObject {
             (security, cx) => account.untrusted_host(service, security, cx)
         );
 
-        Geary.Imap.ClientSession client = new Imap.ClientSession(endpoint);
+        var client = new Imap.ClientSession(endpoint);
         GLib.Error? imap_err = null;
         try {
-            yield client.connect_async(cancellable);
+            yield client.connect_async(
+                Imap.ClientSession.DEFAULT_GREETING_TIMEOUT_SEC, cancellable
+            );
         } catch (GLib.Error err) {
             imap_err = err;
         }
diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala
index 26d4aa6d..adbd9b4d 100644
--- a/src/engine/imap/api/imap-client-service.vala
+++ b/src/engine/imap/api/imap-client-service.vala
@@ -447,11 +447,13 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
 
         ClientSession new_session = new ClientSession(remote);
         new_session.set_logging_parent(this);
-        yield new_session.connect_async(cancellable);
+        yield new_session.connect_async(
+            ClientSession.DEFAULT_GREETING_TIMEOUT_SEC, cancellable
+        );
 
         try {
             yield new_session.initiate_session_async(login, cancellable);
-        } catch (Error err) {
+        } catch (GLib.Error err) {
             // need to disconnect before throwing error ... don't
             // honor Cancellable here, it's important to disconnect
             // the client before dropping the ref
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index 3e95fc5b..d20d65fb 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -74,7 +74,9 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     /** Default keep-alive interval when not in the Selected state. */
     public const uint DEFAULT_UNSELECTED_KEEPALIVE_SEC = RECOMMENDED_KEEPALIVE_SEC;
 
-    private const uint GREETING_TIMEOUT_SEC = Command.DEFAULT_RESPONSE_TIMEOUT_SEC;
+    /** Default time to wait for the server greeting when connecting. */
+    public const uint DEFAULT_GREETING_TIMEOUT_SEC =
+        Command.DEFAULT_RESPONSE_TIMEOUT_SEC;
 
 
     /**
@@ -659,8 +661,10 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
      * the server (that is, not a network problem).  The {@link
      * ClientSession} should be discarded.
      */
-    public async void connect_async(GLib.Cancellable? cancellable)
-        throws GLib.Error {
+    public async void connect_async(
+        uint greeting_timeout_sec = DEFAULT_GREETING_TIMEOUT_SEC,
+        GLib.Cancellable? cancellable = null
+    ) throws GLib.Error {
         MachineParams params = new MachineParams(null);
         fsm.issue(Event.CONNECT, null, params);
 
@@ -683,10 +687,13 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         }
 
         // set up timer to wait for greeting from server
-        Scheduler.Scheduled timeout = Scheduler.after_sec(GREETING_TIMEOUT_SEC, on_greeting_timeout);
+        Scheduler.Scheduled timeout = Scheduler.after_sec(
+            greeting_timeout_sec, on_greeting_timeout
+        );
 
-        // wait for the initial greeting or a timeout ... this prevents the caller from turning
-        // around and issuing a command while still in CONNECTING state
+        // wait for the initial greeting or a timeout ... this
+        // prevents the caller from turning around and issuing a
+        // command while still in CONNECTING state
         try {
             yield connect_waiter.wait_async(cancellable);
         } catch (GLib.IOError.CANCELLED err) {
@@ -783,7 +790,16 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     private uint on_connecting_recv_status(uint state, uint event, void *user, Object? object) {
         StatusResponse status_response = (StatusResponse) object;
 
-        // see on_connected() why signals and semaphore are delayed for this event
+        uint new_state = State.NOAUTH;
+        if (status_response.status != Status.OK) {
+            // Don't need to manually disconnect here, by setting
+            // connect_err here that will be done in connect_async
+            this.connect_err = new ImapError.UNAVAILABLE(
+                "Session denied: %s", status_response.get_text()
+            );
+            new_state = State.LOGOUT;
+        }
+
         try {
             connect_waiter.notify();
         } catch (Error err) {
@@ -793,25 +809,16 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
             );
         }
 
-        if (status_response.status == Status.OK) {
-            fsm.do_post_transition(() => { connected(); });
-
-            return State.NOAUTH;
-        }
-
-        fsm.do_post_transition(() => { session_denied(status_response.get_text()); });
+        return new_state;
+    }
 
+    private uint on_connecting_timeout(uint state, uint event) {
         // Don't need to manually disconnect here, by setting
         // connect_err here that will be done in connect_async
-        this.connect_err = new ImapError.UNAVAILABLE(
-            "Session denied: %s", status_response.get_text()
+        this.connect_err = new GLib.IOError.TIMED_OUT(
+            "Session greeting not sent"
         );
 
-        return State.LOGOUT;
-    }
-
-    private uint on_connecting_timeout(uint state, uint event) {
-        // wake up the waiting task in connect_async
         try {
             connect_waiter.notify();
         } catch (Error err) {
@@ -820,13 +827,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
             );
         }
 
-        // Don't need to manually disconnect here, by setting
-        // connect_err here that will be done in connect_async
-        this.connect_err = new IOError.TIMED_OUT(
-            "Session greeting not seen in %u seconds",
-            GREETING_TIMEOUT_SEC
-        );
-
         return State.LOGOUT;
     }
 
@@ -1093,18 +1093,24 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     //
 
     /**
-     * If seconds is negative or zero, keepalives will be disabled.  (This is not recommended.)
+     * Enables sending keep-alive commands for the sesion.
      *
-     * Although keepalives can be enabled at any time, if they're enabled and trigger sending
-     * a command prior to connection, error signals may be fired.
+     * Although keepalives can be enabled at any time, if they're
+     * enabled and trigger sending a command prior to connection,
+     * error signals may be fired.
+     *
+     * If values are negative or zero, keepalives will be disabled.
+     * (This is not recommended.)
      */
     public void enable_keepalives(uint seconds_while_selected,
-        uint seconds_while_unselected, uint seconds_while_selected_with_idle) {
-        selected_keepalive_secs = seconds_while_selected;
-        selected_with_idle_keepalive_secs = seconds_while_selected_with_idle;
-        unselected_keepalive_secs = seconds_while_unselected;
-
-        // schedule one now, although will be rescheduled if traffic is received before it fires
+                                  uint seconds_while_unselected,
+                                  uint seconds_while_selected_with_idle) {
+        this.selected_keepalive_secs = seconds_while_selected;
+        this.selected_with_idle_keepalive_secs = seconds_while_selected_with_idle;
+        this.unselected_keepalive_secs = seconds_while_unselected;
+
+        // schedule one now, although will be rescheduled if traffic
+        // is received before it fires
         schedule_keepalive();
     }
 
diff --git a/test/engine/imap/transport/imap-client-session-test.vala 
b/test/engine/imap/transport/imap-client-session-test.vala
index 329e9aca..63358ed0 100644
--- a/test/engine/imap/transport/imap-client-session-test.vala
+++ b/test/engine/imap/transport/imap-client-session-test.vala
@@ -7,6 +7,7 @@
 
 class Geary.Imap.ClientSessionTest : TestCase {
 
+    private const uint CONNECT_TIMEOUT = 2;
 
     private TestServer? server = null;
 
@@ -14,6 +15,9 @@ class Geary.Imap.ClientSessionTest : TestCase {
     public ClientSessionTest() {
         base("Geary.Imap.ClientSessionTest");
         add_test("connect_disconnect", connect_disconnect);
+        if (GLib.Test.slow()) {
+            add_test("connect_timeout", connect_timeout);
+        }
         add_test("login", login);
         add_test("logout", logout);
         add_test("login_logout", login_logout);
@@ -37,7 +41,9 @@ class Geary.Imap.ClientSessionTest : TestCase {
         var test_article = new ClientSession(new_endpoint());
         assert_true(test_article.get_protocol_state(null) == NOT_CONNECTED);
 
-        test_article.connect_async.begin(null, this.async_complete_full);
+        test_article.connect_async.begin(
+            CONNECT_TIMEOUT, null, this.async_complete_full
+        );
         test_article.connect_async.end(async_result());
         assert_true(test_article.get_protocol_state(null) == UNAUTHORIZED);
 
@@ -52,6 +58,27 @@ class Geary.Imap.ClientSessionTest : TestCase {
         );
     }
 
+    public void connect_timeout() throws GLib.Error {
+        this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
+
+        var test_article = new ClientSession(new_endpoint());
+
+        GLib.Timer timer = new GLib.Timer();
+        timer.start();
+        test_article.connect_async.begin(
+            CONNECT_TIMEOUT, null, this.async_complete_full
+        );
+        try {
+            test_article.connect_async.end(async_result());
+            assert_not_reached();
+        } catch (GLib.IOError.TIMED_OUT err) {
+            assert_double(timer.elapsed(), CONNECT_TIMEOUT, 0.5);
+        }
+
+        TestServer.Result result = this.server.wait_for_script(this.main_loop);
+        assert_true(result.succeeded);
+    }
+
     public void login() throws GLib.Error {
         this.server.add_script_line(
             SEND_LINE, "* OK localhost test server ready"
@@ -63,7 +90,9 @@ class Geary.Imap.ClientSessionTest : TestCase {
         var test_article = new ClientSession(new_endpoint());
         assert_true(test_article.get_protocol_state(null) == NOT_CONNECTED);
 
-        test_article.connect_async.begin(null, this.async_complete_full);
+        test_article.connect_async.begin(
+            CONNECT_TIMEOUT, null, this.async_complete_full
+        );
         test_article.connect_async.end(async_result());
         assert_true(test_article.get_protocol_state(null) == UNAUTHORIZED);
 
@@ -98,7 +127,9 @@ class Geary.Imap.ClientSessionTest : TestCase {
         var test_article = new ClientSession(new_endpoint());
         assert_true(test_article.get_protocol_state(null) == NOT_CONNECTED);
 
-        test_article.connect_async.begin(null, this.async_complete_full);
+        test_article.connect_async.begin(
+            CONNECT_TIMEOUT, null, this.async_complete_full
+        );
         test_article.connect_async.end(async_result());
         assert_true(test_article.get_protocol_state(null) == UNAUTHORIZED);
 
@@ -127,7 +158,9 @@ class Geary.Imap.ClientSessionTest : TestCase {
         var test_article = new ClientSession(new_endpoint());
         assert_true(test_article.get_protocol_state(null) == NOT_CONNECTED);
 
-        test_article.connect_async.begin(null, this.async_complete_full);
+        test_article.connect_async.begin(
+            CONNECT_TIMEOUT, null, this.async_complete_full
+        );
         test_article.connect_async.end(async_result());
         assert_true(test_article.get_protocol_state(null) == UNAUTHORIZED);
 
diff --git a/test/integration/imap/client-session.vala b/test/integration/imap/client-session.vala
index e2d38a63..7bafd9f4 100644
--- a/test/integration/imap/client-session.vala
+++ b/test/integration/imap/client-session.vala
@@ -41,7 +41,7 @@ class Integration.Imap.ClientSession : TestCase {
     }
 
     public void session_connect() throws GLib.Error {
-        this.session.connect_async.begin(null, async_complete_full);
+        this.session.connect_async.begin(2, null, async_complete_full);
         this.session.connect_async.end(async_result());
 
         this.session.disconnect_async.begin(null, async_complete_full);
@@ -98,7 +98,7 @@ class Integration.Imap.ClientSession : TestCase {
     }
 
     private void do_connect() throws GLib.Error {
-        this.session.connect_async.begin(null, async_complete_full);
+        this.session.connect_async.begin(5, null, async_complete_full);
         this.session.connect_async.end(async_result());
     }