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.
This commit is contained in:
Jim Nelson
parent
d932e7c31f
commit
2bdd67c093
3 changed files with 47 additions and 46 deletions
|
|
@ -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(),
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue