Geary.Imap.ClientSession: Treat logout as disconnect

Convert `get_protocol_state` to an automatic property, so that rather
than requiring explcit signals for lifecycle events, a GObject notify
signal can be used instead.

Convert disconnect signal to a property so it can be accessed if needed.

Convert code listening to the disconnect signal to listen to notify
signals for `protocol_state` instead, and hence also treat the session
as disconnected when a logout is in progress.

Fixes #986
This commit is contained in:
Michael Gratton 2020-09-27 16:00:35 +10:00
parent ed4ba33127
commit 41be8693d4
6 changed files with 130 additions and 96 deletions

View file

@ -252,7 +252,7 @@ public class Geary.Imap.ClientService : Geary.ClientService {
if (!disconnect) { if (!disconnect) {
// If the session has a mailbox selected, close it before // If the session has a mailbox selected, close it before
// adding it back to the pool // adding it back to the pool
ClientSession.ProtocolState proto = session.get_protocol_state(); ClientSession.ProtocolState proto = session.protocol_state;
if (proto == ClientSession.ProtocolState.SELECTED || if (proto == ClientSession.ProtocolState.SELECTED ||
proto == ClientSession.ProtocolState.SELECTING) { proto == ClientSession.ProtocolState.SELECTING) {
// always close mailbox to return to authorized state // always close mailbox to return to authorized state
@ -263,7 +263,7 @@ public class Geary.Imap.ClientService : Geary.ClientService {
session.to_string(), imap_error.message); session.to_string(), imap_error.message);
disconnect = true; disconnect = true;
} }
if (session.get_protocol_state() != AUTHORIZED) { if (session.protocol_state != AUTHORIZED) {
// Closing it didn't leave it in the desired // Closing it didn't leave it in the desired
// state, so drop it // state, so drop it
disconnect = true; disconnect = true;
@ -393,7 +393,7 @@ public class Geary.Imap.ClientService : Geary.ClientService {
/** Determines if a session is valid, disposing of it if not. */ /** Determines if a session is valid, disposing of it if not. */
private async bool check_session(ClientSession target, bool claiming) { private async bool check_session(ClientSession target, bool claiming) {
bool valid = false; bool valid = false;
switch (target.get_protocol_state()) { switch (target.protocol_state) {
case ClientSession.ProtocolState.AUTHORIZED: case ClientSession.ProtocolState.AUTHORIZED:
case ClientSession.ProtocolState.CLOSING_MAILBOX: case ClientSession.ProtocolState.CLOSING_MAILBOX:
valid = true; valid = true;
@ -472,7 +472,7 @@ public class Geary.Imap.ClientService : Geary.ClientService {
// Only bother tracking disconnects and enabling keeping alive // Only bother tracking disconnects and enabling keeping alive
// now the session is properly established. // now the session is properly established.
new_session.disconnected.connect(on_disconnected); new_session.notify["disconnected"].connect(on_session_disconnected);
new_session.enable_keepalives(selected_keepalive_sec, new_session.enable_keepalives(selected_keepalive_sec,
unselected_keepalive_sec, unselected_keepalive_sec,
selected_with_idle_keepalive_sec); selected_with_idle_keepalive_sec);
@ -509,7 +509,7 @@ public class Geary.Imap.ClientService : Geary.ClientService {
} }
private async void disconnect_session(ClientSession session) { private async void disconnect_session(ClientSession session) {
if (session.get_protocol_state() != NOT_CONNECTED) { if (session.protocol_state != NOT_CONNECTED) {
debug("Logging out session: %s", session.to_string()); debug("Logging out session: %s", session.to_string());
// No need to remove it after logging out, the // No need to remove it after logging out, the
// disconnected handler will do that for us. // disconnected handler will do that for us.
@ -548,21 +548,27 @@ public class Geary.Imap.ClientService : Geary.ClientService {
} }
if (removed) { if (removed) {
session.disconnected.disconnect(on_disconnected); session.notify["disconnected"].connect(on_session_disconnected);
} }
return removed; return removed;
} }
private void on_disconnected(ClientSession session, private void on_session_disconnected(GLib.Object source,
ClientSession.DisconnectReason reason) { GLib.ParamSpec param) {
debug( var session = source as ClientSession;
"Session disconnected: %s: %s", if (session != null &&
session.to_string(), reason.to_string() session.protocol_state == NOT_CONNECTED &&
); session.disconnected != null) {
this.remove_session_async.begin( debug(
session, "Session disconnected: %s: %s",
(obj, res) => { this.remove_session_async.end(res); } session.to_string(),
); session.disconnected.to_string()
);
this.remove_session_async.begin(
session,
(obj, res) => { this.remove_session_async.end(res); }
);
}
} }
} }

View file

@ -1170,7 +1170,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
protected override ClientSession get_session() protected override ClientSession get_session()
throws ImapError { throws ImapError {
var session = base.get_session(); var session = base.get_session();
if (session.get_protocol_state() != SELECTED && if (session.protocol_state != SELECTED &&
!this.mailbox.equal_to(session.selected_mailbox)) { !this.mailbox.equal_to(session.selected_mailbox)) {
throw new ImapError.NOT_CONNECTED( throw new ImapError.NOT_CONNECTED(
"IMAP object no longer SELECTED for %s", "IMAP object no longer SELECTED for %s",

View file

@ -39,7 +39,7 @@ public abstract class Geary.Imap.SessionObject : BaseObject, Logging.Source {
*/ */
protected SessionObject(ClientSession session) { protected SessionObject(ClientSession session) {
this.session = session; this.session = session;
this.session.disconnected.connect(on_disconnected); this.session.notify["protocol-state"].connect(on_session_state_change);
} }
~SessionObject() { ~SessionObject() {
@ -63,7 +63,9 @@ public abstract class Geary.Imap.SessionObject : BaseObject, Logging.Source {
this.session = null; this.session = null;
if (old_session != null) { if (old_session != null) {
old_session.disconnected.disconnect(on_disconnected); old_session.notify["protocol-state"].disconnect(
on_session_state_change
);
} }
return old_session; return old_session;
@ -93,7 +95,7 @@ public abstract class Geary.Imap.SessionObject : BaseObject, Logging.Source {
protected virtual ClientSession get_session() protected virtual ClientSession get_session()
throws ImapError { throws ImapError {
if (this.session == null || if (this.session == null ||
this.session.get_protocol_state() == NOT_CONNECTED) { this.session.protocol_state == NOT_CONNECTED) {
throw new ImapError.NOT_CONNECTED( throw new ImapError.NOT_CONNECTED(
"IMAP object has no session or is not connected" "IMAP object has no session or is not connected"
); );
@ -101,11 +103,19 @@ public abstract class Geary.Imap.SessionObject : BaseObject, Logging.Source {
return this.session; return this.session;
} }
private void on_disconnected(ClientSession.DisconnectReason reason) { private void on_session_state_change() {
debug("Disconnected %s", reason.to_string()); if (this.session != null &&
this.session.protocol_state == NOT_CONNECTED) {
close(); // Disconnect reason will null when the session is being
disconnected(reason); // logged out but the logout command has not yet been
// completed.
var reason = (
this.session.disconnected ??
ClientSession.DisconnectReason.LOCAL_CLOSE
);
close();
disconnected(reason);
}
} }
} }

View file

@ -90,7 +90,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
* *
* See [[http://tools.ietf.org/html/rfc3501#section-3]] * See [[http://tools.ietf.org/html/rfc3501#section-3]]
* *
* @see get_protocol_state * @see protocol_state
*/ */
public enum ProtocolState { public enum ProtocolState {
NOT_CONNECTED, NOT_CONNECTED,
@ -230,6 +230,55 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
"Geary.Imap.ClientSession", State.NOT_CONNECTED, State.COUNT, Event.COUNT, "Geary.Imap.ClientSession", State.NOT_CONNECTED, State.COUNT, Event.COUNT,
state_to_string, event_to_string); state_to_string, event_to_string);
/**
* Returns the current IMAP protocol state for the session.
*/
public ProtocolState protocol_state {
get {
var state = ProtocolState.NOT_CONNECTED;
switch (fsm.state) {
case State.NOT_CONNECTED:
case State.LOGOUT:
case State.CLOSED:
state = NOT_CONNECTED;
break;
case State.NOAUTH:
state = UNAUTHORIZED;
break;
case State.AUTHORIZED:
state = AUTHORIZED;
break;
case State.SELECTED:
state = SELECTED;
break;
case State.CONNECTING:
state = CONNECTING;
break;
case State.AUTHORIZING:
state = AUTHORIZING;
break;
case State.SELECTING:
state = SELECTING;
break;
case State.CLOSING_MAILBOX:
state = CLOSING_MAILBOX;
break;
}
return state;
}
}
/** Specifies the reason the session was disconnected, if any. */
public DisconnectReason? disconnected { get; private set; default = null; }
/** /**
* Set of IMAP extensions reported as being supported by the server. * Set of IMAP extensions reported as being supported by the server.
* *
@ -330,9 +379,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
// Connection state changes // Connection state changes
// //
/** Emitted when the session is disconnected for any reason. */
public signal void disconnected(DisconnectReason reason);
/** Emitted when an IMAP command status response is received. */ /** Emitted when an IMAP command status response is received. */
public signal void status_response_received(StatusResponse status_response); public signal void status_response_received(StatusResponse status_response);
@ -482,7 +528,14 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
new Geary.State.Mapping(State.CLOSED, Event.RECV_ERROR, Geary.State.nop), new Geary.State.Mapping(State.CLOSED, Event.RECV_ERROR, Geary.State.nop),
}; };
fsm = new Geary.State.Machine(machine_desc, mappings, on_ignored_transition); this.fsm = new Geary.State.Machine(
machine_desc,
mappings,
on_ignored_transition
);
this.fsm.notify["state"].connect(
() => this.notify_property("protocol_state")
);
} }
~ClientSession() { ~ClientSession() {
@ -493,7 +546,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
break; break;
default: default:
warning("ClientSession ref dropped while still active"); GLib.warning("ClientSession ref dropped while still active");
} }
} }
@ -636,43 +689,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
return delim; return delim;
} }
/**
* Returns the current {@link ProtocolState} of the {@link ClientSession} and, if selected,
* the current mailbox.
*/
public ProtocolState get_protocol_state() {
switch (fsm.get_state()) {
case State.NOT_CONNECTED:
case State.LOGOUT:
case State.CLOSED:
return ProtocolState.NOT_CONNECTED;
case State.NOAUTH:
return ProtocolState.UNAUTHORIZED;
case State.AUTHORIZED:
return ProtocolState.AUTHORIZED;
case State.SELECTED:
return ProtocolState.SELECTED;
case State.CONNECTING:
return ProtocolState.CONNECTING;
case State.AUTHORIZING:
return ProtocolState.AUTHORIZING;
case State.SELECTING:
return ProtocolState.SELECTING;
case State.CLOSING_MAILBOX:
return ProtocolState.CLOSING_MAILBOX;
default:
assert_not_reached();
}
}
// Some commands require waiting for a completion response in order to shift the state machine's // Some commands require waiting for a completion response in order to shift the state machine's
// State; this allocates such a wait, returning false if another command is outstanding also // State; this allocates such a wait, returning false if another command is outstanding also
// waiting for one to finish // waiting for one to finish
@ -1197,7 +1213,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
public void enable_idle() public void enable_idle()
throws GLib.Error { throws GLib.Error {
if (this.is_idle_supported) { if (this.is_idle_supported) {
switch (get_protocol_state()) { switch (this.protocol_state) {
case ProtocolState.AUTHORIZING: case ProtocolState.AUTHORIZING:
case ProtocolState.AUTHORIZED: case ProtocolState.AUTHORIZED:
case ProtocolState.SELECTED: case ProtocolState.SELECTED:
@ -1218,7 +1234,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
unschedule_keepalive(); unschedule_keepalive();
uint seconds; uint seconds;
switch (get_protocol_state()) { switch (this.protocol_state) {
case ProtocolState.NOT_CONNECTED: case ProtocolState.NOT_CONNECTED:
case ProtocolState.CONNECTING: case ProtocolState.CONNECTING:
return; return;
@ -1555,10 +1571,11 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
MachineParams params = (MachineParams) object; MachineParams params = (MachineParams) object;
assert(params.cmd is LogoutCommand); assert(params.cmd is LogoutCommand);
if (!reserve_state_change_cmd(params, state, event)) if (reserve_state_change_cmd(params, state, event)) {
return state; state = State.LOGOUT;
}
return State.LOGOUT; return state;
} }
private uint on_logging_out_recv_status(uint state, private uint on_logging_out_recv_status(uint state,
@ -1625,7 +1642,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
} }
drop_connection(); drop_connection();
disconnected(DisconnectReason.LOCAL_CLOSE); this.disconnected = DisconnectReason.LOCAL_CLOSE;
if (disconnect_err != null) if (disconnect_err != null)
throw disconnect_err; throw disconnect_err;
@ -1661,6 +1678,8 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
} }
private async void do_disconnect(DisconnectReason reason) { private async void do_disconnect(DisconnectReason reason) {
this.disconnected = reason;
try { try {
yield this.cx.disconnect_async(); yield this.cx.disconnect_async();
} catch (GLib.Error err) { } catch (GLib.Error err) {
@ -1668,7 +1687,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
} }
drop_connection(); drop_connection();
disconnected(reason);
} }
// //

View file

@ -44,17 +44,17 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(WAIT_FOR_DISCONNECT, ""); this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.disconnect_async.begin(null, this.async_completion); test_article.disconnect_async.begin(null, this.async_completion);
test_article.disconnect_async.end(async_result()); test_article.disconnect_async.end(async_result());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
TestServer.Result result = this.server.wait_for_script(this.main_loop); TestServer.Result result = this.server.wait_for_script(this.main_loop);
assert_true( assert_true(
@ -148,13 +148,13 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(WAIT_FOR_DISCONNECT, ""); this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.login_async.begin( test_article.login_async.begin(
new Credentials(PASSWORD, "test", "password"), new Credentials(PASSWORD, "test", "password"),
@ -162,11 +162,11 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.async_completion this.async_completion
); );
test_article.login_async.end(async_result()); test_article.login_async.end(async_result());
assert_true(test_article.get_protocol_state() == AUTHORIZED); assert_true(test_article.protocol_state == AUTHORIZED);
test_article.disconnect_async.begin(null, this.async_completion); test_article.disconnect_async.begin(null, this.async_completion);
test_article.disconnect_async.end(async_result()); test_article.disconnect_async.end(async_result());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
TestServer.Result result = this.server.wait_for_script(this.main_loop); TestServer.Result result = this.server.wait_for_script(this.main_loop);
assert_true( assert_true(
@ -185,17 +185,17 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(DISCONNECT, ""); this.server.add_script_line(DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.logout_async.begin(null, this.async_completion); test_article.logout_async.begin(null, this.async_completion);
test_article.logout_async.end(async_result()); test_article.logout_async.end(async_result());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
TestServer.Result result = this.server.wait_for_script(this.main_loop); TestServer.Result result = this.server.wait_for_script(this.main_loop);
assert_true( assert_true(
@ -216,13 +216,13 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(DISCONNECT, ""); this.server.add_script_line(DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.login_async.begin( test_article.login_async.begin(
new Credentials(PASSWORD, "test", "password"), new Credentials(PASSWORD, "test", "password"),
@ -230,11 +230,11 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.async_completion this.async_completion
); );
test_article.login_async.end(async_result()); test_article.login_async.end(async_result());
assert_true(test_article.get_protocol_state() == AUTHORIZED); assert_true(test_article.protocol_state == AUTHORIZED);
test_article.logout_async.begin(null, this.async_completion); test_article.logout_async.begin(null, this.async_completion);
test_article.logout_async.end(async_result()); test_article.logout_async.end(async_result());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
TestServer.Result result = this.server.wait_for_script(this.main_loop); TestServer.Result result = this.server.wait_for_script(this.main_loop);
assert_true( assert_true(
@ -261,13 +261,13 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(WAIT_FOR_DISCONNECT, ""); this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.initiate_session_async.begin( test_article.initiate_session_async.begin(
new Credentials(PASSWORD, "test", "password"), new Credentials(PASSWORD, "test", "password"),
@ -305,13 +305,13 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(WAIT_FOR_DISCONNECT, ""); this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.initiate_session_async.begin( test_article.initiate_session_async.begin(
new Credentials(PASSWORD, "test", "password"), new Credentials(PASSWORD, "test", "password"),
@ -368,13 +368,13 @@ class Geary.Imap.ClientSessionTest : TestCase {
this.server.add_script_line(WAIT_FOR_DISCONNECT, ""); this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
var test_article = new ClientSession(new_endpoint(), new Quirks()); var test_article = new ClientSession(new_endpoint(), new Quirks());
assert_true(test_article.get_protocol_state() == NOT_CONNECTED); assert_true(test_article.protocol_state == NOT_CONNECTED);
test_article.connect_async.begin( test_article.connect_async.begin(
CONNECT_TIMEOUT, null, this.async_completion CONNECT_TIMEOUT, null, this.async_completion
); );
test_article.connect_async.end(async_result()); test_article.connect_async.end(async_result());
assert_true(test_article.get_protocol_state() == UNAUTHORIZED); assert_true(test_article.protocol_state == UNAUTHORIZED);
test_article.initiate_session_async.begin( test_article.initiate_session_async.begin(
new Credentials(PASSWORD, "test", "password"), new Credentials(PASSWORD, "test", "password"),

View file

@ -34,7 +34,7 @@ class Integration.Imap.ClientSession : TestCase {
} }
public override void tear_down() throws GLib.Error { public override void tear_down() throws GLib.Error {
if (this.session.get_protocol_state() != NOT_CONNECTED) { if (this.session.protocol_state != NOT_CONNECTED) {
this.session.disconnect_async.begin(null, this.async_completion); this.session.disconnect_async.begin(null, this.async_completion);
this.session.disconnect_async.end(async_result()); this.session.disconnect_async.end(async_result());
} }