GNOME.org
 

[geary] Improved disconnection/reestablish logic: Bug #733544

commit 2bdd67c093ba0efc05029c82d685b51d0241a66e
Author: Jim Nelson <jim yorba org>
Date:   Thu Aug 7 16:20:44 2014 -0700

    Improved disconnection/reestablish logic: Bug #733544
    
    This code fixes a couple of corner cases discovered by a user and
    myself regarding connection drops and reestablishment.
    
    1. When BYE received from the server, begin disconnecting immediately.
    Otherwise it's possible for the server to send a BYE and drop the cx
    ungracefully (or the FCLOSE is never received) and leave the
    ClientSession active.  By disconnecting immediately, Geary can cleanly
    break the session and immediately reestablish another one.
    
    2. Remove the DISCONNECTING state from ClientSession.  There was an
    asymmetry in the state machine with regards to how local disconnects
    were handled (and exposed by #1).  Analysis showed that the
    DISCONNECTING state was unnecessary since there was no need to wait
    for events to come back from the ClientConenction.  Now the FSM
    transitions straight to BROKEN when disconnecting.
    
    3. Flushing pending commands in the replay queue was being determined
    by the remote close reason.  This is insufficient in the case of BYE
    because it's a non-error close but, because it's initiated by the
    server, pending commands should not be flushed.  An additional close
    flag deals with this case.

 .../imap-engine/imap-engine-minimal-folder.vala    |   15 ++--
 .../replay-ops/imap-engine-replay-disconnect.vala  |    5 +-
 src/engine/imap/transport/imap-client-session.vala |   73 +++++++++----------
 3 files changed, 47 insertions(+), 46 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala 
b/src/engine/imap-engine/imap-engine-minimal-folder.vala
index 30e1093..f2e3a71 100644
--- a/src/engine/imap-engine/imap-engine-minimal-folder.vala
+++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala
@@ -590,7 +590,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.AbstractFolder, Geary.Folde
                 
                 // schedule immediate close
                 close_internal_async.begin(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false,
-                    cancellable);
+                    false, cancellable);
                 
                 return;
             }
@@ -648,7 +648,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.AbstractFolder, Geary.Folde
             
             // schedule immediate close and force reestablishment
             close_internal_async.begin(CloseReason.LOCAL_CLOSE, remote_reason, force_reestablishment,
-                null);
+                false, null);
             
             return;
         }
@@ -685,7 +685,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.AbstractFolder, Geary.Folde
             
             // schedule immediate close
             close_internal_async.begin(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false,
-                cancellable);
+                false, cancellable);
             
             return;
         }
@@ -705,18 +705,19 @@ private class Geary.ImapEngine.MinimalFolder : Geary.AbstractFolder, Geary.Folde
         if (remote_folder != null)
             _properties.remove(remote_folder.properties);
         
-        yield close_internal_async(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false,
+        yield close_internal_async(CloseReason.LOCAL_CLOSE, CloseReason.REMOTE_CLOSE, false, true,
             cancellable);
     }
     
     // NOTE: This bypasses open_count and forces the Folder closed.
     internal async void close_internal_async(Folder.CloseReason local_reason, Folder.CloseReason 
remote_reason,
-        bool force_reestablish, Cancellable? cancellable) {
+        bool force_reestablish, bool flush_pending, Cancellable? cancellable) {
         cancel_remote_open_timer();
         
         // only flushing pending ReplayOperations if this is a "clean" close, not forced due to
-        // error
-        bool flush_pending = !remote_reason.is_error();
+        // error and if specified by caller (could be a non-error close on the server, i.e. "BYE",
+        // but the connection is dropping, so don't flush pending)
+        flush_pending = flush_pending && !remote_reason.is_error();
         
         // If closing due to error, notify all operations waiting for the remote that it's not
         // coming available ... this wakes up any ReplayOperation blocking on wait_for_open_async(),
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala 
b/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
index 9ac2663..d277b9c 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-replay-disconnect.vala
@@ -34,8 +34,11 @@ private class Geary.ImapEngine.ReplayDisconnect : Geary.ImapEngine.ReplayOperati
         // that means a ReplayOperation is scheduling a ReplayOperation, which isn't something
         // we want to encourage, so use the Idle queue to schedule close_internal_async
         Idle.add(() => {
+            // ReplayDisconnect is only used when remote disconnects, so never flush pending, the
+            // connection is down or going down, but always force reestablish connection, since
+            // it wasn't our idea
             owner.close_internal_async.begin(Geary.Folder.CloseReason.LOCAL_CLOSE, remote_reason,
-                false, null);
+                true, false, null);
             
             return false;
         });
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index a49620c..ec45b41 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -42,6 +42,10 @@ public class Geary.Imap.ClientSession : BaseObject {
         public bool is_error() {
             return (this == LOCAL_ERROR) || (this == REMOTE_ERROR);
         }
+        
+        public bool is_remote() {
+            return (this == REMOTE_CLOSE) || (this == REMOTE_ERROR);
+        }
     }
     
     // Many of the async commands go through the FSM, and this is used to pass state in and out of
