[geary/mjog/mutex-lock-cleanup: 1/3] engine: Update Geary.Nonblocking.Mutex API
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary/mjog/mutex-lock-cleanup: 1/3] engine: Update Geary.Nonblocking.Mutex API
- Date: Sun, 4 Apr 2021 04:03:21 +0000 (UTC)
commit c3784496032fbb3424ff6c377c898ae164aca7ce
Author: Michael Gratton <mike vee net>
Date: Mon Mar 22 22:25:03 2021 +1100
engine: Update Geary.Nonblocking.Mutex API
Add a new inner `Token` class to replace the public int-based tokens,
allowing the token to expose it's own release method that does not
throw an exception, and hence can be used with async calls in a
finally clause (as a work-around for GNOME/vala#743).
Also drops `_async` from method names and cleans up other minor source
code style issues.
src/client/accounts/accounts-manager.vala | 14 +--
src/client/application/application-client.vala | 17 ++--
.../conversation-list/conversation-list-store.vala | 27 ++---
src/engine/app/app-search-folder.vala | 21 ++--
src/engine/db/db-versioned-database.vala | 14 +--
.../imap-engine/imap-engine-email-prefetcher.vala | 19 ++--
.../imap-engine/imap-engine-minimal-folder.vala | 46 +++------
src/engine/imap/api/imap-account-session.vala | 20 ++--
src/engine/imap/api/imap-folder-session.vala | 38 ++-----
src/engine/nonblocking/nonblocking-mutex.vala | 112 ++++++++++++++++-----
10 files changed, 156 insertions(+), 172 deletions(-)
---
diff --git a/src/client/accounts/accounts-manager.vala b/src/client/accounts/accounts-manager.vala
index f9c2bdffd..122b2898b 100644
--- a/src/client/accounts/accounts-manager.vala
+++ b/src/client/accounts/accounts-manager.vala
@@ -351,19 +351,11 @@ public class Accounts.Manager : GLib.Object {
// Ensure only one async task is saving an info at once, since
// at least the Engine can cause multiple saves to be called
// in quick succession when updating special folder config.
- int token = yield info.write_lock.claim_async(cancellable);
-
- GLib.Error? thrown = null;
+ var token = yield info.write_lock.claim(cancellable);
try {
yield save_account_locked(info, cancellable);
- } catch (GLib.Error err) {
- thrown = err;
- }
-
- info.write_lock.release(ref token);
-
- if (thrown != null) {
- throw thrown;
+ } finally {
+ token.release();
}
}
diff --git a/src/client/application/application-client.vala b/src/client/application/application-client.vala
index 6ce19ce2a..b363f9ef9 100644
--- a/src/client/application/application-client.vala
+++ b/src/client/application/application-client.vala
@@ -977,9 +977,9 @@ public class Application.Client : Gtk.Application {
private async void create_controller() {
bool first_run = false;
bool open_failed = false;
- int mutex_token = Geary.Nonblocking.Mutex.INVALID_TOKEN;
+ Geary.Nonblocking.Mutex.Token? token = null;
try {
- mutex_token = yield this.controller_mutex.claim_async();
+ token = yield this.controller_mutex.claim();
if (this.controller == null) {
message(
"%s %s%s prefix=%s exec_dir=%s is_installed=%s",
@@ -1007,13 +1007,8 @@ public class Application.Client : Gtk.Application {
dialog.show();
}
- if (mutex_token != Geary.Nonblocking.Mutex.INVALID_TOKEN) {
- try {
- this.controller_mutex.release(ref mutex_token);
- } catch (GLib.Error error) {
- warning("Failed to release controller mutex: %s",
- error.message);
- }
+ if (token != null) {
+ token.release();
}
if (open_failed) {
@@ -1033,12 +1028,12 @@ public class Application.Client : Gtk.Application {
// Closes the controller, if running
private async void destroy_controller() {
try {
- int mutex_token = yield this.controller_mutex.claim_async();
+ var token = yield this.controller_mutex.claim();
if (this.controller != null) {
yield this.controller.close();
this.controller = null;
}
- this.controller_mutex.release(ref mutex_token);
+ token.release();
} catch (GLib.Error err) {
warning("Error destroying controller: %s", err.message);
}
diff --git a/src/client/conversation-list/conversation-list-store.vala
b/src/client/conversation-list/conversation-list-store.vala
index 3854253c4..0e94448fd 100644
--- a/src/client/conversation-list/conversation-list-store.vala
+++ b/src/client/conversation-list/conversation-list-store.vala
@@ -155,25 +155,16 @@ public class ConversationListStore : Gtk.ListStore {
// "scan-started" signals as messages come in fast and furious, but only want to process
// previews one at a time, otherwise it's possible to issue multiple requests for the
// same set
- int token;
try {
- token = yield refresh_mutex.claim_async(this.cancellable);
- } catch (Error err) {
- debug("Unable to claim refresh mutex: %s", err.message);
-
- return;
- }
-
- preview_monitor.notify_start();
-
- yield do_refresh_previews_async(conversation_monitor);
-
- preview_monitor.notify_finish();
-
- try {
- refresh_mutex.release(ref token);
- } catch (Error err) {
- debug("Unable to release refresh mutex: %s", err.message);
+ var token = yield refresh_mutex.claim(this.cancellable);
+ preview_monitor.notify_start();
+ yield do_refresh_previews_async(conversation_monitor);
+ preview_monitor.notify_finish();
+ token.release();
+ } catch (GLib.IOError.CANCELLED mutex_err) {
+ // fine
+ } catch (GLib.Error mutex_err) {
+ warning("Unable to release refresh mutex: %s", mutex_err.message);
}
}
diff --git a/src/engine/app/app-search-folder.vala b/src/engine/app/app-search-folder.vala
index a8fa9404e..17967d776 100644
--- a/src/engine/app/app-search-folder.vala
+++ b/src/engine/app/app-search-folder.vala
@@ -209,9 +209,9 @@ public class Geary.App.SearchFolder :
GLib.Cancellable? cancellable = null)
throws GLib.Error {
debug("Waiting for checking contains");
- int result_mutex_token = yield this.result_mutex.claim_async(cancellable);
+ var result_mutex_token = yield this.result_mutex.claim(cancellable);
var existing_ids = this.ids;
- result_mutex.release(ref result_mutex_token);
+ result_mutex_token.release();
debug("Checking contains");
return Geary.traverse(
@@ -229,10 +229,10 @@ public class Geary.App.SearchFolder :
Cancellable? cancellable = null
) throws GLib.Error {
debug("Waiting to list email");
- int result_mutex_token = yield this.result_mutex.claim_async(cancellable);
+ var result_mutex_token = yield this.result_mutex.claim(cancellable);
var existing_entries = this.entries;
var existing_ids = this.ids;
- result_mutex.release(ref result_mutex_token);
+ result_mutex_token.release();
debug("Listing email");
var engine_ids = new Gee.LinkedList<EmailIdentifier>();
@@ -420,7 +420,7 @@ public class Geary.App.SearchFolder :
debug("Waiting to append to search results");
try {
- int result_mutex_token = yield this.result_mutex.claim_async(
+ var result_mutex_token = yield this.result_mutex.claim(
cancellable
);
try {
@@ -432,8 +432,7 @@ public class Geary.App.SearchFolder :
new AccountProblemReport(this.account.information, error)
);
}
-
- this.result_mutex.release(ref result_mutex_token);
+ result_mutex_token.release();
} catch (GLib.IOError.CANCELLED mutex_err) {
// all good
} catch (GLib.Error mutex_err) {
@@ -449,7 +448,7 @@ public class Geary.App.SearchFolder :
debug("Waiting to update search results");
try {
- int result_mutex_token = yield this.result_mutex.claim_async(
+ var result_mutex_token = yield this.result_mutex.claim(
cancellable
);
try {
@@ -460,7 +459,7 @@ public class Geary.App.SearchFolder :
);
}
- this.result_mutex.release(ref result_mutex_token);
+ result_mutex_token.release();
} catch (GLib.IOError.CANCELLED mutex_err) {
// all good
} catch (GLib.Error mutex_err) {
@@ -478,7 +477,7 @@ public class Geary.App.SearchFolder :
debug("Waiting to remove from search results");
try {
- int result_mutex_token = yield this.result_mutex.claim_async(
+ var result_mutex_token = yield this.result_mutex.claim(
cancellable
);
@@ -500,7 +499,7 @@ public class Geary.App.SearchFolder :
}
}
- this.result_mutex.release(ref result_mutex_token);
+ result_mutex_token.release();
} catch (GLib.IOError.CANCELLED mutex_err) {
// all good
} catch (GLib.Error mutex_err) {
diff --git a/src/engine/db/db-versioned-database.vala b/src/engine/db/db-versioned-database.vala
index fc0345e3c..8efcc7c8e 100644
--- a/src/engine/db/db-versioned-database.vala
+++ b/src/engine/db/db-versioned-database.vala
@@ -147,23 +147,15 @@ public class Geary.Db.VersionedDatabase : Geary.Db.Database {
// means overall it might take a bit longer, but it keeps
// things usable in the meantime. See
// <https://bugzilla.gnome.org/show_bug.cgi?id=724475>.
- int token = yield VersionedDatabase.upgrade_mutex.claim_async(
+ var token = yield VersionedDatabase.upgrade_mutex.claim(
cancellable
);
-
- Error? locked_err = null;
try {
yield execute_upgrade(
cx, db_version, upgrade_script, cancellable
);
- } catch (Error err) {
- locked_err = err;
- }
-
- VersionedDatabase.upgrade_mutex.release(ref token);
-
- if (locked_err != null) {
- throw locked_err;
+ } finally {
+ token.release();
}
}
diff --git a/src/engine/imap-engine/imap-engine-email-prefetcher.vala
b/src/engine/imap-engine/imap-engine-email-prefetcher.vala
index 3c12583b5..b20b1c056 100644
--- a/src/engine/imap-engine/imap-engine-email-prefetcher.vala
+++ b/src/engine/imap-engine/imap-engine-email-prefetcher.vala
@@ -140,23 +140,18 @@ private class Geary.ImapEngine.EmailPrefetcher : Geary.BaseObject {
}
private async void do_prefetch_async() {
- int token = Nonblocking.Mutex.INVALID_TOKEN;
+ Nonblocking.Mutex.Token? token = null;
try {
- token = yield mutex.claim_async(cancellable);
+ token = yield mutex.claim(cancellable);
yield do_prefetch_batch_async();
} catch (Error err) {
if (!(err is IOError.CANCELLED))
debug("Error while prefetching emails for %s: %s", folder.to_string(), err.message);
- }
-
- // this round is done
- active_sem.blind_notify();
-
- if (token != Nonblocking.Mutex.INVALID_TOKEN) {
- try {
- mutex.release(ref token);
- } catch (Error release_err) {
- debug("Unable to release email prefetcher mutex: %s", release_err.message);
+ } finally {
+ // this round is done
+ this.active_sem.blind_notify();
+ if (token != null) {
+ token.release();
}
}
}
diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala
b/src/engine/imap-engine/imap-engine-minimal-folder.vala
index a004e1844..5db2af0ac 100644
--- a/src/engine/imap-engine/imap-engine-minimal-folder.vala
+++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala
@@ -224,23 +224,13 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
// 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;
- }
- this.lifecycle_mutex.release(ref token);
- } catch (Error err) {
- // oh well
- }
- if (open_err != null) {
- throw open_err;
+ var token = yield this.lifecycle_mutex.claim(cancellable);
+ try {
+ opening = yield open_locked(open_flags, cancellable);
+ } finally {
+ token.release();
}
-
return opening;
}
@@ -747,7 +737,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
Cancellable? cancellable) {
bool is_closing = false;
try {
- int token = yield this.lifecycle_mutex.claim_async(cancellable);
+ var token = yield this.lifecycle_mutex.claim(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
@@ -761,11 +751,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
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
- }
+ token.release();
}
);
} else {
@@ -774,7 +760,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
} else {
is_closing = true;
}
- this.lifecycle_mutex.release(ref token);
+ token.release();
}
} catch (Error err) {
// oh well
@@ -894,15 +880,15 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
private async void force_close(Folder.CloseReason local_reason,
Folder.CloseReason remote_reason) {
try {
- int token = yield this.lifecycle_mutex.claim_async(null);
+ var token = yield this.lifecycle_mutex.claim(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, null);
}
- this.lifecycle_mutex.release(ref token);
- } catch (Error err) {
- // oh well
+ token.release();
+ } catch (GLib.Error mutex_err) {
+ warning("Failed to acquire lifecycle mutex: %s", mutex_err.message);
}
}
@@ -994,7 +980,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
*/
private async void open_remote_session() {
try {
- int token = yield this.remote_mutex.claim_async(this.open_cancellable);
+ var token = yield this.remote_mutex.claim(this.open_cancellable);
// Ensure we are open already and guard against someone
// else having called this just before we did.
@@ -1007,9 +993,9 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport
this.opening_monitor.notify_finish();
}
- this.remote_mutex.release(ref token);
- } catch (Error err) {
- // Lock error
+ token.release();
+ } catch (GLib.Error mutex_err) {
+ warning("Failed to acquire remote mutex: %s", mutex_err.message);
}
}
diff --git a/src/engine/imap/api/imap-account-session.vala b/src/engine/imap/api/imap-account-session.vala
index 813d9e5e2..53ab9113b 100644
--- a/src/engine/imap/api/imap-account-session.vala
+++ b/src/engine/imap/api/imap-account-session.vala
@@ -436,29 +436,21 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject {
Cancellable? cancellable)
throws Error {
Gee.Map<Command, StatusResponse>? responses = null;
- int token = yield this.cmd_mutex.claim_async(cancellable);
+ var token = yield this.cmd_mutex.claim(cancellable);
// set up collectors
this.list_collector = list_results;
this.status_collector = status_results;
- Error? cmd_err = null;
try {
responses = yield session.send_multiple_commands_async(
cmds, cancellable
);
- } catch (Error err) {
- cmd_err = err;
- }
-
- // tear down collectors
- this.list_collector = null;
- this.status_collector = null;
-
- this.cmd_mutex.release(ref token);
-
- if (cmd_err != null) {
- throw cmd_err;
+ } finally {
+ // tear down collectors
+ this.list_collector = null;
+ this.status_collector = null;
+ token.release();
}
return responses;
diff --git a/src/engine/imap/api/imap-folder-session.vala b/src/engine/imap/api/imap-folder-session.vala
index d544b3b5c..251a0f60d 100644
--- a/src/engine/imap/api/imap-folder-session.vala
+++ b/src/engine/imap/api/imap-folder-session.vala
@@ -125,21 +125,14 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
/**
* Enables IMAP IDLE for the session, if supported.
*/
- public async void enable_idle(Cancellable? cancellable)
- throws Error {
- ClientSession session = get_session();
- int token = yield this.cmd_mutex.claim_async(cancellable);
- Error? cmd_err = null;
+ public async void enable_idle(GLib.Cancellable? cancellable)
+ throws GLib.Error {
+ var session = get_session();
+ var token = yield this.cmd_mutex.claim(cancellable);
try {
session.enable_idle();
- } catch (Error err) {
- cmd_err = err;
- }
-
- this.cmd_mutex.release(ref token);
-
- if (cmd_err != null) {
- throw cmd_err;
+ } finally {
+ token.release();
}
}
@@ -305,27 +298,18 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
throws GLib.Error {
ClientSession session = get_session();
Gee.Map<Command, StatusResponse>? responses = null;
- int token = yield this.cmd_mutex.claim_async(cancellable);
+ var token = yield this.cmd_mutex.claim(cancellable);
this.fetch_accumulator = fetch_results;
this.search_accumulator = search_results;
-
- Error? cmd_err = null;
try {
responses = yield session.send_multiple_commands_async(
cmds, cancellable
);
- } catch (Error err) {
- cmd_err = err;
- }
-
- this.fetch_accumulator = null;
- this.search_accumulator = null;
-
- this.cmd_mutex.release(ref token);
-
- if (cmd_err != null) {
- throw cmd_err;
+ } finally {
+ this.fetch_accumulator = null;
+ this.search_accumulator = null;
+ token.release();
}
foreach (Command cmd in responses.keys) {
diff --git a/src/engine/nonblocking/nonblocking-mutex.vala b/src/engine/nonblocking/nonblocking-mutex.vala
index 0030138ca..616d8aa3e 100644
--- a/src/engine/nonblocking/nonblocking-mutex.vala
+++ b/src/engine/nonblocking/nonblocking-mutex.vala
@@ -1,9 +1,9 @@
/*
- * Copyright 2016 Software Freedom Conservancy Inc.
- * Copyright 2018 Michael Gratton <mike vee net>
+ * Copyright © 2016 Software Freedom Conservancy Inc.
+ * Copyright © 2018-2021 Michael Gratton <mike vee net>
*
* This software is licensed under the GNU Lesser General Public License
- * (version 2.1 or later). See the COPYING file in this distribution.
+ * (version 2.1 or later). See the COPYING file in this distribution.
*/
/**
@@ -12,16 +12,65 @@
* Two methods can be used for executing code protected by this
* mutex. The easiest is to create a {@link CriticalSection} delegate
* and pass it to {@link execute_locked}. This will manage acquiring
- * the lock as needed. The lower-level method is to call {@link
- * claim_async}, execute the critical section, then ensure {@link
- * release} is always called afterwards.
+ * the lock as needed. The lower-level method is to call {@link claim}
+ * to claim a token, execute the critical section, then ensure {@link
+ * Token.release} is called on the token afterwards.
*
* This class is ''not'' thread safe and should only be used by
* asynchronous tasks.
*/
public class Geary.Nonblocking.Mutex : BaseObject {
- public const int INVALID_TOKEN = -1;
+
+ private const int INVALID_TOKEN = -1;
+
+
+ /**
+ * The object returned when claiming a mutex.
+ *
+ * To release the mutex, return the token instance by calling the
+ * token's {@link release} method or passing it to {@link
+ * Mutex.release} on the mutex it was obtained from.
+ */
+ public class Token : GLib.Object {
+
+ /** The mutex this token was obtained from. */
+ public Mutex owner { get; private set; }
+
+ /** Internal token identifier. */
+ internal int id { get; private set; }
+
+ internal Token(int id, Mutex owner) {
+ this.id = id;
+ this.owner = owner;
+ }
+
+ /**
+ * Releases the lock at the end of executing a critical section.
+ *
+ * It is essential this method (or {@link Mutex.release} is
+ * called after the critical section has executed, else the
+ * lock will not be released.
+ *
+ * The token will be modified by this call, calling it again
+ * will have no effect.
+ *
+ * @see Mutex.release
+ */
+ public void release() {
+ try {
+ this.owner.release(this);
+ } catch (GLib.Error err) {
+ warning("Error releasing token: %s", err.message);
+ }
+ }
+
+ /** Invalidates the token after use.*/
+ internal void invalidate() {
+ this.id = INVALID_TOKEN;
+ }
+
+ }
/** A delegate that can be executed by this lock. */
public delegate void CriticalSection() throws GLib.Error;
@@ -52,15 +101,15 @@ public class Geary.Nonblocking.Mutex : BaseObject {
* //target//.
*/
public async void execute_locked(Mutex.CriticalSection target,
- Cancellable? cancellable = null)
- throws Error {
- int token = yield claim_async(cancellable);
+ GLib.Cancellable? cancellable = null)
+ throws GLib.Error {
+ var token = yield claim(cancellable);
try {
target();
} finally {
try {
- release(ref token);
- } catch (Error err) {
+ release(token);
+ } catch (GLib.Error err) {
debug("Mutex error releasing token: %s", err.message);
}
}
@@ -75,40 +124,49 @@ public class Geary.Nonblocking.Mutex : BaseObject {
* @return A token which must be passed to {@link release} when
* the critical section has completed executing.
*/
- public async int claim_async(Cancellable? cancellable = null) throws Error {
+ public async Token claim(GLib.Cancellable? cancellable = null)
+ throws GLib.Error {
for (;;) {
if (!locked) {
locked = true;
do {
- locked_token = next_token++;
+ this.locked_token = this.next_token++;
} while (locked_token == INVALID_TOKEN);
- return locked_token;
+ return new Token(locked_token, this);
}
- yield spinlock.wait_async(cancellable);
+ yield this.spinlock.wait_async(cancellable);
}
}
/**
* Releases the lock at the end of executing a critical section.
*
- * The token returned by {@link claim_async} must be supplied as a
- * parameter. It will be modified by this call so it can't be
+ * The token returned by {@link claim} must be supplied as a
+ * parameter. It will be modified by this call so it can't be
* reused.
*
- * Throws IOError.INVALID_ARGUMENT if the token was not the one
- * returned by claim_async.
+ * Throws {@link GLib.IOError.INVALID_ARGUMENT} if the token was
+ * not the one returned by {@link claim}.
+ *
+ * @see Token.release
*/
- public void release(ref int token) throws Error {
- if (token != locked_token || token == INVALID_TOKEN)
- throw new IOError.INVALID_ARGUMENT("Token %d is not the lock token", token);
+ public void release(Token token) throws GLib.Error {
+ if (token.id != this.locked_token ||
+ token.id == INVALID_TOKEN ||
+ token.owner != this) {
+ throw new GLib.IOError.INVALID_ARGUMENT(
+ "Token %d is not the lock token", token.id
+ );
+ }
+
+ token.invalidate();
- locked = false;
- token = INVALID_TOKEN;
- locked_token = INVALID_TOKEN;
+ this.locked = false;
+ this.locked_token = INVALID_TOKEN;
- spinlock.notify();
+ this.spinlock.notify();
}
}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
