GNOME.org
 

[geary/wip/733544-disconnect] More bug fixing with broken connections and disconnect

commit bd8b624366fd38b9fc625757359a2f3f43c13a61
Author: Jim Nelson <jim yorba org>
Date:   Tue Aug 5 12:08:19 2014 -0700

    More bug fixing with broken connections and disconnect
    
    This does two things: (a) there was an asymmetry in the ClientSession
    state machine with how client-side disconnects were handled.  The
    DISCONNECTING state was used in some code paths and not in others.
    Analysis shows it's unnecessary.  All client-side disconnects immed.
    transition to BROKEN.
    
    (b) 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 |   68 ++++++++------------
 3 files changed, 40 insertions(+), 48 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 7f147a5..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;
             
@@ -1015,7 +1006,7 @@ public class Geary.Imap.ClientSession : BaseObject {
                 // nothing more we can do; drop connection and report disconnect to user
                 cx.disconnect_async.begin(null, on_bye_disconnect_completed);
                 
-                state = State.DISCONNECTING;
+                state = State.BROKEN;
             break;
             
             default:
@@ -1027,7 +1018,7 @@ public class Geary.Imap.ClientSession : BaseObject {
     }
     
     private void on_bye_disconnect_completed(Object? source, AsyncResult result) {
-        dispatch_send_recv_results(DisconnectReason.REMOTE_CLOSE, result);
+        dispatch_disconnect_results(DisconnectReason.REMOTE_CLOSE, result);
     }
     
     //
@@ -1238,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) {
@@ -1247,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;
     }
     
@@ -1297,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) {
@@ -1310,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]