@@ -102,7 +106,6 @@ public class Geary.Imap.ClientSession : BaseObject {
         SELECTING,
         CLOSING_MAILBOX,
         LOGGING_OUT,
-        DISCONNECTING,
         
         // terminal state
         BROKEN,
@@ -337,7 +340,7 @@ public class Geary.Imap.ClientSession : BaseObject {
             new Geary.State.Mapping(State.LOGGING_OUT, Event.DISCONNECT, on_disconnect),
             new Geary.State.Mapping(State.LOGGING_OUT, Event.RECV_STATUS, on_dropped_response),
             new Geary.State.Mapping(State.LOGGING_OUT, Event.RECV_COMPLETION, 
on_logging_out_recv_completion),
-            new Geary.State.Mapping(State.LOGGING_OUT, Event.RECV_ERROR, on_disconnected),
+            new Geary.State.Mapping(State.LOGGING_OUT, Event.RECV_ERROR, on_recv_error),
             new Geary.State.Mapping(State.LOGGING_OUT, Event.SEND_ERROR, on_send_error),
             
             new Geary.State.Mapping(State.LOGGED_OUT, Event.CONNECT, on_already_connected),
@@ -349,22 +352,9 @@ public class Geary.Imap.ClientSession : BaseObject {
             new Geary.State.Mapping(State.LOGGED_OUT, Event.DISCONNECT, on_disconnect),
             new Geary.State.Mapping(State.LOGGED_OUT, Event.RECV_STATUS, on_dropped_response),
             new Geary.State.Mapping(State.LOGGED_OUT, Event.RECV_COMPLETION, on_dropped_response),
-            new Geary.State.Mapping(State.LOGGED_OUT, Event.RECV_ERROR, on_disconnected),
+            new Geary.State.Mapping(State.LOGGED_OUT, Event.RECV_ERROR, on_recv_error),
             new Geary.State.Mapping(State.LOGGED_OUT, Event.SEND_ERROR, on_send_error),
             
-            new Geary.State.Mapping(State.DISCONNECTING, Event.CONNECT, on_late_command),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.LOGIN, on_late_command),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.SEND_CMD, on_late_command),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.SELECT, on_late_command),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.CLOSE_MAILBOX, on_late_command),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.LOGOUT, on_late_command),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.DISCONNECT, Geary.State.nop),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.DISCONNECTED, on_disconnected),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.RECV_STATUS, on_dropped_response),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.RECV_COMPLETION, on_dropped_response),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.SEND_ERROR, on_disconnected),
-            new Geary.State.Mapping(State.DISCONNECTING, Event.RECV_ERROR, on_disconnected),
-            
             new Geary.State.Mapping(State.BROKEN, Event.CONNECT, on_late_command),
             new Geary.State.Mapping(State.BROKEN, Event.LOGIN, on_late_command),
             new Geary.State.Mapping(State.BROKEN, Event.SEND_CMD, on_late_command),
