nettika/geary
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Geary.App.ConversationMonitor: Fix re-entrancy issues

Browse source 
Avoid some issues if a monitor is closed as it is still being opened,
clean up the API and implementation a bit.
This commit is contained in:
Michael Gratton 2019-11-05 09:30:15 +11:00 • committed by Michael James Gratton
parent 800e53bc50
commit c002285fef
3 changed files with 65 additions and 57 deletions
Download patch file Download diff file Expand all files Collapse all files

10
src/client/components/main-window.vala
View file

@ -429,7 +429,6 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
this.conversations = new Geary.App.ConversationMonitor(
to_select,
NO_DELAY,
// Include fields for the conversation viewer as well so
// conversations can be displayed without having to go
// back to the db
@ -1035,11 +1034,12 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
to_open.conversations_added.connect(on_conversation_count_changed);
to_open.conversations_removed.connect(on_conversation_count_changed);
to_open.start_monitoring_async.begin(
to_open.start_monitoring.begin(
NO_DELAY,
cancellable,
(obj, res) => {
try {
to_open.start_monitoring_async.end(res);
to_open.start_monitoring.end(res);
} catch (GLib.Error err) {
handle_error(to_open.base_folder.account.information, err);
}
@ -1055,11 +1055,11 @@ public class MainWindow : Gtk.ApplicationWindow, Geary.BaseInterface {
to_close.conversations_added.disconnect(on_conversation_count_changed);
to_close.conversations_removed.disconnect(on_conversation_count_changed);
to_close.stop_monitoring_async.begin(
to_close.stop_monitoring.begin(
null,
(obj, res) => {
try {
to_close.stop_monitoring_async.end(res);
to_close.stop_monitoring.end(res);
} catch (GLib.Error err) {
warning(
"Error closing conversation monitor %s: %s",

75
src/engine/app/app-conversation-monitor.vala
View file

@ -20,7 +20,7 @@
* conversations can be loaded afterwards as needed.
*
* When monitoring starts via a call to {@link
* start_monitoring_async}, the folder will perform an initial
* start_monitoring}, the folder will perform an initial
* //scan// of messages in the base folder and load conversation load
* based on that. Increasing {@link min_window_count} will cause
* additional scan operations to be executed as needed to fill the new
@ -138,9 +138,8 @@ public class Geary.App.ConversationMonitor : BaseObject {
internal bool fill_complete { get; set; default = false; }
private Geary.Email.Field required_fields;
private Geary.Folder.OpenFlags open_flags;
private ConversationOperationQueue queue = null;
private Cancellable? operation_cancellable = null;
private ConversationOperationQueue queue;
private GLib.Cancellable operation_cancellable = new GLib.Cancellable();
// Set of known, in-folder emails, explicitly loaded for the
// monitor's window. This exists purely to support the window_size
@ -276,19 +275,18 @@ public class Geary.App.ConversationMonitor : BaseObject {
* Creates a conversation monitor for the given folder.
*
* @param base_folder a Folder to monitor for conversations
* @param open_flags See {@link Geary.Folder}
* @param required_fields See {@link Geary.Folder}
* @param min_window_count Minimum number of conversations that will be loaded
*/
public ConversationMonitor(Folder base_folder,
Folder.OpenFlags open_flags,
Email.Field required_fields,
int min_window_count) {
this.base_folder = base_folder;
this.open_flags = open_flags;
this.required_fields = required_fields | REQUIRED_FIELDS;
this._min_window_count = min_window_count;
this.conversations = new ConversationSet(base_folder);
this.operation_cancellable = new Cancellable();
this.queue = new ConversationOperationQueue(this.progress_monitor);
}
/**
@ -301,16 +299,19 @@ public class Geary.App.ConversationMonitor : BaseObject {
* The //cancellable// parameter will be used when opening the
* folder, but not subsequently when scanning for new messages. To
* cancel any such operations, simply close the monitor via {@link
* stop_monitoring_async}.
* stop_monitoring}.
*
* @param open_flags See {@link Geary.Folder}
* @param cancellable Passed to the folder open operation
*/
public async bool start_monitoring_async(Cancellable? cancellable = null)
throws Error {
public async bool start_monitoring(Folder.OpenFlags open_flags,
GLib.Cancellable? cancellable)
throws GLib.Error {
if (this.is_monitoring)
return false;
// Set early yield to guard against reentrancy
this.is_monitoring = true;
this.operation_cancellable = new Cancellable();
this.base_folder.email_appended.connect(on_folder_email_appended);
this.base_folder.email_inserted.connect(on_folder_email_inserted);
@ -323,25 +324,37 @@ public class Geary.App.ConversationMonitor : BaseObject {
this.base_folder.account.email_removed.connect(on_account_email_removed);
this.base_folder.account.email_flags_changed.connect(on_account_email_flags_changed);
this.queue = new ConversationOperationQueue(this.progress_monitor);
this.queue.operation_error.connect(on_operation_error);
this.queue.add(new FillWindowOperation(this));
// Take the union of the two cancellables so that of the
// monitor is closed while it is opening, the folder open is
// also cancelled
GLib.Cancellable opening = new GLib.Cancellable();
if (cancellable != null) {
cancellable.cancelled.connect(() => opening.cancel());
}
this.operation_cancellable.cancelled.connect(() => opening.cancel());
try {
yield this.base_folder.open_async(open_flags, cancellable);
yield this.base_folder.open_async(open_flags, opening);
} catch (Error err) {
try {
yield stop_monitoring_internal(false, null);
} catch (Error stop_error) {
warning(
"Error cleaning up after folder open error: %s", err.message
);
if (this.is_monitoring) {
try {
yield stop_monitoring_internal(false, null);
} catch (Error stop_error) {
warning(
"Error cleaning up after folder open error: %s", err.message
);
}
throw err;
}
throw err;
}
// Now the folder is open, start the queue running
this.queue.run_process_async.begin();
if (this.is_monitoring) {
this.queue.run_process_async.begin();
}
return true;
}
@ -356,11 +369,13 @@ public class Geary.App.ConversationMonitor : BaseObject {
* internal monitor operations to complete, but will not prevent
* attempts to close the base folder.
*/
public async bool stop_monitoring_async(Cancellable? cancellable) throws Error {
if (!is_monitoring)
return false;
return yield stop_monitoring_internal(true, cancellable);
public async bool stop_monitoring(GLib.Cancellable? cancellable)
throws GLib.Error {
bool is_closing = false;
if (this.is_monitoring) {
is_closing = yield stop_monitoring_internal(true, cancellable);
}
return is_closing;
}
/**
@ -603,7 +618,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
email_flags_changed(conversation, email);
}
private async bool stop_monitoring_internal(bool close_folder,
private async bool stop_monitoring_internal(bool folder_was_opened,
Cancellable? cancellable)
throws Error {
// set now to prevent reentrancy during yield or signal
@ -631,19 +646,15 @@ public class Geary.App.ConversationMonitor : BaseObject {
} catch (Error err) {
close_err = err;
}
this.queue = null;
this.operation_cancellable = null;
bool closing = false;
if (close_folder) {
if (folder_was_opened) {
try {
// Always close the folder to prevent open leaks
closing = yield this.base_folder.close_async(null);
} catch (Error err) {
warning("Unable to close monitored folder %s: %s",
this.base_folder.to_string(), err.message);
close_err = err;
}
}

37
test/engine/app/app-conversation-monitor-test.vala
View file

@ -64,7 +64,7 @@ class Geary.App.ConversationMonitorTest : TestCase {
public void start_stop_monitoring() throws Error {
ConversationMonitor monitor = new ConversationMonitor(
this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10
this.base_folder, Email.Field.NONE, 10
);
Cancellable test_cancellable = new Cancellable();
@ -73,27 +73,24 @@ class Geary.App.ConversationMonitorTest : TestCase {
monitor.scan_started.connect(() => { saw_scan_started = true; });
monitor.scan_completed.connect(() => { saw_scan_completed = true; });
this.base_folder.expect_call(
"open_async",
{ MockObject.int_arg(Folder.OpenFlags.NONE), test_cancellable }
);
this.base_folder.expect_call("open_async");
this.base_folder.expect_call("list_email_by_id_async");
this.base_folder.expect_call("close_async");
monitor.start_monitoring_async.begin(
test_cancellable, (obj, res) => { async_complete(res); }
monitor.start_monitoring.begin(
NONE, test_cancellable, (obj, res) => { async_complete(res); }
);
monitor.start_monitoring_async.end(async_result());
monitor.start_monitoring.end(async_result());
// Process all of the async tasks arising from the open
while (this.main_loop.pending()) {
this.main_loop.iteration(true);
}
monitor.stop_monitoring_async.begin(
monitor.stop_monitoring.begin(
test_cancellable, (obj, res) => { async_complete(res); }
);
monitor.stop_monitoring_async.end(async_result());
monitor.stop_monitoring.end(async_result());
assert_true(saw_scan_started, "scan_started not fired");
assert_true(saw_scan_completed, "scan_completed not fired");
@ -103,18 +100,18 @@ class Geary.App.ConversationMonitorTest : TestCase {
public void open_error() throws Error {
ConversationMonitor monitor = new ConversationMonitor(
this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10
this.base_folder, Email.Field.NONE, 10
);
ExpectedCall open = this.base_folder
.expect_call("open_async")
.throws(new EngineError.SERVER_UNAVAILABLE("Mock error"));
monitor.start_monitoring_async.begin(
null, (obj, res) => { async_complete(res); }
monitor.start_monitoring.begin(
NONE, null, (obj, res) => { async_complete(res); }
);
try {
monitor.start_monitoring_async.end(async_result());
monitor.start_monitoring.end(async_result());
assert_not_reached();
} catch (Error err) {
assert_error(open.throw_error, err);
@ -248,10 +245,10 @@ class Geary.App.ConversationMonitorTest : TestCase {
// Close the monitor to cancel the final load so it does not
// error out during later tests
this.base_folder.expect_call("close_async");
monitor.stop_monitoring_async.begin(
monitor.stop_monitoring.begin(
null, (obj, res) => { async_complete(res); }
);
monitor.stop_monitoring_async.end(async_result());
monitor.stop_monitoring.end(async_result());
}
public void external_folder_message_appended() throws Error {
@ -387,7 +384,7 @@ class Geary.App.ConversationMonitorTest : TestCase {
Gee.MultiMap<Email,FolderPath>[] related_paths = {})
throws Error {
ConversationMonitor monitor = new ConversationMonitor(
this.base_folder, Folder.OpenFlags.NONE, Email.Field.NONE, 10
this.base_folder, Email.Field.NONE, 10
);
Cancellable test_cancellable = new Cancellable();
@ -479,10 +476,10 @@ class Geary.App.ConversationMonitorTest : TestCase {
}
}
monitor.start_monitoring_async.begin(
test_cancellable, (obj, res) => { async_complete(res); }
monitor.start_monitoring.begin(
NONE, test_cancellable, (obj, res) => { async_complete(res); }
);
monitor.start_monitoring_async.end(async_result());
monitor.start_monitoring.end(async_result());
if (base_folder_email.length == 0) {
wait_for_call(list_call);