[geary/wip/789924-network-transition-redux] Don't let a task try to re-open a folder while it is being closed.
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/wip/789924-network-transition-redux] Don't let a task try to re-open a folder while it is being closed.
- Date: Sat, 3 Mar 2018 00:32:14 +0000 (UTC)
commit dbe00b7f62efc2361d32ffbbaa277e01ad5541ce
Author: Michael James Gratton <mike vee net>
Date: Sat Mar 3 11:06:35 2018 +1100
Don't let a task try to re-open a folder while it is being closed.
Really (for sure this time) ensure folder opening/closing is not racy by
using the same mutex to guard the complete opening and closing processes.
* src/engine/imap-engine/imap-engine-minimal-folder.vala (MinimalFolder):
Rename open_mutex to remote_mutex since it's only actually used when
opening a remote session. Rename close_mutex to lifecycle_mutex and
ensure that has always been claimed when manipulating the
open_count. In particular, ensure that all of open_async is executed
under that mutex and ensure that both decrementing the close count, and
the complete actual closing process is always protected by it as well.
.../imap-engine/imap-engine-minimal-folder.vala | 273 ++++++++++++--------
.../replay-ops/imap-engine-user-close.vala | 19 +-
2 files changed, 173 insertions(+), 119 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala
b/src/engine/imap-engine/imap-engine-minimal-folder.vala
index ec5a371..3808642 100644
--- a/src/engine/imap-engine/imap-engine-minimal-folder.vala
+++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala
@@ -90,11 +90,11 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
private int open_count = 0;
private Folder.OpenFlags open_flags = OpenFlags.NONE;
private Cancellable? open_cancellable = null;
- private Nonblocking.Mutex open_mutex = new Nonblocking.Mutex();
- private Nonblocking.Mutex close_mutex = new Nonblocking.Mutex();
+ private Nonblocking.Mutex lifecycle_mutex = new Nonblocking.Mutex();
private Nonblocking.Semaphore closed_semaphore = new Nonblocking.Semaphore();
private Imap.FolderSession? remote_session = null;
+ private Nonblocking.Mutex remote_mutex = new Nonblocking.Mutex();
private Nonblocking.ReportingSemaphore<bool> remote_wait_semaphore =
new Nonblocking.ReportingSemaphore<bool>(false);
private TimeoutManager remote_open_timer;
@@ -161,7 +161,6 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
~MinimalFolder() {
if (open_count > 0)
warning("Folder %s destroyed without closing", to_string());
- this.local_folder.email_complete.disconnect(on_email_complete);
}
protected virtual void notify_closing(Gee.List<ReplayOperation> final_ops) {
@@ -200,6 +199,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
notify_special_folder_type_changed(old_type, new_type);
}
+ /** {@inheritDoc} */
public override Geary.Folder.OpenState get_open_state() {
if (this.open_count == 0)
return Geary.Folder.OpenState.CLOSED;
@@ -210,61 +210,30 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
}
/** {@inheritDoc} */
- public override async bool open_async(Geary.Folder.OpenFlags open_flags, Cancellable? cancellable = null)
+ public override async bool open_async(Folder.OpenFlags open_flags,
+ Cancellable? cancellable = null)
throws Error {
- if (open_count++ > 0) {
- // even if opened or opening, or if forcing a re-open, respect the NO_DELAY flag
- if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
- // add NO_DELAY flag if it forces an open
- if (this.remote_session == null)
- this.open_flags |= OpenFlags.NO_DELAY;
-
- this.open_remote_session.begin();
+ // Claim the lifecycle_mutex here so we don't try to re-open when
+ // the folder is in the middle of being closed.
+ bool opening = false;
+ Error? open_err = null;
+ try {
+ int token = yield this.lifecycle_mutex.claim_async(cancellable);
+ try {
+ opening = yield open_locked(open_flags, cancellable);
+ } catch (Error err) {
+ open_err = err;
}
- return false;
+ this.lifecycle_mutex.release(ref token);
+ } catch (Error err) {
+ // oh well
}
- // first open gets to name the flags, but see note above
- this.open_flags = open_flags;
-
- // reset to force waiting in wait_for_close_async()
- this.closed_semaphore.reset();
-
- // reset unseen count refresh since it will be updated when
- // the remote opens
- this.refresh_unseen_timer.reset();
-
- // Construct objects needed when open
- this.open_cancellable = new Cancellable();
- this.replay_queue = new ReplayQueue(this);
-
- // Notify the email prefetcher
- this.email_prefetcher.open();
-
- // notify about the local open
- notify_opened(
- Geary.Folder.OpenState.LOCAL,
- this.local_folder.get_properties().email_total
- );
-
- // Unless NO_DELAY is set, do NOT open the remote side here; wait for the ReplayQueue to
- // require a remote connection or wait_for_remote_async() to be called ... this allows for
- // fast local-only operations to occur, local-only either because (a) the folder has all
- // the information required (for a list or fetch operation), or (b) the operation was de
- // facto local-only. In particular, EmailStore will open and close lots of folders,
- // causing a lot of connection setup and teardown
- //
- // However, want to eventually open, otherwise if there's no user interaction (i.e. a
- // second account Inbox they don't manipulate), no remote connection will ever be made,
- // meaning that folder normalization never happens and unsolicited notifications never
- // arrive
- this._account.session_pool.ready.connect(on_remote_ready);
- if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
- this.open_remote_session.begin();
- } else {
- this.remote_open_timer.start();
+ if (open_err != null) {
+ throw open_err;
}
- return true;
+
+ return opening;
}
/** {@inheritDoc} */
@@ -305,36 +274,17 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
/** {@inheritDoc} */
public override async bool close_async(Cancellable? cancellable = null)
throws Error {
- bool is_closing = false;
- if (open_count > 0) {
- UserClose user_close = new UserClose(
- () => {
- // Decrement the open count only if we are not
- // going to be fully closed here, since if so we
- // want close_internal_locked to be able to manage
- // when it is actually set to zero so it can clean
- // up beforehand as needed.
- if (this.open_count == 1) {
- is_closing = true;
- // Call close_internal in the background since
- // it recursively causes replay operations to
- // be scheduled, which since this is being
- // called from the replay queue would
- // otherwise deadlock.
- this.close_internal.begin(
- CloseReason.LOCAL_CLOSE,
- CloseReason.REMOTE_CLOSE,
- cancellable
- );
- } else if (this.open_count > 1) {
- this.open_count -= 1;
- }
- return is_closing;
- });
- this.replay_queue.schedule(user_close);
- yield user_close.wait_for_ready_async(cancellable);
- }
- return is_closing;
+ // Although it's inefficient in the case of just decrementing
+ // the open count, pass all requests to close via the replay
+ // queue so that other operations queued are interleaved in an
+ // expected way, the same code path can be used to both test
+ // and decrement the open count, and that the decrement can be
+ // used under the same lock as actually closing the folder,
+ // making it essentially an atomic operation.
+ UserClose op = new UserClose(this, cancellable);
+ this.replay_queue.schedule(op);
+ yield op.wait_for_ready_async(cancellable);
+ return op.is_closing.is_certain();
}
/** {@inheritDoc} */
@@ -715,9 +665,54 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
}
/**
+ * Closes the folder and the remote session.
+ *
+ * This should only be called from the replay queue.
+ */
+ internal async bool close_internal(Folder.CloseReason local_reason,
+ Folder.CloseReason remote_reason,
+ Cancellable? cancellable) {
+ bool is_closing = false;
+ try {
+ int token = yield this.lifecycle_mutex.claim_async(cancellable);
+ // Don't ever decrement to zero here,
+ // close_internal_locked will do that later when it's
+ // appropriate to do so, after having flushed the replay
+ // queue. For the same reason, if we're actually going to
+ // do the close here, we need to hold the lock until it's
+ // done so that it's not possible to re-open half way
+ // through.
+ if (this.open_count == 1) {
+ is_closing = true;
+ this.close_internal_locked.begin(
+ local_reason, remote_reason, cancellable,
+ (obj, res) => {
+ this.close_internal_locked.end(res);
+ try {
+ this.lifecycle_mutex.release(ref token);
+ } catch (Error err) {
+ // oh well
+ }
+ }
+ );
+ } else {
+ if (this.open_count > 1) {
+ this.open_count -= 1;
+ } else {
+ is_closing = true;
+ }
+ this.lifecycle_mutex.release(ref token);
+ }
+ } catch (Error err) {
+ // oh well
+ }
+ return is_closing;
+ }
+
+ /**
* Unhooks the IMAP folder session and returns it to the account.
*/
- internal async void close_remote_session(Folder.CloseReason remote_reason) {
+ private async void close_remote_session(Folder.CloseReason remote_reason) {
// Since the remote session has is/has gone away, we need to
// let waiters know. In the case of the folder being closed,
// notify that no more remotes will ever come back, otherwise
@@ -749,32 +744,97 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
}
}
+ // Must be called when lifecycle_mutex is locked, i.e. from
+ // open_async().
+ private async bool open_locked(Folder.OpenFlags open_flags,
+ Cancellable cancellable)
+ throws Error {
+ if (this.open_count++ > 0) {
+ // even if opened or opening, or if forcing a re-open,
+ // respect the NO_DELAY flag
+ if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
+ // add NO_DELAY flag if it forces an open
+ if (this.remote_session == null)
+ this.open_flags |= OpenFlags.NO_DELAY;
+
+ this.open_remote_session.begin();
+ }
+ return false;
+ }
+
+ // first open gets to name the flags, but see note above
+ this.open_flags = open_flags;
+
+ // reset to force waiting in wait_for_close_async()
+ this.closed_semaphore.reset();
+
+ // reset unseen count refresh since it will be updated when
+ // the remote opens
+ this.refresh_unseen_timer.reset();
+
+ // Construct objects needed when open
+ this.open_cancellable = new Cancellable();
+ this.replay_queue = new ReplayQueue(this);
+
+ // Notify the email prefetcher
+ this.email_prefetcher.open();
+
+ // notify about the local open
+ notify_opened(
+ Geary.Folder.OpenState.LOCAL,
+ this.local_folder.get_properties().email_total
+ );
+
+ // Unless NO_DELAY is set, do NOT open the remote side here; wait for the ReplayQueue to
+ // require a remote connection or wait_for_remote_async() to be called ... this allows for
+ // fast local-only operations to occur, local-only either because (a) the folder has all
+ // the information required (for a list or fetch operation), or (b) the operation was de
+ // facto local-only. In particular, EmailStore will open and close lots of folders,
+ // causing a lot of connection setup and teardown
+ //
+ // However, want to eventually open, otherwise if there's no user interaction (i.e. a
+ // second account Inbox they don't manipulate), no remote connection will ever be made,
+ // meaning that folder normalization never happens and unsolicited notifications never
+ // arrive
+ this._account.session_pool.ready.connect(on_remote_ready);
+ if (open_flags.is_all_set(OpenFlags.NO_DELAY)) {
+ this.open_remote_session.begin();
+ } else {
+ this.remote_open_timer.start();
+ }
+
+ debug("%s: Folder opened", to_string());
+ return true;
+ }
+
/**
- * Closes the folder and the remote session.
+ * Closes the folder regardless of the open count.
+ *
+ * This only useful when an unrecoverable error has occurred. No
+ * cancellable argument is provided since the close must complete.
*/
- private async void close_internal(Folder.CloseReason local_reason,
- Folder.CloseReason remote_reason,
- Cancellable? cancellable) {
+ private async void force_close(Folder.CloseReason local_reason,
+ Folder.CloseReason remote_reason) {
try {
- int token = yield this.close_mutex.claim_async(cancellable);
- // Only actually close if we are still open. This guards
- // against e.g. multiple callers calling when the open
- // count is 1.
+ int token = yield this.lifecycle_mutex.claim_async(null);
+ // Check we actually need to do the close in case the
+ // folder was in the process of closing anyway
if (this.open_count > 0) {
- yield close_internal_locked(
- local_reason, remote_reason, cancellable
- );
+ yield close_internal_locked(local_reason, remote_reason, null);
}
- this.close_mutex.release(ref token);
+ this.lifecycle_mutex.release(ref token);
} catch (Error err) {
// oh well
}
}
- // Should only be called when close_mutex is locked, i.e. use close_internal()
+ // Must be called when lifecycle_mutex is locked, i.e. from
+ // close_internal() or force_close().
private async void close_internal_locked(Folder.CloseReason local_reason,
Folder.CloseReason remote_reason,
Cancellable? cancellable) {
+ debug("%s: Folder closing", to_string());
+
// Ensure we don't attempt to start opening a remote while
// closing
this._account.session_pool.ready.disconnect(on_remote_ready);
@@ -856,7 +916,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
*/
private async void open_remote_session() {
try {
- int token = yield this.open_mutex.claim_async(this.open_cancellable);
+ int token = yield this.remote_mutex.claim_async(this.open_cancellable);
// Ensure we are open already and guard against someone
// else having called this just before we did.
@@ -869,13 +929,13 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
this.opening_monitor.notify_finish();
}
- this.open_mutex.release(ref token);
+ this.remote_mutex.release(ref token);
} catch (Error err) {
// Lock error
}
}
- // Should only be called when open_mutex is locked, i.e. use open_remote_session()
+ // Should only be called when remote_mutex is locked, i.e. use open_remote_session()
private async void open_remote_session_locked(Cancellable? cancellable) {
debug("%s: Opening remote session", to_string());
@@ -941,12 +1001,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
local_reason = CloseReason.LOCAL_CLOSE;
remote_reason = CloseReason.REMOTE_ERROR;
}
-
- yield close_internal(
- local_reason,
- remote_reason,
- null // Don't pass cancellable, close must complete
- );
+ yield force_close(local_reason, remote_reason);
}
return;
}
@@ -964,11 +1019,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
yield this._account.release_folder_session(session);
if (!(err is IOError.CANCELLED)) {
notify_open_failed(Folder.OpenFailed.LOCAL_ERROR, err);
- yield close_internal(
- CloseReason.LOCAL_ERROR,
- CloseReason.REMOTE_CLOSE,
- null // Don't pass cancellable, close must complete
- );
+ yield force_close(CloseReason.LOCAL_ERROR, CloseReason.REMOTE_CLOSE);
}
return;
}
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
b/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
index d023c53..48814ba 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-user-close.vala
@@ -6,18 +6,17 @@
private class Geary.ImapEngine.UserClose : Geary.ImapEngine.ReplayOperation {
- /** A function that this operation can call to close the folder. */
- public delegate bool CloseFolder();
-
/** Determines the state of the close operation. */
public Trillian is_closing = Trillian.UNKNOWN;
- private CloseFolder close;
+ private MinimalFolder owner;
+ private Cancellable? cancellable;
- public UserClose(owned CloseFolder close) {
- base ("UserClose", Scope.LOCAL_ONLY);
- this.close = (owned) close;
+ public UserClose(MinimalFolder owner, Cancellable? cancellable) {
+ base("UserClose", Scope.LOCAL_ONLY);
+ this.owner = owner;
+ this.cancellable = cancellable;
}
public override void notify_remote_removed_position(Imap.SequenceNumber removed) {
@@ -30,7 +29,11 @@ private class Geary.ImapEngine.UserClose : Geary.ImapEngine.ReplayOperation {
}
public override async ReplayOperation.Status replay_local_async() throws Error {
- bool closing = this.close();
+ bool closing = yield this.owner.close_internal(
+ Folder.CloseReason.LOCAL_CLOSE,
+ Folder.CloseReason.REMOTE_CLOSE,
+ this.cancellable
+ );
this.is_closing = Trillian.from_boolean(closing);
return ReplayOperation.Status.COMPLETED;
}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