@@ -374,7 +364,9 @@ public class Geary.Imap.ClientSession : BaseObject {
             new Geary.State.Mapping(State.BROKEN, Event.DISCONNECT, Geary.State.nop),
             new Geary.State.Mapping(State.BROKEN, Event.DISCONNECTED, Geary.State.nop),
             new Geary.State.Mapping(State.BROKEN, Event.RECV_STATUS, on_dropped_response),
-            new Geary.State.Mapping(State.BROKEN, Event.RECV_COMPLETION, on_dropped_response)
+            new Geary.State.Mapping(State.BROKEN, Event.RECV_COMPLETION, on_dropped_response),
+            new Geary.State.Mapping(State.BROKEN, Event.SEND_ERROR, Geary.State.nop),
+            new Geary.State.Mapping(State.BROKEN, Event.RECV_ERROR, Geary.State.nop),
         };
         
         fsm = new Geary.State.Machine(machine_desc, mappings, on_ignored_transition);
@@ -410,7 +402,6 @@ public class Geary.Imap.ClientSession : BaseObject {
             case State.UNCONNECTED:
             case State.LOGGED_OUT:
             case State.LOGGING_OUT:
-            case State.DISCONNECTING:
             case State.BROKEN:
                 return Context.UNCONNECTED;
             
@@ -1011,6 +1002,11 @@ public class Geary.Imap.ClientSession : BaseObject {
             
             case Status.BYE:
                 debug("[%s] Received BYE from server: %s", to_string(), status_response.to_string());
+                
+                // nothing more we can do; drop connection and report disconnect to user
+                cx.disconnect_async.begin(null, on_bye_disconnect_completed);
+                
+                state = State.BROKEN;
             break;
             
             default:
@@ -1021,6 +1017,10 @@ public class Geary.Imap.ClientSession : BaseObject {
         return state;
     }
     
+    private void on_bye_disconnect_completed(Object? source, AsyncResult result) {
+        dispatch_disconnect_results(DisconnectReason.REMOTE_CLOSE, result);
+    }
+    
     //
     // select/examine
     //
@@ -1229,8 +1229,21 @@ public class Geary.Imap.ClientSession : BaseObject {
         if (params.err != null)
             throw params.err;
         
-        if (params.proceed)
+        if (!params.proceed)
+            return;
+        
+        Error? disconnect_err = null;
+        try {
             yield cx.disconnect_async(cancellable);
+        } catch (Error err) {
+            disconnect_err = err;
+        }
+        
+        drop_connection();
+        disconnected(DisconnectReason.LOCAL_CLOSE);
+        
+        if (disconnect_err != null)
+            throw disconnect_err;
     }
     
     private uint on_disconnect(uint state, uint event, void *user, Object? object) {
@@ -1238,22 +1251,6 @@ public class Geary.Imap.ClientSession : BaseObject {
         
         params.proceed = true;
         
-        return State.DISCONNECTING;
-    }
-    
-    private uint on_disconnected(uint state, uint event) {
-        // don't do inside signal handler -- although today drop_connection() doesn't fire signals or call
-        // callbacks, it could in the future
-        fsm.do_post_transition(() => {
-            drop_connection();
-            disconnected(DisconnectReason.LOCAL_CLOSE);
-        });
-        
-        // although we could go to the UNCONNECTED state, that implies the object can be reused ...
-        // while possible, that requires all state (not just the FSM) be reset at this point, and
-        // it just seems simpler and less buggy to require the user to discard this object and
-        // instantiate a new one
-        
         return State.BROKEN;
     }
     
@@ -1288,7 +1285,7 @@ public class Geary.Imap.ClientSession : BaseObject {
     }
     
     private void on_fire_send_error_signal(Object? object, AsyncResult result) {
-        dispatch_send_recv_results(DisconnectReason.LOCAL_ERROR, result);
+        dispatch_disconnect_results(DisconnectReason.LOCAL_ERROR, result);
     }
     
     private uint on_recv_error(uint state, uint event, void *user, Object? object, Error? err) {
@@ -1301,10 +1298,10 @@ public class Geary.Imap.ClientSession : BaseObject {
     }
     
     private void on_fire_recv_error_signal(Object? object, AsyncResult result) {
-        dispatch_send_recv_results(DisconnectReason.REMOTE_ERROR, result);
+        dispatch_disconnect_results(DisconnectReason.REMOTE_ERROR, result);
     }
     
-    private void dispatch_send_recv_results(DisconnectReason reason, AsyncResult result) {
+    private void dispatch_disconnect_results(DisconnectReason reason, AsyncResult result) {
         debug("[%s] Disconnected due to %s", to_string(), reason.to_string());
         
         try {
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]