From 880df8691f90aebb393699e3f7c2b4e0faac2510 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Tue, 17 Dec 2019 12:43:55 +0100 Subject: [PATCH 01/57] Start towards clearing old messages beyond prefetch window - Add custom optimised query to ImapDB.Folder and use in synchronizer --- src/engine/imap-db/imap-db-folder.vala | 28 +++++++++++++++++++ .../imap-engine-account-synchronizer.vala | 8 +++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 9b854173..381bafde 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -882,6 +882,34 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { }, cancellable); } + public async void detach_emails_before_timestamp(DateTime cutoff, + Cancellable? cancellable) throws Error { + warning("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); + + yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { + // Query was found to be faster than other approaches. MessageLocationTable.ordering + // isn't relied on due to IMAP folder UIDs not guaranteed to be in order. + StringBuilder sql = new StringBuilder(); + sql.append(""" + DELETE FROM MessageLocationTable + WHERE folder_id = ? + AND message_id IN ( + SELECT id + FROM MessageTable + INDEXED BY MessageTableInternalDateTimeTIndex + WHERE internaldate_time_t < ? + ) + """); + + Db.Statement stmt = cx.prepare(sql.str); + stmt.bind_rowid(0, this.folder_id); + stmt.bind_int64(1, cutoff.to_unix()); + stmt.exec(cancellable); + + return Db.TransactionOutcome.COMMIT; + }, cancellable); + } + public async void mark_email_async(Gee.Collection to_mark, Geary.EmailFlags? flags_to_add, Geary.EmailFlags? flags_to_remove, Cancellable? cancellable) throws Error { diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 5d399f24..f0f2b84c 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -243,9 +243,15 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { prefetch_max_epoch = this.sync_max_epoch; } + ImapDB.Folder local_folder = ((MinimalFolder) this.folder).local_folder; + + // Detach older emails outside the prefetch window + if (this.account.information.prefetch_period_days >= 0) { + yield local_folder.detach_emails_before_timestamp(prefetch_max_epoch, cancellable); + } + // get oldest local email and its time, as well as number // of messages in local store - ImapDB.Folder local_folder = ((MinimalFolder) this.folder).local_folder; Gee.List? list = yield local_folder.list_email_by_id_async( null, 1, From dbb1d783a164059256f6de1141d56bbc634459fe Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 18 Dec 2019 12:08:24 +0100 Subject: [PATCH 02/57] Demote log message level --- src/engine/imap-db/imap-db-folder.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 381bafde..489dc908 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -884,7 +884,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { public async void detach_emails_before_timestamp(DateTime cutoff, Cancellable? cancellable) throws Error { - warning("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); + debug("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { // Query was found to be faster than other approaches. MessageLocationTable.ordering From 7684c33830ab5a286e5095164987b5335ff92726 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 11:00:01 +0100 Subject: [PATCH 03/57] Track all windows going out of focus --- .../application/application-controller.vala | 28 +++++++++++++++++++ .../application/application-main-window.vala | 9 ++++++ src/client/composer/composer-window.vala | 9 ++++++ 3 files changed, 46 insertions(+) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 87f70a44..17b23295 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -18,6 +18,7 @@ internal class Application.Controller : Geary.BaseObject { private const uint MAX_AUTH_ATTEMPTS = 3; + private const uint CLEANUP_WORK_AFTER_IDLE_BACKGROUND_MINUTES = 60 * 24; /** Determines if conversations can be trashed from the given folder. */ public static bool does_folder_support_trash(Geary.Folder? target) { @@ -91,6 +92,9 @@ internal class Application.Controller : Geary.BaseObject { // Requested mailto composers not yet fullfulled private Gee.List pending_mailtos = new Gee.ArrayList(); + // Timeout to do work in idle after all windows have been sent to the background + private Geary.TimeoutManager all_windows_backgrounded_timeout = null; + /** * Emitted when an account is added or is enabled. @@ -1393,6 +1397,30 @@ internal class Application.Controller : Geary.BaseObject { } } + // Track a window receiving focus, for idle background work + public void window_focus_in() { + if (this.all_windows_backgrounded_timeout != null) { + this.all_windows_backgrounded_timeout.reset(); + this.all_windows_backgrounded_timeout = null; + } + } + + // Track a window going unfocused, for idle background work + public void window_focus_out() { + this.all_windows_backgrounded_timeout = new Geary.TimeoutManager.seconds(CLEANUP_WORK_AFTER_IDLE_BACKGROUND_MINUTES * 60, on_unfocused_idle); + this.all_windows_backgrounded_timeout.start(); + } + + private void on_unfocused_idle() { + // Schedule later, catching cases where work should occur later while still in background + this.all_windows_backgrounded_timeout = null; + window_focus_out(); + + debug("Checking for backgrounded idle work"); + foreach (AccountContext context in this.accounts.values) { + } + } + /** Displays a composer on the last active main window. */ internal void show_composer(Composer.Widget composer, Gee.Collection? refers_to, diff --git a/src/client/application/application-main-window.vala b/src/client/application/application-main-window.vala index d605e027..b57537d1 100644 --- a/src/client/application/application-main-window.vala +++ b/src/client/application/application-main-window.vala @@ -496,6 +496,15 @@ public class Application.MainWindow : // Window actions add_action_entries(MainWindow.WINDOW_ACTIONS, this); + this.focus_in_event.connect((w, e) => { + application.controller.window_focus_in(); + return true; + }); + this.focus_out_event.connect((w, e) => { + application.controller.window_focus_out(); + return true; + }); + setup_layout(application.config); on_change_orientation(); diff --git a/src/client/composer/composer-window.vala b/src/client/composer/composer-window.vala index 1608fbd0..3f4c6f7a 100644 --- a/src/client/composer/composer-window.vala +++ b/src/client/composer/composer-window.vala @@ -48,6 +48,15 @@ public class Composer.Window : Gtk.ApplicationWindow, Container { set_titlebar(this.composer.header); } + this.focus_in_event.connect((w, e) => { + application.controller.window_focus_in(); + return true; + }); + this.focus_out_event.connect((w, e) => { + application.controller.window_focus_out(); + return true; + }); + show(); set_position(Gtk.WindowPosition.CENTER); } From 52bd48bab5f520d2ef68f912c085f27565315920 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 11:04:51 +0100 Subject: [PATCH 04/57] Signalling for locally removed messages --- src/engine/api/geary-account.vala | 10 ++++++++++ src/engine/api/geary-folder.vala | 6 ++++++ src/engine/app/app-conversation-monitor.vala | 2 ++ src/engine/app/app-search-folder.vala | 2 ++ .../imap-engine/imap-engine-generic-account.vala | 7 +++++++ 5 files changed, 27 insertions(+) diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index 37ade9e3..30bb1d1b 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -236,6 +236,11 @@ public abstract class Geary.Account : BaseObject, Logging.Source { */ public signal void email_removed(Geary.Folder folder, Gee.Collection ids); + /** + * Fired when emails are removed from a local folder in this account. + */ + public signal void email_locally_removed(Geary.Folder folder, Gee.Collection ids); + /** * Fired when one or more emails have been locally saved to a folder with * the full set of Fields. @@ -549,6 +554,11 @@ public abstract class Geary.Account : BaseObject, Logging.Source { email_removed(folder, ids); } + /** Fires a {@link email_locally_removed} signal. */ + protected virtual void notify_email_locally_removed(Geary.Folder folder, Gee.Collection ids) { + email_locally_removed(folder, ids); + } + /** Fires a {@link email_locally_complete} signal. */ protected virtual void notify_email_locally_complete(Geary.Folder folder, Gee.Collection ids) { diff --git a/src/engine/api/geary-folder.vala b/src/engine/api/geary-folder.vala index 2602f87e..a219c282 100644 --- a/src/engine/api/geary-folder.vala +++ b/src/engine/api/geary-folder.vala @@ -364,6 +364,12 @@ public abstract class Geary.Folder : BaseObject, Logging.Source { */ public signal void email_removed(Gee.Collection ids); + + /** + * Fired when emails are removed from the local folder. + */ + public signal void email_locally_removed(Gee.Collection ids); + /** * Fired when the total count of email in a folder has changed in any way. * diff --git a/src/engine/app/app-conversation-monitor.vala b/src/engine/app/app-conversation-monitor.vala index a4de164d..7b87b783 100644 --- a/src/engine/app/app-conversation-monitor.vala +++ b/src/engine/app/app-conversation-monitor.vala @@ -320,6 +320,7 @@ public class Geary.App.ConversationMonitor : BaseObject { this.base_folder.email_inserted.connect(on_folder_email_inserted); this.base_folder.email_locally_complete.connect(on_folder_email_complete); this.base_folder.email_removed.connect(on_folder_email_removed); + this.base_folder.email_locally_removed.connect(on_folder_email_removed); this.base_folder.opened.connect(on_folder_opened); this.base_folder.account.email_appended.connect(on_account_email_appended); this.base_folder.account.email_inserted.connect(on_account_email_inserted); @@ -653,6 +654,7 @@ public class Geary.App.ConversationMonitor : BaseObject { this.base_folder.email_inserted.disconnect(on_folder_email_inserted); this.base_folder.email_locally_complete.disconnect(on_folder_email_complete); this.base_folder.email_removed.disconnect(on_folder_email_removed); + this.base_folder.email_locally_removed.disconnect(on_folder_email_removed); this.base_folder.opened.disconnect(on_folder_opened); this.base_folder.account.email_appended.disconnect(on_account_email_appended); this.base_folder.account.email_inserted.disconnect(on_account_email_inserted); diff --git a/src/engine/app/app-search-folder.vala b/src/engine/app/app-search-folder.vala index 5867dd0e..bdc3f743 100644 --- a/src/engine/app/app-search-folder.vala +++ b/src/engine/app/app-search-folder.vala @@ -129,6 +129,7 @@ public class Geary.App.SearchFolder : account.folders_special_type.connect(on_folders_special_type); account.email_locally_complete.connect(on_email_locally_complete); account.email_removed.connect(on_account_email_removed); + account.email_locally_removed.connect(on_account_email_removed); new_contents(); @@ -142,6 +143,7 @@ public class Geary.App.SearchFolder : account.folders_special_type.disconnect(on_folders_special_type); account.email_locally_complete.disconnect(on_email_locally_complete); account.email_removed.disconnect(on_account_email_removed); + account.email_locally_removed.disconnect(on_account_email_removed); } /** diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 0605c5de..0cfc9ae1 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -764,6 +764,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { folder.email_appended.connect(notify_email_appended); folder.email_inserted.connect(notify_email_inserted); folder.email_removed.connect(notify_email_removed); + folder.email_locally_removed.connect(notify_email_locally_removed); folder.email_locally_complete.connect(notify_email_locally_complete); folder.email_flags_changed.connect(notify_email_flags_changed); } @@ -773,6 +774,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { folder.email_appended.disconnect(notify_email_appended); folder.email_inserted.disconnect(notify_email_inserted); folder.email_removed.disconnect(notify_email_removed); + folder.email_locally_removed.disconnect(notify_email_locally_removed); folder.email_locally_complete.disconnect(notify_email_locally_complete); folder.email_flags_changed.disconnect(notify_email_flags_changed); } @@ -797,6 +799,11 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { schedule_unseen_update(folder); } + /** {@inheritDoc} */ + protected override void notify_email_locally_removed(Geary.Folder folder, Gee.Collection ids) { + base.notify_email_locally_removed(folder, ids); + } + /** {@inheritDoc} */ protected override void notify_email_flags_changed(Geary.Folder folder, Gee.Map flag_map) { From cecf66df08c488a726a9379fc72a0694de4f1e23 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 11:31:25 +0100 Subject: [PATCH 05/57] Track and notify messages detached during cleanup Split detach query into two so that messages identifiers can be captured which are then used to signal through that the folder has locally removed messages --- src/engine/imap-db/imap-db-folder.vala | 49 ++++++++++++++++--- .../imap-engine-account-synchronizer.vala | 5 +- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 489dc908..b6ff465c 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -882,16 +882,18 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { }, cancellable); } - public async void detach_emails_before_timestamp(DateTime cutoff, + public async Gee.Collection? detach_emails_before_timestamp(DateTime cutoff, Cancellable? cancellable) throws Error { debug("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); + Gee.Collection? deleted_ids = null; yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { - // Query was found to be faster than other approaches. MessageLocationTable.ordering - // isn't relied on due to IMAP folder UIDs not guaranteed to be in order. + // MessageLocationTable.ordering isn't relied on due to IMAP folder + // UIDs not guaranteed to be in order. StringBuilder sql = new StringBuilder(); sql.append(""" - DELETE FROM MessageLocationTable + SELECT id, message_id, ordering + FROM MessageLocationTable WHERE folder_id = ? AND message_id IN ( SELECT id @@ -902,12 +904,45 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { """); Db.Statement stmt = cx.prepare(sql.str); - stmt.bind_rowid(0, this.folder_id); + stmt.bind_rowid(0, folder_id); stmt.bind_int64(1, cutoff.to_unix()); - stmt.exec(cancellable); - return Db.TransactionOutcome.COMMIT; + Db.Result results = stmt.exec(cancellable); + + StringBuilder? ids_sql_sublist = null; + while (!results.finished) { + if (ids_sql_sublist == null) { + deleted_ids = new Gee.ArrayList(); + ids_sql_sublist = new StringBuilder(); + } else { + ids_sql_sublist.append(","); + } + + deleted_ids.add(new ImapDB.EmailIdentifier(results.int64_at(1), new Imap.UID(results.int64_at(2)))); + ids_sql_sublist.append(results.rowid_at(0).to_string()); + + results.next(cancellable); + } + + if (deleted_ids != null) { + sql = new StringBuilder(); + sql.append(""" + DELETE FROM MessageLocationTable + WHERE id IN ( + """); + sql.append(ids_sql_sublist.str); + sql.append(")"); + stmt = cx.prepare(sql.str); + + stmt.exec(cancellable); + + return Db.TransactionOutcome.COMMIT; + } else { + return Db.TransactionOutcome.DONE; + } }, cancellable); + + return deleted_ids; } public async void mark_email_async(Gee.Collection to_mark, diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index f0f2b84c..654ca039 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -247,7 +247,10 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { // Detach older emails outside the prefetch window if (this.account.information.prefetch_period_days >= 0) { - yield local_folder.detach_emails_before_timestamp(prefetch_max_epoch, cancellable); + Gee.Collection? detached_ids = yield local_folder.detach_emails_before_timestamp(prefetch_max_epoch, cancellable); + if (detached_ids != null) { + this.folder.email_locally_removed(detached_ids); + } } // get oldest local email and its time, as well as number From 15561a1108afc9962672d9b7c1973da838d1ed37 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 11:33:48 +0100 Subject: [PATCH 06/57] Update local folder message count property during cleanup Likely WIP with other properties to be added --- src/engine/imap-db/imap-db-folder.vala | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index b6ff465c..06ee87a7 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -936,6 +936,9 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { stmt.exec(cancellable); + // Update local message count + this.properties.set_status_message_count(this.properties.email_total - deleted_ids.size, true); + return Db.TransactionOutcome.COMMIT; } else { return Db.TransactionOutcome.DONE; From ff6c45c29c2b4d38988a3921e97bbad7648958cb Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 11:50:54 +0100 Subject: [PATCH 07/57] Add minor missing cleanup this.gc would previously never have been released if GC reap wasn't recommended and run --- src/engine/imap-db/imap-db-database.vala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 91e2f01c..f1a5e323 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -109,6 +109,8 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { if ((recommended & GC.RecommendedOperation.REAP) != 0) { // run in the background and allow application to continue running this.gc.reap_async.begin(gc_cancellable, on_reap_async_completed); + } else { + this.gc = null; } if (cancellable != null) From 584ae84e5b4e7c14e0c50fa6ca54cc8dac4ecc18 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 12:09:55 +0100 Subject: [PATCH 08/57] Run GC after emptying spam or trash --- src/engine/imap-engine/imap-engine-minimal-folder.vala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala b/src/engine/imap-engine/imap-engine-minimal-folder.vala index 923b8e71..d8937422 100644 --- a/src/engine/imap-engine/imap-engine-minimal-folder.vala +++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala @@ -1277,6 +1277,8 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport // open while processing first the flag updates then the // expunge from the remote yield this.replay_queue.checkpoint(cancellable); + + yield this._account.local.db.run_gc(cancellable); } private void check_open(string method) throws EngineError { From f7b0a0a61fa1ed221f2583d5015499402889f6dc Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 12:21:27 +0100 Subject: [PATCH 09/57] Breakout ImapDB.Database garbage collection Including: - Moved into method run_gc - Added ability to force reap (useful after old message cleanup when we don't want to wait the regular 10 day span) - Added tracking of desire to vacuum, which will later by executed when backgrounded for a period - Disabled vacuum on startup - Added stopping IMAP while vacuuming - Used ugly WIP hack of passing through account to stop IMAP --- src/engine/imap-db/imap-db-database.vala | 64 ++++++++++++++++++------ 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index f1a5e323..1eaad6e9 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -18,6 +18,8 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { /** SQLite UTF-8 collation name. */ public const string UTF8_COLLATE = "UTF8COLL"; + public bool want_background_vacuum { get; set; default = false; } + private static void utf8_transliterate_fold(Sqlite.Context context, Sqlite.Value[] values) { @@ -73,6 +75,29 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { throws Error { yield base.open(flags, cancellable); + yield run_gc(cancellable); + } + + /** + * Run garbage collection + * + * Reap should only be forced when there is known cleanup to perform and + * the interval based recommendation should be bypassed. + * + * TODO Passing of account is a WIP hack. It is currently used to both + * signify that it's an appropriate time to run a vacuum (ie. we're + * idle in the background) and provide access for stopping IMAP. + */ + public async void run_gc(Cancellable? cancellable, + bool force_reap = false, + Geary.ImapEngine.GenericAccount? account = null) + throws Error { + + if (this.gc != null) { + message("GC abandoned, possibly already running"); + return; + } + // Tie user-supplied Cancellable to internal Cancellable, which is used when close() is // called if (cancellable != null) @@ -89,24 +114,35 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { // VACUUM needs to execute in the foreground with the user given a busy prompt (and cannot // be run at the same time as REAP) if ((recommended & GC.RecommendedOperation.VACUUM) != 0) { - if (!vacuum_monitor.is_in_progress) - vacuum_monitor.notify_start(); + if (account != null) { + this.want_background_vacuum = false; + yield account.imap.stop(gc_cancellable); - try { - yield this.gc.vacuum_async(gc_cancellable); - } catch (Error err) { - message( - "Vacuum of IMAP database %s failed: %s", this.path, err.message - ); - throw err; - } finally { - if (vacuum_monitor.is_in_progress) - vacuum_monitor.notify_finish(); - } + if (!vacuum_monitor.is_in_progress) + vacuum_monitor.notify_start(); + + try { + yield this.gc.vacuum_async(gc_cancellable); + } catch (Error err) { + message( + "Vacuum of IMAP database %s failed: %s", this.path, err.message + ); + throw err; + } finally { + if (vacuum_monitor.is_in_progress) + vacuum_monitor.notify_finish(); + } + + yield account.imap.start(gc_cancellable); + } else { + // Flag a vacuum to run later when we've been idle in the background + debug("Flagging desire to GC vacuum"); + this.want_background_vacuum = true; + } } // REAP can run in the background while the application is executing - if ((recommended & GC.RecommendedOperation.REAP) != 0) { + if (force_reap || (recommended & GC.RecommendedOperation.REAP) != 0) { // run in the background and allow application to continue running this.gc.reap_async.begin(gc_cancellable, on_reap_async_completed); } else { From 1e8465f5559181f7fdd0208cdc48c5b49fb36093 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 12:58:54 +0100 Subject: [PATCH 10/57] Old message cleanup after folder becomes available Simple mechanism for triggering GC reap after messages were removed during folder check on becoming available --- src/engine/imap-db/imap-db-database.vala | 22 +++++++++++++++++++ .../imap-engine-account-synchronizer.vala | 12 ++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 1eaad6e9..f91d0bea 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -50,6 +50,8 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private const int OPEN_PUMP_EVENT_LOOP_MSEC = 100; + private const uint DELAY_GC_AFTER_OLD_MESSAGE_CLEANUP_SECONDS = 30; + private ProgressMonitor upgrade_monitor; private ProgressMonitor vacuum_monitor; private bool new_db = false; @@ -57,6 +59,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private GC? gc = null; private Cancellable gc_cancellable = new Cancellable(); + private TimeoutManager gc_post_old_message_detach_timer; + // Cancellable from account synchronizer for GC after old message cleanup + private Cancellable? post_gc_cleanup_cancellable; + public Database(GLib.File db_file, GLib.File schema_dir, GLib.File attachments_path, @@ -66,6 +72,11 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { this.attachments_path = attachments_path; this.upgrade_monitor = upgrade_monitor; this.vacuum_monitor = vacuum_monitor; + + this.gc_post_old_message_detach_timer = + new TimeoutManager.seconds( + DELAY_GC_AFTER_OLD_MESSAGE_CLEANUP_SECONDS, run_gc_post_old_message_detach + ); } /** @@ -179,6 +190,17 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { base.close(cancellable); } + public void schedule_gc_after_old_messages_cleanup(Cancellable? cancellable) { + this.post_gc_cleanup_cancellable = cancellable; + this.gc_post_old_message_detach_timer.start(); + } + + public void run_gc_post_old_message_detach() { + debug("Requesting GC post old message cleanup"); + run_gc.begin(this.post_gc_cleanup_cancellable); + this.post_gc_cleanup_cancellable = null; + } + protected override void starting_upgrade(int current_version, bool new_db) { this.new_db = new_db; diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 654ca039..e5e64d84 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -71,6 +71,9 @@ private class Geary.ImapEngine.AccountSynchronizer : ) : new RefreshFolderSync(this.account, imap_folder); + if (became_available) + ((CheckFolderSync) op).old_message_detached.connect(this.old_messages_removed_during_sync); + try { this.account.queue_operation(op); } catch (Error err) { @@ -89,6 +92,13 @@ private class Geary.ImapEngine.AccountSynchronizer : } } + private void old_messages_removed_during_sync(Cancellable cancellable) { + // This is not a daily cleanup. We've detached some messages, let's GC if + // recommended. + GenericAccount account = (GenericAccount) this.account; + account.local.db.schedule_gc_after_old_messages_cleanup(cancellable); + } + private void on_account_prefetch_changed() { this.prefetch_timer.start(); } @@ -219,6 +229,7 @@ private class Geary.ImapEngine.RefreshFolderSync : FolderOperation { */ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { + public signal void old_message_detached(Cancellable cancellable); private DateTime sync_max_epoch; @@ -250,6 +261,7 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { Gee.Collection? detached_ids = yield local_folder.detach_emails_before_timestamp(prefetch_max_epoch, cancellable); if (detached_ids != null) { this.folder.email_locally_removed(detached_ids); + old_message_detached(cancellable); } } From 47bdda6af59eeee160823fb0d6227862eed1c59a Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 13:02:22 +0100 Subject: [PATCH 11/57] After GC reap check for a vacuum The idea here being if we've just reduced prefetch period, reap has detached a whole lot of messages and we want to vacuum. This check catches that vacuum recommendation, flagging it to run when the app is backgrounded and idle. --- src/engine/imap-db/imap-db-database.vala | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index f91d0bea..5e0fc0ab 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -172,6 +172,24 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { this.path, err.message); } + // Check if after reap we now want to schedule a background vacuum. The idea + // here is eg. if we've just reduced prefetch period, reap has detached a + // whole lot of messages and we want to vacuum. This check catches that + // vacuum recommendation, flagging it to run when in background. + this.gc.should_run_async.begin( + gc_cancellable, + (obj, res) => { + try { + GC.RecommendedOperation recommended = this.gc.should_run_async.end(res); + if ((recommended & GC.RecommendedOperation.VACUUM) != 0) + this.want_background_vacuum = true; + } catch (Error err) { + debug("Failed to run GC check on %s after REAP: %s", + this.path, err.message); + } + } + ); + this.gc = null; } From a2d8f1790dd3858c944c5ceb46ec5193dc02cae5 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 13:38:05 +0100 Subject: [PATCH 12/57] Fix typo backgrounded app idle timeout value --- src/client/application/application-controller.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 17b23295..9f98f091 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -18,7 +18,7 @@ internal class Application.Controller : Geary.BaseObject { private const uint MAX_AUTH_ATTEMPTS = 3; - private const uint CLEANUP_WORK_AFTER_IDLE_BACKGROUND_MINUTES = 60 * 24; + private const uint CLEANUP_WORK_AFTER_IDLE_BACKGROUND_MINUTES = 5; /** Determines if conversations can be trashed from the given folder. */ public static bool does_folder_support_trash(Geary.Folder? target) { From 609ee01e3e04c0e3b43acbed2cc8fcf5346b4660 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 14:08:52 +0100 Subject: [PATCH 13/57] Trigger idle cleanup per account when backgrounded Also handle continuation of cleanup after old messages have been detached. Flagged vacuuming is performed even when the cleanup interval hasn't been reached. The tracking of last background cleanup time in Account.Information is possibly temporary and isn't yet persisted. --- .../application/application-controller.vala | 2 ++ src/engine/api/geary-account-information.vala | 1 + src/engine/api/geary-account.vala | 8 +++++ .../imap-engine-generic-account.vala | 36 +++++++++++++++++++ test/engine/api/geary-account-mock.vala | 3 ++ 5 files changed, 50 insertions(+) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 9f98f091..cfb7d186 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -1418,6 +1418,8 @@ internal class Application.Controller : Geary.BaseObject { debug("Checking for backgrounded idle work"); foreach (AccountContext context in this.accounts.values) { + Geary.Account account = context.account; + account.app_backgrounded_cleanup.begin(context.cancellable); } } diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index cdc05eb6..60c42e7a 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -210,6 +210,7 @@ public class Geary.AccountInformation : BaseObject { default = new Gee.LinkedList(); } + public DateTime? last_backgrounded_cleanup_time { get; set; default = null; } /** * Emitted when a service has reported an authentication failure. diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index 30bb1d1b..d9100c1c 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -507,6 +507,14 @@ public abstract class Geary.Account : BaseObject, Logging.Source { return new Logging.State(this, this.information.id); } + /** + * Run account cleanup work if the appropriate interval has past since + * last execution. Alternatively if the interval has not past but vacuum + * GC has been flagged to run this will be executed. Designed to be run + * while the app is in the background and idle. + */ + public abstract async void app_backgrounded_cleanup(Cancellable? cancellable); + /** Fires a {@link opened} signal. */ protected virtual void notify_opened() { opened(); diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 0cfc9ae1..a8b8a68c 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -17,6 +17,10 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // we don't need to double check. private const int REFRESH_FOLDER_LIST_SEC = 15 * 60; + // Frequency of account cleanup work, performed when idle with the app + // backgrounded + private const uint APP_BACKGROUNDED_CLEANUP_WORK_INTERVAL_MINUTES = 60 * 24; + private const Geary.SpecialFolderType[] SUPPORTED_SPECIAL_FOLDERS = { Geary.SpecialFolderType.DRAFTS, Geary.SpecialFolderType.SENT, @@ -39,6 +43,8 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { /** Local database for the account. */ public ImapDB.Account local { get; private set; } + public signal void old_messages_background_cleanup_request(Cancellable? cancellable); + private bool open = false; private Cancellable? open_cancellable = null; private Nonblocking.Semaphore? remote_ready_lock = null; @@ -524,6 +530,36 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { return (map.size == 0) ? null : map; } + /** {@inheritDoc} */ + public override async void app_backgrounded_cleanup(Cancellable? cancellable) { + debug("Backgrounded cleanup check for %s account", this.information.display_name); + + DateTime now = new DateTime.now_local(); + DateTime? last_cleanup = this.information.last_backgrounded_cleanup_time; + + if (last_cleanup == null || + (now.difference(last_cleanup) / TimeSpan.MINUTE > APP_BACKGROUNDED_CLEANUP_WORK_INTERVAL_MINUTES)) { + // Interval check is OK, start by detaching old messages + this.information.last_backgrounded_cleanup_time = now; + this.old_messages_background_cleanup_request(cancellable); + } else if (local.db.want_background_vacuum) { + // Vacuum has been flagged as needed, run it + local.db.run_gc.begin(cancellable, false, this); + } + } + + // Continue backgrounded app cleanup work after the first phase, + // old message detachment, has completed + public void app_backgrounded_cleanup_continued(bool messages_detached, Cancellable? cancellable) { + if (messages_detached) { + // Kick off GC, forcing reap as we've removed messages, allowing vacuum + local.db.run_gc.begin(cancellable, true, this); + } else { + // Kick off GC, allowing vacuum + local.db.run_gc.begin(cancellable, false, this); + } + } + /** * Constructs a set of folders and adds them to the account. * diff --git a/test/engine/api/geary-account-mock.vala b/test/engine/api/geary-account-mock.vala index 9f074d62..77ca8287 100644 --- a/test/engine/api/geary-account-mock.vala +++ b/test/engine/api/geary-account-mock.vala @@ -274,4 +274,7 @@ public class Geary.MockAccount : Account, MockObject { ); } + public override async void app_backgrounded_cleanup(Cancellable? cancellable) { + + } } From f7ee234addc4e17c3db140823e2a12402b3319a9 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 14:15:07 +0100 Subject: [PATCH 14/57] Minor tidy on last commit --- src/engine/imap-engine/imap-engine-generic-account.vala | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index a8b8a68c..b1f673c8 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -551,13 +551,8 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // Continue backgrounded app cleanup work after the first phase, // old message detachment, has completed public void app_backgrounded_cleanup_continued(bool messages_detached, Cancellable? cancellable) { - if (messages_detached) { - // Kick off GC, forcing reap as we've removed messages, allowing vacuum - local.db.run_gc.begin(cancellable, true, this); - } else { - // Kick off GC, allowing vacuum - local.db.run_gc.begin(cancellable, false, this); - } + // Kick off GC, allowing vacuum and forcing reap if we've removed messages + local.db.run_gc.begin(cancellable, messages_detached, this); } /** From 9a5871381cbaa13fcfa01ffda8cb1c28b82a3a5a Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 14:19:07 +0100 Subject: [PATCH 15/57] Clean old messages before continuing bg idle work Monitor folder checking operations so we can perform garbage collection once any message detaching has finished. WIP as this is a second mechanism for triggering GC after folder checks have completed. Need to discuss. --- .../imap-engine-account-synchronizer.vala | 106 +++++++++++++++++- 1 file changed, 102 insertions(+), 4 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index e5e64d84..6fcc583a 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -16,6 +16,7 @@ private class Geary.ImapEngine.AccountSynchronizer : private DateTime max_epoch = new DateTime( new TimeZone.local(), 2000, 1, 1, 0, 0, 0.0 ); + private OldMessageCleaner old_message_cleaner; public AccountSynchronizer(GenericAccount account) { @@ -25,6 +26,7 @@ private class Geary.ImapEngine.AccountSynchronizer : ); this.account.information.notify["prefetch-period-days"].connect(on_account_prefetch_changed); + this.account.old_messages_background_cleanup_request.connect(old_messages_background_cleanup); this.account.folders_available_unavailable.connect(on_folders_updated); this.account.folders_contents_altered.connect(on_folders_contents_altered); } @@ -92,11 +94,24 @@ private class Geary.ImapEngine.AccountSynchronizer : } } + private void old_messages_background_cleanup(Cancellable? cancellable) { + if (this.account.is_open()) { + this.old_message_cleaner = new OldMessageCleaner(this.account, this.max_epoch); + this.old_message_cleaner.run(); + this.old_message_cleaner.completed.connect((messages_detached) => { + this.old_message_cleaner = null; + this.account.app_backgrounded_cleanup_continued(messages_detached, cancellable); + }); + } + } + private void old_messages_removed_during_sync(Cancellable cancellable) { - // This is not a daily cleanup. We've detached some messages, let's GC if - // recommended. - GenericAccount account = (GenericAccount) this.account; - account.local.db.schedule_gc_after_old_messages_cleanup(cancellable); + if (this.old_message_cleaner == null) { + // This is not a daily cleanup. We've detached some messages, let's GC if + // recommended. + GenericAccount account = (GenericAccount) this.account; + account.local.db.schedule_gc_after_old_messages_cleanup(cancellable); + } } private void on_account_prefetch_changed() { @@ -392,3 +407,86 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { } } + + +private class Geary.ImapEngine.OldMessageCleaner: BaseObject, Logging.Source { + + public bool messages_detached {get; set; default = false; } + + private GenericAccount account; + private DateTime sync_max_epoch; + private List operations = + new List(); + + public signal void completed(bool messages_detached); + + /** {@inheritDoc} */ + public Logging.Flag logging_flags { + get; protected set; default = Logging.Flag.ALL; + } + + /** {@inheritDoc} */ + public Logging.Source? logging_parent { + get { return this.account; } + } + + internal OldMessageCleaner(GenericAccount account, + DateTime sync_max_epoch) { + this.sync_max_epoch = sync_max_epoch; + this.account = account; + } + + public void run() { + foreach (Folder folder in this.account.list_folders()) { + // Only sync folders that: + // 1. Can actually be opened (i.e. are selectable) + // 2. Are remote backed + // + // All this implies the folder must be a MinimalFolder and + // we do require that for syncing at the moment anyway, + // but keep the tests in for that one glorious day where + // we can just use a generic folder. + MinimalFolder? imap_folder = folder as MinimalFolder; + if (imap_folder != null && + folder.properties.is_openable.is_possible() && + !folder.properties.is_local_only && + !folder.properties.is_virtual) { + + CheckFolderSync op = + new CheckFolderSync( + this.account, imap_folder, this.sync_max_epoch + ); + try { + this.account.queue_operation(op); + this.operations.append(op); + + // Let daily cleanup monitor know that we detached some messages + // (which will result in a 'forced' GC reap) + op.old_message_detached.connect(() => { + this.messages_detached = true; + }); + op.completed.connect(() => { + this.operations.remove(op); + if (operations.length() == 0) { + this.completed(this.messages_detached); + } + + }); + } catch (Error err) { + warning("Failed to queue sync operation: %s", err.message); + } + } + } + } + + /** {@inheritDoc} */ + public virtual Logging.State to_logging_state() { + return new Logging.State( + this, + "%s, %s", + this.account.information.id, + this.sync_max_epoch.to_string() + ); + } + +} From 2b20f1cc4dae768ce75a03224c0f0a0e679c63dc Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 14:22:51 +0100 Subject: [PATCH 16/57] WIP towards responding to window focus in cleanup The idea being that if we've returned into the foreground while old message detachment was running the remaining background cleanup tasks shouldn't be run. --- src/client/application/application-controller.vala | 5 +++++ src/engine/imap-engine/imap-engine-generic-account.vala | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index cfb7d186..903f7943 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -95,6 +95,9 @@ internal class Application.Controller : Geary.BaseObject { // Timeout to do work in idle after all windows have been sent to the background private Geary.TimeoutManager all_windows_backgrounded_timeout = null; + // Whether we're fully in the background + public bool all_windows_backgrounded { get; private set; default = false; } + /** * Emitted when an account is added or is enabled. @@ -1399,6 +1402,7 @@ internal class Application.Controller : Geary.BaseObject { // Track a window receiving focus, for idle background work public void window_focus_in() { + this.all_windows_backgrounded = false; if (this.all_windows_backgrounded_timeout != null) { this.all_windows_backgrounded_timeout.reset(); this.all_windows_backgrounded_timeout = null; @@ -1414,6 +1418,7 @@ internal class Application.Controller : Geary.BaseObject { private void on_unfocused_idle() { // Schedule later, catching cases where work should occur later while still in background this.all_windows_backgrounded_timeout = null; + this.all_windows_backgrounded = true; window_focus_out(); debug("Checking for backgrounded idle work"); diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index b1f673c8..dc8ade26 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -551,6 +551,10 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // Continue backgrounded app cleanup work after the first phase, // old message detachment, has completed public void app_backgrounded_cleanup_continued(bool messages_detached, Cancellable? cancellable) { + // TODO bail on remaining work if we've returned from the background + // if (!application.controller.all_windows_backgrounded) + // return; + // Kick off GC, allowing vacuum and forcing reap if we've removed messages local.db.run_gc.begin(cancellable, messages_detached, this); } From b3ab74839e6146aa360d6e7234fc97defbde29d4 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 8 Jan 2020 14:49:53 +0100 Subject: [PATCH 17/57] Improve const naming --- src/client/application/application-controller.vala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 903f7943..7a3fdbe8 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -18,7 +18,7 @@ internal class Application.Controller : Geary.BaseObject { private const uint MAX_AUTH_ATTEMPTS = 3; - private const uint CLEANUP_WORK_AFTER_IDLE_BACKGROUND_MINUTES = 5; + private const uint CLEANUP_CHECK_AFTER_IDLE_BACKGROUND_MINUTES = 5; /** Determines if conversations can be trashed from the given folder. */ public static bool does_folder_support_trash(Geary.Folder? target) { @@ -1411,7 +1411,7 @@ internal class Application.Controller : Geary.BaseObject { // Track a window going unfocused, for idle background work public void window_focus_out() { - this.all_windows_backgrounded_timeout = new Geary.TimeoutManager.seconds(CLEANUP_WORK_AFTER_IDLE_BACKGROUND_MINUTES * 60, on_unfocused_idle); + this.all_windows_backgrounded_timeout = new Geary.TimeoutManager.seconds(CLEANUP_CHECK_AFTER_IDLE_BACKGROUND_MINUTES * 60, on_unfocused_idle); this.all_windows_backgrounded_timeout.start(); } From dfc4c55a99479bb9cc63fa419bb855d44903ead1 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 12:36:47 +0100 Subject: [PATCH 18/57] Cleanup timeout usage --- src/client/application/application-controller.vala | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 7a3fdbe8..9c1a3df3 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -93,7 +93,7 @@ internal class Application.Controller : Geary.BaseObject { private Gee.List pending_mailtos = new Gee.ArrayList(); // Timeout to do work in idle after all windows have been sent to the background - private Geary.TimeoutManager all_windows_backgrounded_timeout = null; + private Geary.TimeoutManager all_windows_backgrounded_timeout; // Whether we're fully in the background public bool all_windows_backgrounded { get; private set; default = false; } @@ -154,6 +154,9 @@ internal class Application.Controller : Geary.BaseObject { ConversationWebView.load_resources(); Accounts.SignatureWebView.load_resources(); + this.all_windows_backgrounded_timeout = + new Geary.TimeoutManager.seconds(CLEANUP_CHECK_AFTER_IDLE_BACKGROUND_MINUTES * 60, on_unfocused_idle); + this.folks = Folks.IndividualAggregator.dup(); if (!this.folks.is_prepared) { // Do this in the background since it can take a long time @@ -1402,22 +1405,17 @@ internal class Application.Controller : Geary.BaseObject { // Track a window receiving focus, for idle background work public void window_focus_in() { - this.all_windows_backgrounded = false; - if (this.all_windows_backgrounded_timeout != null) { - this.all_windows_backgrounded_timeout.reset(); - this.all_windows_backgrounded_timeout = null; - } + this.all_windows_backgrounded_timeout.reset(); } // Track a window going unfocused, for idle background work public void window_focus_out() { - this.all_windows_backgrounded_timeout = new Geary.TimeoutManager.seconds(CLEANUP_CHECK_AFTER_IDLE_BACKGROUND_MINUTES * 60, on_unfocused_idle); this.all_windows_backgrounded_timeout.start(); } private void on_unfocused_idle() { // Schedule later, catching cases where work should occur later while still in background - this.all_windows_backgrounded_timeout = null; + this.all_windows_backgrounded_timeout.reset(); this.all_windows_backgrounded = true; window_focus_out(); From 29d947cd6d1f8df235d1f054d0dd8d976e5d5621 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 12:40:56 +0100 Subject: [PATCH 19/57] Code convention style tweaks --- .../application/application-controller.vala | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 9c1a3df3..04e04909 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -1403,29 +1403,20 @@ internal class Application.Controller : Geary.BaseObject { } } - // Track a window receiving focus, for idle background work + /** + * Track a window receiving focus, for idle background work. + */ public void window_focus_in() { this.all_windows_backgrounded_timeout.reset(); } - // Track a window going unfocused, for idle background work + /** + * Track a window going unfocused, for idle background work. + */ public void window_focus_out() { this.all_windows_backgrounded_timeout.start(); } - private void on_unfocused_idle() { - // Schedule later, catching cases where work should occur later while still in background - this.all_windows_backgrounded_timeout.reset(); - this.all_windows_backgrounded = true; - window_focus_out(); - - debug("Checking for backgrounded idle work"); - foreach (AccountContext context in this.accounts.values) { - Geary.Account account = context.account; - account.app_backgrounded_cleanup.begin(context.cancellable); - } - } - /** Displays a composer on the last active main window. */ internal void show_composer(Composer.Widget composer, Gee.Collection? refers_to, @@ -1792,6 +1783,19 @@ internal class Application.Controller : Geary.BaseObject { } } + private void on_unfocused_idle() { + // Schedule later, catching cases where work should occur later while still in background + this.all_windows_backgrounded_timeout.reset(); + this.all_windows_backgrounded = true; + window_focus_out(); + + debug("Checking for backgrounded idle work"); + foreach (AccountContext context in this.accounts.values) { + Geary.Account account = context.account; + account.app_backgrounded_cleanup.begin(context.cancellable); + } + } + } From d019d7ae9f0d97b4eea4f94c8808f59b4ca6fdb8 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 13:00:52 +0100 Subject: [PATCH 20/57] Move, rename and document property --- src/engine/api/geary-account-information.vala | 2 -- src/engine/api/geary-account.vala | 12 ++++++++++++ .../imap-engine/imap-engine-generic-account.vala | 4 ++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index 60c42e7a..6c8c06f2 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -210,8 +210,6 @@ public class Geary.AccountInformation : BaseObject { default = new Gee.LinkedList(); } - public DateTime? last_backgrounded_cleanup_time { get; set; default = null; } - /** * Emitted when a service has reported an authentication failure. * diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index d9100c1c..5e2641b4 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -145,6 +145,18 @@ public abstract class Geary.Account : BaseObject, Logging.Source { public ProgressMonitor db_upgrade_monitor { get; protected set; } public ProgressMonitor db_vacuum_monitor { get; protected set; } + /** + * The last time the account storage was cleaned. + * + * This does not imply that a full reap plus vacuum garbage + * collection (GC) is performed, merely that: + * 1. Any old messages are removed + * 2. If any old messages were removed, or the defined period + * (in ImapDB.GC) has past, a GC reap is performed + * 3. GC vacuum is run if recommended + */ + public DateTime? last_storage_cleanup { get; set; default = null; } + public signal void opened(); diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index dc8ade26..a6b4f2d5 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -535,12 +535,12 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { debug("Backgrounded cleanup check for %s account", this.information.display_name); DateTime now = new DateTime.now_local(); - DateTime? last_cleanup = this.information.last_backgrounded_cleanup_time; + DateTime? last_cleanup = this.last_storage_cleanup; if (last_cleanup == null || (now.difference(last_cleanup) / TimeSpan.MINUTE > APP_BACKGROUNDED_CLEANUP_WORK_INTERVAL_MINUTES)) { // Interval check is OK, start by detaching old messages - this.information.last_backgrounded_cleanup_time = now; + this.last_storage_cleanup = now; this.old_messages_background_cleanup_request(cancellable); } else if (local.db.want_background_vacuum) { // Vacuum has been flagged as needed, run it From 7dfe2971c653efc8b4c18d63f96da28d4edd031e Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 13:07:17 +0100 Subject: [PATCH 21/57] Rename and improve doc. on method --- src/client/application/application-controller.vala | 2 +- src/engine/api/geary-account.vala | 10 ++++++---- .../imap-engine/imap-engine-generic-account.vala | 4 ++-- test/engine/api/geary-account-mock.vala | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 04e04909..a6cdaf2f 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -1792,7 +1792,7 @@ internal class Application.Controller : Geary.BaseObject { debug("Checking for backgrounded idle work"); foreach (AccountContext context in this.accounts.values) { Geary.Account account = context.account; - account.app_backgrounded_cleanup.begin(context.cancellable); + account.cleanup_storage.begin(context.cancellable); } } diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index 5e2641b4..7ed28141 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -520,12 +520,14 @@ public abstract class Geary.Account : BaseObject, Logging.Source { } /** - * Run account cleanup work if the appropriate interval has past since - * last execution. Alternatively if the interval has not past but vacuum - * GC has been flagged to run this will be executed. Designed to be run + * Perform cleanup of account storage. + * + * Work is performed if the appropriate interval has past since last + * execution. Alternatively if the interval has not past but vacuum GC + * has been flagged to run this will be executed. Designed to be run * while the app is in the background and idle. */ - public abstract async void app_backgrounded_cleanup(Cancellable? cancellable); + public abstract async void cleanup_storage(Cancellable? cancellable); /** Fires a {@link opened} signal. */ protected virtual void notify_opened() { diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index a6b4f2d5..da5bec38 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -531,8 +531,8 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { } /** {@inheritDoc} */ - public override async void app_backgrounded_cleanup(Cancellable? cancellable) { - debug("Backgrounded cleanup check for %s account", this.information.display_name); + public override async void cleanup_storage(Cancellable? cancellable) { + debug("Backgrounded storage cleanup check for %s account", this.information.display_name); DateTime now = new DateTime.now_local(); DateTime? last_cleanup = this.last_storage_cleanup; diff --git a/test/engine/api/geary-account-mock.vala b/test/engine/api/geary-account-mock.vala index 77ca8287..f7b63f4c 100644 --- a/test/engine/api/geary-account-mock.vala +++ b/test/engine/api/geary-account-mock.vala @@ -274,7 +274,7 @@ public class Geary.MockAccount : Account, MockObject { ); } - public override async void app_backgrounded_cleanup(Cancellable? cancellable) { + public override async void cleanup_storage(Cancellable? cancellable) { } } From 8d9c9ea943491a838ee7bc17820d04c5441ba339 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 13:38:33 +0100 Subject: [PATCH 22/57] Also stop SMTP when GC vacuuming --- src/engine/imap-db/imap-db-database.vala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 5e0fc0ab..d490d6c6 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -128,6 +128,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { if (account != null) { this.want_background_vacuum = false; yield account.imap.stop(gc_cancellable); + yield account.smtp.stop(gc_cancellable); if (!vacuum_monitor.is_in_progress) vacuum_monitor.notify_start(); @@ -145,6 +146,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { } yield account.imap.start(gc_cancellable); + yield account.smtp.start(gc_cancellable); } else { // Flag a vacuum to run later when we've been idle in the background debug("Flagging desire to GC vacuum"); From 8ccc0f3d60db5d924137789d39e6d8293abeaa36 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 13:41:02 +0100 Subject: [PATCH 23/57] Code cleanup for old message detach Although this logic will likely be later removed --- .../imap-engine-account-synchronizer.vala | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 6fcc583a..43916249 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -67,14 +67,16 @@ private class Geary.ImapEngine.AccountSynchronizer : !folder.properties.is_local_only && !folder.properties.is_virtual) { - AccountOperation op = became_available - ? new CheckFolderSync( + AccountOperation op; + if (became_available) { + CheckFolderSync check_op = new CheckFolderSync( this.account, imap_folder, this.max_epoch - ) - : new RefreshFolderSync(this.account, imap_folder); - - if (became_available) - ((CheckFolderSync) op).old_message_detached.connect(this.old_messages_removed_during_sync); + ); + check_op.old_message_detached.connect(this.old_messages_removed_during_sync); + op = check_op; + } else { + op = new RefreshFolderSync(this.account, imap_folder); + } try { this.account.queue_operation(op); From f60c42a36bad153ee592cb7eb9b40dd7b1cda03e Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 14:08:46 +0100 Subject: [PATCH 24/57] More explicit class identification --- src/engine/api/geary-account.vala | 2 +- src/engine/imap-db/imap-db-database.vala | 6 +++--- src/engine/imap-db/imap-db-folder.vala | 2 +- .../imap-engine/imap-engine-account-synchronizer.vala | 6 +++--- src/engine/imap-engine/imap-engine-generic-account.vala | 6 +++--- test/engine/api/geary-account-mock.vala | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index 7ed28141..628a14ec 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -527,7 +527,7 @@ public abstract class Geary.Account : BaseObject, Logging.Source { * has been flagged to run this will be executed. Designed to be run * while the app is in the background and idle. */ - public abstract async void cleanup_storage(Cancellable? cancellable); + public abstract async void cleanup_storage(GLib.Cancellable? cancellable); /** Fires a {@link opened} signal. */ protected virtual void notify_opened() { diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index d490d6c6..86f351b0 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -61,7 +61,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private TimeoutManager gc_post_old_message_detach_timer; // Cancellable from account synchronizer for GC after old message cleanup - private Cancellable? post_gc_cleanup_cancellable; + private GLib.Cancellable? post_gc_cleanup_cancellable; public Database(GLib.File db_file, GLib.File schema_dir, @@ -99,7 +99,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { * signify that it's an appropriate time to run a vacuum (ie. we're * idle in the background) and provide access for stopping IMAP. */ - public async void run_gc(Cancellable? cancellable, + public async void run_gc(GLib.Cancellable? cancellable, bool force_reap = false, Geary.ImapEngine.GenericAccount? account = null) throws Error { @@ -210,7 +210,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { base.close(cancellable); } - public void schedule_gc_after_old_messages_cleanup(Cancellable? cancellable) { + public void schedule_gc_after_old_messages_cleanup(GLib.Cancellable? cancellable) { this.post_gc_cleanup_cancellable = cancellable; this.gc_post_old_message_detach_timer.start(); } diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 06ee87a7..4028be67 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -883,7 +883,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { } public async Gee.Collection? detach_emails_before_timestamp(DateTime cutoff, - Cancellable? cancellable) throws Error { + GLib.Cancellable? cancellable) throws Error { debug("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); Gee.Collection? deleted_ids = null; diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 43916249..e4b65b42 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -96,7 +96,7 @@ private class Geary.ImapEngine.AccountSynchronizer : } } - private void old_messages_background_cleanup(Cancellable? cancellable) { + private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { if (this.account.is_open()) { this.old_message_cleaner = new OldMessageCleaner(this.account, this.max_epoch); this.old_message_cleaner.run(); @@ -107,7 +107,7 @@ private class Geary.ImapEngine.AccountSynchronizer : } } - private void old_messages_removed_during_sync(Cancellable cancellable) { + private void old_messages_removed_during_sync(GLib.Cancellable cancellable) { if (this.old_message_cleaner == null) { // This is not a daily cleanup. We've detached some messages, let's GC if // recommended. @@ -246,7 +246,7 @@ private class Geary.ImapEngine.RefreshFolderSync : FolderOperation { */ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { - public signal void old_message_detached(Cancellable cancellable); + public signal void old_message_detached(GLib.Cancellable cancellable); private DateTime sync_max_epoch; diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index da5bec38..e221bfa3 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -43,7 +43,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { /** Local database for the account. */ public ImapDB.Account local { get; private set; } - public signal void old_messages_background_cleanup_request(Cancellable? cancellable); + public signal void old_messages_background_cleanup_request(GLib.Cancellable? cancellable); private bool open = false; private Cancellable? open_cancellable = null; @@ -531,7 +531,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { } /** {@inheritDoc} */ - public override async void cleanup_storage(Cancellable? cancellable) { + public override async void cleanup_storage(GLib.Cancellable? cancellable) { debug("Backgrounded storage cleanup check for %s account", this.information.display_name); DateTime now = new DateTime.now_local(); @@ -550,7 +550,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // Continue backgrounded app cleanup work after the first phase, // old message detachment, has completed - public void app_backgrounded_cleanup_continued(bool messages_detached, Cancellable? cancellable) { + public void app_backgrounded_cleanup_continued(bool messages_detached, GLib.Cancellable? cancellable) { // TODO bail on remaining work if we've returned from the background // if (!application.controller.all_windows_backgrounded) // return; diff --git a/test/engine/api/geary-account-mock.vala b/test/engine/api/geary-account-mock.vala index f7b63f4c..9ee59c43 100644 --- a/test/engine/api/geary-account-mock.vala +++ b/test/engine/api/geary-account-mock.vala @@ -274,7 +274,7 @@ public class Geary.MockAccount : Account, MockObject { ); } - public override async void cleanup_storage(Cancellable? cancellable) { + public override async void cleanup_storage(GLib.Cancellable? cancellable) { } } From 872c7d7fd51563518b4fe78b00c2c2c37a160165 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 14:45:20 +0100 Subject: [PATCH 25/57] Storage cleanup timing improvements Ensure only one account is cleaned at a time - they're now cleaned in sequence. Also ensure re-entrancy isn't possible. (Geary.Account.last_storage_cleanup was previously assisting with this but is now unsufficient with the above change). --- src/client/application/application-controller.vala | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index a6cdaf2f..14994d65 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -95,6 +95,9 @@ internal class Application.Controller : Geary.BaseObject { // Timeout to do work in idle after all windows have been sent to the background private Geary.TimeoutManager all_windows_backgrounded_timeout; + // Track whether storage cleanup is running + private bool storage_cleanup_running = false; + // Whether we're fully in the background public bool all_windows_backgrounded { get; private set; default = false; } @@ -1789,11 +1792,18 @@ internal class Application.Controller : Geary.BaseObject { this.all_windows_backgrounded = true; window_focus_out(); + if (!storage_cleanup_running) + do_background_storage_cleanup.begin(); + } + + private async void do_background_storage_cleanup() { debug("Checking for backgrounded idle work"); + storage_cleanup_running = true; foreach (AccountContext context in this.accounts.values) { Geary.Account account = context.account; - account.cleanup_storage.begin(context.cancellable); + yield account.cleanup_storage(context.cancellable); } + storage_cleanup_running = false; } } From b70a9b4e6e3fe1244104c9f8df2ca63f8d02bab9 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 18:23:40 +0100 Subject: [PATCH 26/57] Delete old messages in batches Done to avoid hitting SQLITE_MAX_SQL_LENGTH with the IN clause, although fairly unlikely --- src/engine/imap-db/imap-db-folder.vala | 62 +++++++++++++++++--------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 4028be67..7d57d29b 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -41,6 +41,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { private const int LIST_EMAIL_FIELDS_CHUNK_COUNT = 500; private const int REMOVE_COMPLETE_LOCATIONS_CHUNK_COUNT = 500; private const int CREATE_MERGE_EMAIL_CHUNK_COUNT = 25; + private const int MAX_DB_SUBLIST_LENGTH = 500000; [Flags] public enum ListFlags { @@ -885,7 +886,8 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { public async Gee.Collection? detach_emails_before_timestamp(DateTime cutoff, GLib.Cancellable? cancellable) throws Error { debug("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); - Gee.Collection? deleted_ids = null; + Gee.Collection? deleted_email_ids = null; + Gee.ArrayList deleted_primary_keys = null; yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { // MessageLocationTable.ordering isn't relied on due to IMAP folder @@ -909,35 +911,53 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { Db.Result results = stmt.exec(cancellable); - StringBuilder? ids_sql_sublist = null; while (!results.finished) { - if (ids_sql_sublist == null) { - deleted_ids = new Gee.ArrayList(); - ids_sql_sublist = new StringBuilder(); - } else { - ids_sql_sublist.append(","); + if (deleted_email_ids == null) { + deleted_email_ids = new Gee.ArrayList(); + deleted_primary_keys = new Gee.ArrayList(); } - deleted_ids.add(new ImapDB.EmailIdentifier(results.int64_at(1), new Imap.UID(results.int64_at(2)))); - ids_sql_sublist.append(results.rowid_at(0).to_string()); + deleted_email_ids.add( + new ImapDB.EmailIdentifier(results.int64_at(1), + new Imap.UID(results.int64_at(2))) + ); + deleted_primary_keys.add(results.rowid_at(0).to_string()); results.next(cancellable); } - if (deleted_ids != null) { - sql = new StringBuilder(); - sql.append(""" - DELETE FROM MessageLocationTable - WHERE id IN ( - """); - sql.append(ids_sql_sublist.str); - sql.append(")"); - stmt = cx.prepare(sql.str); + if (deleted_email_ids != null) { + // Delete in batches to avoid hiting SQLite maximum query + // length (although quite unlikely) + int delete_index = 0; + while (delete_index < deleted_primary_keys.size) { + StringBuilder ids_sql_sublist = new StringBuilder(); + while (delete_index < deleted_primary_keys.size + && ids_sql_sublist.len < MAX_DB_SUBLIST_LENGTH) { + if (ids_sql_sublist.len > 0) + ids_sql_sublist.append(","); + ids_sql_sublist.append( + deleted_primary_keys.get(delete_index) + ); + delete_index++; + } - stmt.exec(cancellable); + sql = new StringBuilder(); + sql.append(""" + DELETE FROM MessageLocationTable + WHERE id IN ( + """); + sql.append(ids_sql_sublist.str); + sql.append(")"); + stmt = cx.prepare(sql.str); + + stmt.exec(cancellable); + } // Update local message count - this.properties.set_status_message_count(this.properties.email_total - deleted_ids.size, true); + this.properties.set_status_message_count( + this.properties.email_total - deleted_email_ids.size, true + ); return Db.TransactionOutcome.COMMIT; } else { @@ -945,7 +965,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { } }, cancellable); - return deleted_ids; + return deleted_email_ids; } public async void mark_email_async(Gee.Collection to_mark, From 4f89a593a568a7ded4c414f474b44d13dceb800a Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 20:23:49 +0100 Subject: [PATCH 27/57] Remove temporary WIP code --- src/client/application/application-controller.vala | 4 ---- src/engine/imap-engine/imap-engine-generic-account.vala | 3 --- 2 files changed, 7 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 14994d65..53d12ee9 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -98,9 +98,6 @@ internal class Application.Controller : Geary.BaseObject { // Track whether storage cleanup is running private bool storage_cleanup_running = false; - // Whether we're fully in the background - public bool all_windows_backgrounded { get; private set; default = false; } - /** * Emitted when an account is added or is enabled. @@ -1789,7 +1786,6 @@ internal class Application.Controller : Geary.BaseObject { private void on_unfocused_idle() { // Schedule later, catching cases where work should occur later while still in background this.all_windows_backgrounded_timeout.reset(); - this.all_windows_backgrounded = true; window_focus_out(); if (!storage_cleanup_running) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index e221bfa3..30c0f072 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -551,9 +551,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // Continue backgrounded app cleanup work after the first phase, // old message detachment, has completed public void app_backgrounded_cleanup_continued(bool messages_detached, GLib.Cancellable? cancellable) { - // TODO bail on remaining work if we've returned from the background - // if (!application.controller.all_windows_backgrounded) - // return; // Kick off GC, allowing vacuum and forcing reap if we've removed messages local.db.run_gc.begin(cancellable, messages_detached, this); From 187d613391d6395350382b15ad93535faf2b38f3 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 9 Jan 2020 20:25:55 +0100 Subject: [PATCH 28/57] Try AccountOperation approach for GC post detach --- src/engine/imap-db/imap-db-database.vala | 27 +-- .../imap-engine-account-synchronizer.vala | 154 +++++++----------- .../imap-engine-generic-account.vala | 10 +- 3 files changed, 63 insertions(+), 128 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 86f351b0..c45054e0 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -50,8 +50,6 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private const int OPEN_PUMP_EVENT_LOOP_MSEC = 100; - private const uint DELAY_GC_AFTER_OLD_MESSAGE_CLEANUP_SECONDS = 30; - private ProgressMonitor upgrade_monitor; private ProgressMonitor vacuum_monitor; private bool new_db = false; @@ -59,10 +57,6 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private GC? gc = null; private Cancellable gc_cancellable = new Cancellable(); - private TimeoutManager gc_post_old_message_detach_timer; - // Cancellable from account synchronizer for GC after old message cleanup - private GLib.Cancellable? post_gc_cleanup_cancellable; - public Database(GLib.File db_file, GLib.File schema_dir, GLib.File attachments_path, @@ -72,12 +66,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { this.attachments_path = attachments_path; this.upgrade_monitor = upgrade_monitor; this.vacuum_monitor = vacuum_monitor; - - this.gc_post_old_message_detach_timer = - new TimeoutManager.seconds( - DELAY_GC_AFTER_OLD_MESSAGE_CLEANUP_SECONDS, run_gc_post_old_message_detach - ); - } + } /** * Prepares the ImapDB database for use. @@ -101,6 +90,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { */ public async void run_gc(GLib.Cancellable? cancellable, bool force_reap = false, + bool allow_vacuum = false, Geary.ImapEngine.GenericAccount? account = null) throws Error { @@ -125,7 +115,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { // VACUUM needs to execute in the foreground with the user given a busy prompt (and cannot // be run at the same time as REAP) if ((recommended & GC.RecommendedOperation.VACUUM) != 0) { - if (account != null) { + if (allow_vacuum) { this.want_background_vacuum = false; yield account.imap.stop(gc_cancellable); yield account.smtp.stop(gc_cancellable); @@ -210,17 +200,6 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { base.close(cancellable); } - public void schedule_gc_after_old_messages_cleanup(GLib.Cancellable? cancellable) { - this.post_gc_cleanup_cancellable = cancellable; - this.gc_post_old_message_detach_timer.start(); - } - - public void run_gc_post_old_message_detach() { - debug("Requesting GC post old message cleanup"); - run_gc.begin(this.post_gc_cleanup_cancellable); - this.post_gc_cleanup_cancellable = null; - } - protected override void starting_upgrade(int current_version, bool new_db) { this.new_db = new_db; diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index e4b65b42..bd2903fa 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -16,7 +16,6 @@ private class Geary.ImapEngine.AccountSynchronizer : private DateTime max_epoch = new DateTime( new TimeZone.local(), 2000, 1, 1, 0, 0, 0.0 ); - private OldMessageCleaner old_message_cleaner; public AccountSynchronizer(GenericAccount account) { @@ -51,7 +50,8 @@ private class Geary.ImapEngine.AccountSynchronizer : ); } - private void send_all(Gee.Collection folders, bool became_available) { + private void send_all(Gee.Collection folders, bool became_available, + bool for_storage_clean=false) { foreach (Folder folder in folders) { // Only sync folders that: // 1. Can actually be opened (i.e. are selectable) @@ -68,12 +68,13 @@ private class Geary.ImapEngine.AccountSynchronizer : !folder.properties.is_virtual) { AccountOperation op; - if (became_available) { - CheckFolderSync check_op = new CheckFolderSync( - this.account, imap_folder, this.max_epoch + if (became_available || for_storage_clean) { + op = new CheckFolderSync( + this.account, + imap_folder, + this.max_epoch, + for_storage_clean ); - check_op.old_message_detached.connect(this.old_messages_removed_during_sync); - op = check_op; } else { op = new RefreshFolderSync(this.account, imap_folder); } @@ -97,22 +98,8 @@ private class Geary.ImapEngine.AccountSynchronizer : } private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { - if (this.account.is_open()) { - this.old_message_cleaner = new OldMessageCleaner(this.account, this.max_epoch); - this.old_message_cleaner.run(); - this.old_message_cleaner.completed.connect((messages_detached) => { - this.old_message_cleaner = null; - this.account.app_backgrounded_cleanup_continued(messages_detached, cancellable); - }); - } - } - - private void old_messages_removed_during_sync(GLib.Cancellable cancellable) { - if (this.old_message_cleaner == null) { - // This is not a daily cleanup. We've detached some messages, let's GC if - // recommended. - GenericAccount account = (GenericAccount) this.account; - account.local.db.schedule_gc_after_old_messages_cleanup(cancellable); + if (this.account.is_open()) { + send_all(this.account.list_folders(), false, true); } } @@ -249,13 +236,16 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { public signal void old_message_detached(GLib.Cancellable cancellable); private DateTime sync_max_epoch; + private bool for_storage_clean; internal CheckFolderSync(GenericAccount account, MinimalFolder folder, - DateTime sync_max_epoch) { + DateTime sync_max_epoch, + bool for_storage_clean) { base(account, folder); this.sync_max_epoch = sync_max_epoch; + this.for_storage_clean = for_storage_clean; } protected override async void sync_folder(Cancellable cancellable) @@ -275,10 +265,20 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { // Detach older emails outside the prefetch window if (this.account.information.prefetch_period_days >= 0) { - Gee.Collection? detached_ids = yield local_folder.detach_emails_before_timestamp(prefetch_max_epoch, cancellable); + Gee.Collection? detached_ids = + yield local_folder.detach_emails_before_timestamp(prefetch_max_epoch, + cancellable); if (detached_ids != null) { this.folder.email_locally_removed(detached_ids); old_message_detached(cancellable); + GenericAccount imap_account = (GenericAccount) account; + GarbageCollectPostMessageDetach op = + new GarbageCollectPostMessageDetach(imap_account, for_storage_clean); + try { + imap_account.queue_operation(op); + } catch (Error err) { + warning("Failed to queue sync operation: %s", err.message); + } } } @@ -410,85 +410,49 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { } +/** + * Kicks off garbage collection after old messages have been removed. + * + * Queues a GC run which will run with varying parameters depending on whether + * we're running as part of the daily background storage cleanup. `equal_to` + * is used to ensure the operation is only queued once. + */ +private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation { -private class Geary.ImapEngine.OldMessageCleaner: BaseObject, Logging.Source { + private GenericAccount imap_account; + private bool for_daily_storage_clean; - public bool messages_detached {get; set; default = false; } + internal GarbageCollectPostMessageDetach(GenericAccount account, + bool for_daily_storage_clean) { - private GenericAccount account; - private DateTime sync_max_epoch; - private List operations = - new List(); - - public signal void completed(bool messages_detached); - - /** {@inheritDoc} */ - public Logging.Flag logging_flags { - get; protected set; default = Logging.Flag.ALL; + base(account); + this.imap_account = account; + this.for_daily_storage_clean = for_daily_storage_clean; } - /** {@inheritDoc} */ - public Logging.Source? logging_parent { - get { return this.account; } + public override async void execute(GLib.Cancellable cancellable) + throws Error { + GenericAccount generic_account = (GenericAccount) account; + + // Run GC, forcing reap and allowing vacuum if performing daily storage + // clean + yield generic_account.local.db.run_gc(cancellable, + for_daily_storage_clean, + for_daily_storage_clean, + generic_account); } - internal OldMessageCleaner(GenericAccount account, - DateTime sync_max_epoch) { - this.sync_max_epoch = sync_max_epoch; - this.account = account; - } + public override bool equal_to(AccountOperation op) { + bool basic_eq = (op != null + && (this == op || this.get_type() == op.get_type()) + && this.account == op.account); + if (!basic_eq) + return false; - public void run() { - foreach (Folder folder in this.account.list_folders()) { - // Only sync folders that: - // 1. Can actually be opened (i.e. are selectable) - // 2. Are remote backed - // - // All this implies the folder must be a MinimalFolder and - // we do require that for syncing at the moment anyway, - // but keep the tests in for that one glorious day where - // we can just use a generic folder. - MinimalFolder? imap_folder = folder as MinimalFolder; - if (imap_folder != null && - folder.properties.is_openable.is_possible() && - !folder.properties.is_local_only && - !folder.properties.is_virtual) { + GarbageCollectPostMessageDetach other_op = + (GarbageCollectPostMessageDetach) op; - CheckFolderSync op = - new CheckFolderSync( - this.account, imap_folder, this.sync_max_epoch - ); - try { - this.account.queue_operation(op); - this.operations.append(op); - - // Let daily cleanup monitor know that we detached some messages - // (which will result in a 'forced' GC reap) - op.old_message_detached.connect(() => { - this.messages_detached = true; - }); - op.completed.connect(() => { - this.operations.remove(op); - if (operations.length() == 0) { - this.completed(this.messages_detached); - } - - }); - } catch (Error err) { - warning("Failed to queue sync operation: %s", err.message); - } - } - } - } - - /** {@inheritDoc} */ - public virtual Logging.State to_logging_state() { - return new Logging.State( - this, - "%s, %s", - this.account.information.id, - this.sync_max_epoch.to_string() - ); + return this.for_daily_storage_clean == other_op.for_daily_storage_clean; } } diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 30c0f072..c5774d51 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -544,18 +544,10 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { this.old_messages_background_cleanup_request(cancellable); } else if (local.db.want_background_vacuum) { // Vacuum has been flagged as needed, run it - local.db.run_gc.begin(cancellable, false, this); + local.db.run_gc.begin(cancellable, false, true, this); } } - // Continue backgrounded app cleanup work after the first phase, - // old message detachment, has completed - public void app_backgrounded_cleanup_continued(bool messages_detached, GLib.Cancellable? cancellable) { - - // Kick off GC, allowing vacuum and forcing reap if we've removed messages - local.db.run_gc.begin(cancellable, messages_detached, this); - } - /** * Constructs a set of folders and adds them to the account. * From 8cb83433669bf196e98033b178c24a4031c5a8fa Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 08:22:23 +0100 Subject: [PATCH 29/57] Revert unwanted folder property update --- src/engine/api/geary-account-information.vala | 1 + src/engine/api/geary-folder.vala | 1 - src/engine/imap-db/imap-db-folder.vala | 5 ----- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/engine/api/geary-account-information.vala b/src/engine/api/geary-account-information.vala index 6c8c06f2..cdc05eb6 100644 --- a/src/engine/api/geary-account-information.vala +++ b/src/engine/api/geary-account-information.vala @@ -210,6 +210,7 @@ public class Geary.AccountInformation : BaseObject { default = new Gee.LinkedList(); } + /** * Emitted when a service has reported an authentication failure. * diff --git a/src/engine/api/geary-folder.vala b/src/engine/api/geary-folder.vala index a219c282..c067cf6a 100644 --- a/src/engine/api/geary-folder.vala +++ b/src/engine/api/geary-folder.vala @@ -364,7 +364,6 @@ public abstract class Geary.Folder : BaseObject, Logging.Source { */ public signal void email_removed(Gee.Collection ids); - /** * Fired when emails are removed from the local folder. */ diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 7d57d29b..206a7dec 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -954,11 +954,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { stmt.exec(cancellable); } - // Update local message count - this.properties.set_status_message_count( - this.properties.email_total - deleted_email_ids.size, true - ); - return Db.TransactionOutcome.COMMIT; } else { return Db.TransactionOutcome.DONE; From 8871f02532ded31fa43ed1404da24c670f168783 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 09:04:25 +0100 Subject: [PATCH 30/57] Tweaked old message detachment batching --- src/engine/imap-db/imap-db-folder.vala | 39 +++++++++++++++----------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 206a7dec..524665b3 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -41,7 +41,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { private const int LIST_EMAIL_FIELDS_CHUNK_COUNT = 500; private const int REMOVE_COMPLETE_LOCATIONS_CHUNK_COUNT = 500; private const int CREATE_MERGE_EMAIL_CHUNK_COUNT = 25; - private const int MAX_DB_SUBLIST_LENGTH = 500000; + private const int OLD_MSG_DETACH_BATCH_SIZE = 1000; [Flags] public enum ListFlags { @@ -889,7 +889,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { Gee.Collection? deleted_email_ids = null; Gee.ArrayList deleted_primary_keys = null; - yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { + yield db.exec_transaction_async(Db.TransactionType.RO, (cx) => { // MessageLocationTable.ordering isn't relied on due to IMAP folder // UIDs not guaranteed to be in order. StringBuilder sql = new StringBuilder(); @@ -925,40 +925,47 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { results.next(cancellable); } + return Db.TransactionOutcome.DONE; + }, cancellable); - if (deleted_email_ids != null) { - // Delete in batches to avoid hiting SQLite maximum query - // length (although quite unlikely) - int delete_index = 0; - while (delete_index < deleted_primary_keys.size) { + if (deleted_email_ids != null) { + warning("size: %d", deleted_email_ids.size); + // Delete in batches to avoid hiting SQLite maximum query + // length (although quite unlikely) + int delete_index = 0; + while (delete_index < deleted_primary_keys.size) { + int batch_counter = 0; + + yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { StringBuilder ids_sql_sublist = new StringBuilder(); while (delete_index < deleted_primary_keys.size - && ids_sql_sublist.len < MAX_DB_SUBLIST_LENGTH) { - if (ids_sql_sublist.len > 0) + && batch_counter < OLD_MSG_DETACH_BATCH_SIZE) { + if (batch_counter > 0) ids_sql_sublist.append(","); ids_sql_sublist.append( deleted_primary_keys.get(delete_index) ); delete_index++; + batch_counter++; } - sql = new StringBuilder(); + StringBuilder sql = new StringBuilder(); sql.append(""" DELETE FROM MessageLocationTable WHERE id IN ( """); sql.append(ids_sql_sublist.str); sql.append(")"); - stmt = cx.prepare(sql.str); + Db.Statement stmt = cx.prepare(sql.str); + warning(" batch delete: %s", sql.str); stmt.exec(cancellable); - } - return Db.TransactionOutcome.COMMIT; - } else { - return Db.TransactionOutcome.DONE; + return Db.TransactionOutcome.COMMIT; + + }, cancellable); } - }, cancellable); + } return deleted_email_ids; } From f4a797253f979e8113860b237bcdbd8afe431da1 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 09:06:44 +0100 Subject: [PATCH 31/57] Cleanup to last commit --- src/engine/imap-db/imap-db-folder.vala | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 524665b3..80a96c88 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -929,7 +929,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { }, cancellable); if (deleted_email_ids != null) { - warning("size: %d", deleted_email_ids.size); // Delete in batches to avoid hiting SQLite maximum query // length (although quite unlikely) int delete_index = 0; @@ -957,7 +956,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { sql.append(ids_sql_sublist.str); sql.append(")"); Db.Statement stmt = cx.prepare(sql.str); - warning(" batch delete: %s", sql.str); stmt.exec(cancellable); From 47a5fbcd2faa6437f24759ce14562808b474b54d Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 10:45:32 +0100 Subject: [PATCH 32/57] Block input to main windows for DB vacuum/upgrade --- src/client/dialogs/upgrade-dialog.vala | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/client/dialogs/upgrade-dialog.vala b/src/client/dialogs/upgrade-dialog.vala index 29d88864..e4871984 100644 --- a/src/client/dialogs/upgrade-dialog.vala +++ b/src/client/dialogs/upgrade-dialog.vala @@ -32,6 +32,11 @@ public class UpgradeDialog : Object { } private void on_start() { + // Disable main windows + foreach (Application.MainWindow window in this.application.get_main_windows()) { + window.sensitive = false; + } + Gtk.Builder builder = GioUtil.create_builder("upgrade_dialog.glade"); this.dialog = (Gtk.Dialog) builder.get_object("dialog"); this.dialog.set_transient_for( @@ -58,6 +63,11 @@ public class UpgradeDialog : Object { this.dialog.hide(); this.dialog = null; } + + // Enable main windows + foreach (Application.MainWindow window in this.application.get_main_windows()) { + window.sensitive = true; + } } /** From f5279c6533f345dcfb0f8dadbaa13e1861616d9f Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 10:59:40 +0100 Subject: [PATCH 33/57] GC vacuum: verify non-null services Check IMAP and SMTP aren't null before stopping and starting, although this should never occur anymore as we don't vacuum at startup. --- src/engine/imap-db/imap-db-database.vala | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index c45054e0..3ee24193 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -117,8 +117,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { if ((recommended & GC.RecommendedOperation.VACUUM) != 0) { if (allow_vacuum) { this.want_background_vacuum = false; - yield account.imap.stop(gc_cancellable); - yield account.smtp.stop(gc_cancellable); + if (account.imap != null) + yield account.imap.stop(gc_cancellable); + if (account.smtp != null) + yield account.smtp.stop(gc_cancellable); if (!vacuum_monitor.is_in_progress) vacuum_monitor.notify_start(); @@ -135,8 +137,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { vacuum_monitor.notify_finish(); } - yield account.imap.start(gc_cancellable); - yield account.smtp.start(gc_cancellable); + if (account.imap != null) + yield account.imap.start(gc_cancellable); + if (account.smtp != null) + yield account.smtp.start(gc_cancellable); } else { // Flag a vacuum to run later when we've been idle in the background debug("Flagging desire to GC vacuum"); From 0f6db1ee2819eaea8ec09e36d893cb90c7dae4aa Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 14:18:01 +0100 Subject: [PATCH 34/57] Updated approach to background storage cleanup Storage cleanup work now continues and also waits for each account to complete before moving on --- src/engine/imap-db/imap-db-database.vala | 19 +-- .../imap-engine-account-synchronizer.vala | 109 +++++++++++------- .../imap-engine-generic-account.vala | 2 +- 3 files changed, 81 insertions(+), 49 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 3ee24193..6dd97dcb 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -91,7 +91,8 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { public async void run_gc(GLib.Cancellable? cancellable, bool force_reap = false, bool allow_vacuum = false, - Geary.ImapEngine.GenericAccount? account = null) + Geary.Imap.ClientService? imap_service = null, + Geary.Smtp.ClientService? smtp_service = null) throws Error { if (this.gc != null) { @@ -117,10 +118,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { if ((recommended & GC.RecommendedOperation.VACUUM) != 0) { if (allow_vacuum) { this.want_background_vacuum = false; - if (account.imap != null) - yield account.imap.stop(gc_cancellable); - if (account.smtp != null) - yield account.smtp.stop(gc_cancellable); + if (imap_service != null) + yield imap_service.stop(gc_cancellable); + if (smtp_service != null) + yield smtp_service.stop(gc_cancellable); if (!vacuum_monitor.is_in_progress) vacuum_monitor.notify_start(); @@ -137,10 +138,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { vacuum_monitor.notify_finish(); } - if (account.imap != null) - yield account.imap.start(gc_cancellable); - if (account.smtp != null) - yield account.smtp.start(gc_cancellable); + if (imap_service != null) + yield imap_service.start(gc_cancellable); + if (smtp_service != null) + yield smtp_service.start(gc_cancellable); } else { // Flag a vacuum to run later when we've been idle in the background debug("Flagging desire to GC vacuum"); diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index bd2903fa..1614531d 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -50,8 +50,11 @@ private class Geary.ImapEngine.AccountSynchronizer : ); } - private void send_all(Gee.Collection folders, bool became_available, - bool for_storage_clean=false) { + private void send_all(Gee.Collection folders, + bool became_available, + bool for_storage_clean=false, + SyncDetachMonitor? monitor=null) { + foreach (Folder folder in folders) { // Only sync folders that: // 1. Can actually be opened (i.e. are selectable) @@ -69,12 +72,15 @@ private class Geary.ImapEngine.AccountSynchronizer : AccountOperation op; if (became_available || for_storage_clean) { - op = new CheckFolderSync( + CheckFolderSync check_op = new CheckFolderSync( this.account, imap_folder, this.max_epoch, for_storage_clean ); + if (monitor != null) + monitor.add(check_op); + op = check_op; } else { op = new RefreshFolderSync(this.account, imap_folder); } @@ -99,7 +105,18 @@ private class Geary.ImapEngine.AccountSynchronizer : private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { if (this.account.is_open()) { - send_all(this.account.list_folders(), false, true); + SyncDetachMonitor monitor = new SyncDetachMonitor(); + send_all(this.account.list_folders(), false, true, monitor); + monitor.initialised = true; + monitor.completed.connect((messages_detached) => { + // Run GC. Reap is forced if messages were detached. Vacuum + // is allowed as we're running in the background. + account.local.db.run_gc.begin(cancellable, + messages_detached, + true, + account.imap, + account.smtp); + }); } } @@ -271,13 +288,16 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { if (detached_ids != null) { this.folder.email_locally_removed(detached_ids); old_message_detached(cancellable); - GenericAccount imap_account = (GenericAccount) account; - GarbageCollectPostMessageDetach op = - new GarbageCollectPostMessageDetach(imap_account, for_storage_clean); - try { - imap_account.queue_operation(op); - } catch (Error err) { - warning("Failed to queue sync operation: %s", err.message); + + if (!for_storage_clean) { + GenericAccount imap_account = (GenericAccount) account; + GarbageCollectPostMessageDetach op = + new GarbageCollectPostMessageDetach(imap_account); + try { + imap_account.queue_operation(op); + } catch (Error err) { + warning("Failed to queue sync operation: %s", err.message); + } } } } @@ -413,46 +433,57 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { /** * Kicks off garbage collection after old messages have been removed. * - * Queues a GC run which will run with varying parameters depending on whether - * we're running as part of the daily background storage cleanup. `equal_to` - * is used to ensure the operation is only queued once. + * Queues a basic GC run which will run if old messages were detached + * after a folder became available. Not used for backgrounded account + * storage operations, which are handled instead by the {@link SyncDetachMonitor}. */ private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation { - private GenericAccount imap_account; - private bool for_daily_storage_clean; - - internal GarbageCollectPostMessageDetach(GenericAccount account, - bool for_daily_storage_clean) { - + internal GarbageCollectPostMessageDetach(GenericAccount account) { base(account); - this.imap_account = account; - this.for_daily_storage_clean = for_daily_storage_clean; } public override async void execute(GLib.Cancellable cancellable) throws Error { + // Run basic GC GenericAccount generic_account = (GenericAccount) account; - - // Run GC, forcing reap and allowing vacuum if performing daily storage - // clean - yield generic_account.local.db.run_gc(cancellable, - for_daily_storage_clean, - for_daily_storage_clean, - generic_account); + yield generic_account.local.db.run_gc(cancellable); } public override bool equal_to(AccountOperation op) { - bool basic_eq = (op != null - && (this == op || this.get_type() == op.get_type()) - && this.account == op.account); - if (!basic_eq) - return false; - - GarbageCollectPostMessageDetach other_op = - (GarbageCollectPostMessageDetach) op; - - return this.for_daily_storage_clean == other_op.for_daily_storage_clean; + return (op != null + && (this == op || this.get_type() == op.get_type()) + && this.account == op.account); } } + +/** + * Monitor account sync operations until completion. + * + * Monitors added {@link CheckFolderSync} operations until completed + * and signals whether messages were detached. + */ +private class SyncDetachMonitor: Geary.BaseObject { + + public bool messages_detached { get; private set; default = false; } + public bool initialised { get; set; default = false; } + + private List operations = + new List(); + + public signal void completed(bool messages_detached); + + public void add(Geary.ImapEngine.CheckFolderSync operation) { + this.operations.append(operation); + operation.old_message_detached.connect(() => { + this.messages_detached = true; + }); + operation.completed.connect(() => { + this.operations.remove(operation); + if (initialised && operations.length() == 0) { + this.completed(this.messages_detached); + } + }); + } +} diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index c5774d51..3aaac788 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -544,7 +544,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { this.old_messages_background_cleanup_request(cancellable); } else if (local.db.want_background_vacuum) { // Vacuum has been flagged as needed, run it - local.db.run_gc.begin(cancellable, false, true, this); + local.db.run_gc.begin(cancellable, false, true, this.imap, this.smtp); } } From 075eb636bc66158c84fbd5b88ed98c0d30bcf973 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 15:37:16 +0100 Subject: [PATCH 35/57] Bugfix for window focus event propogation --- src/client/application/application-main-window.vala | 4 ++-- src/client/composer/composer-window.vala | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/application/application-main-window.vala b/src/client/application/application-main-window.vala index b57537d1..e7708001 100644 --- a/src/client/application/application-main-window.vala +++ b/src/client/application/application-main-window.vala @@ -498,11 +498,11 @@ public class Application.MainWindow : this.focus_in_event.connect((w, e) => { application.controller.window_focus_in(); - return true; + return false; }); this.focus_out_event.connect((w, e) => { application.controller.window_focus_out(); - return true; + return false; }); setup_layout(application.config); diff --git a/src/client/composer/composer-window.vala b/src/client/composer/composer-window.vala index df0be889..5a2d5d18 100644 --- a/src/client/composer/composer-window.vala +++ b/src/client/composer/composer-window.vala @@ -50,11 +50,11 @@ public class Composer.Window : Gtk.ApplicationWindow, Container { this.focus_in_event.connect((w, e) => { application.controller.window_focus_in(); - return true; + return false; }); this.focus_out_event.connect((w, e) => { application.controller.window_focus_out(); - return true; + return false; }); show(); From ae19398944b3401fe4992f042112990aeae3e36b Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 17:41:06 +0100 Subject: [PATCH 36/57] Improve cancel-ability of account storage cleanup --- .../application/application-controller.vala | 25 ++++++++++++++++++- src/engine/imap-db/imap-db-database.vala | 8 +++++- .../imap-engine-account-synchronizer.vala | 8 +++++- .../imap-engine-generic-account.vala | 3 +-- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index 53d12ee9..9c07cf5b 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -98,6 +98,8 @@ internal class Application.Controller : Geary.BaseObject { // Track whether storage cleanup is running private bool storage_cleanup_running = false; + private GLib.Cancellable? storage_cleanup_cancellable; + /** * Emitted when an account is added or is enabled. @@ -1408,6 +1410,21 @@ internal class Application.Controller : Geary.BaseObject { */ public void window_focus_in() { this.all_windows_backgrounded_timeout.reset(); + + if (this.storage_cleanup_cancellable != null) { + this.storage_cleanup_cancellable.cancel(); + + // Cleanup was still running and we don't know where we got to so + // we'll clear each of these so it runs next time we're in the + // background + foreach (AccountContext context in this.accounts.values) { + context.cancellable.cancelled.disconnect(this.storage_cleanup_cancellable.cancel); + + Geary.Account account = context.account; + account.last_storage_cleanup = null; + } + this.storage_cleanup_cancellable = null; + } } /** @@ -1795,9 +1812,15 @@ internal class Application.Controller : Geary.BaseObject { private async void do_background_storage_cleanup() { debug("Checking for backgrounded idle work"); storage_cleanup_running = true; + this.storage_cleanup_cancellable = new GLib.Cancellable(); + foreach (AccountContext context in this.accounts.values) { Geary.Account account = context.account; - yield account.cleanup_storage(context.cancellable); + context.cancellable.cancelled.connect(this.storage_cleanup_cancellable.cancel); + yield account.cleanup_storage(this.storage_cleanup_cancellable); + if (this.storage_cleanup_cancellable.is_cancelled()) + break; + context.cancellable.cancelled.disconnect(this.storage_cleanup_cancellable.cancel); } storage_cleanup_running = false; } diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 6dd97dcb..36a32b54 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -84,7 +84,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { * Reap should only be forced when there is known cleanup to perform and * the interval based recommendation should be bypassed. * - * TODO Passing of account is a WIP hack. It is currently used to both + * TODO Passing of the services is a WIP hack. It is currently used to both * signify that it's an appropriate time to run a vacuum (ie. we're * idle in the background) and provide access for stopping IMAP. */ @@ -149,6 +149,12 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { } } + // Abandon REAP if cancelled + if (cancellable != null && cancellable.is_cancelled()) { + cancellable.cancelled.disconnect(cancel_gc); + return; + } + // REAP can run in the background while the application is executing if (force_reap || (recommended & GC.RecommendedOperation.REAP) != 0) { // run in the background and allow application to continue running diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 1614531d..cd27a670 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -103,12 +103,15 @@ private class Geary.ImapEngine.AccountSynchronizer : } } - private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { + private void old_messages_background_cleanup(GLib.Cancellable cancellable) { if (this.account.is_open()) { SyncDetachMonitor monitor = new SyncDetachMonitor(); send_all(this.account.list_folders(), false, true, monitor); monitor.initialised = true; monitor.completed.connect((messages_detached) => { + if (cancellable.is_cancelled()) + return; + // Run GC. Reap is forced if messages were detached. Vacuum // is allowed as we're running in the background. account.local.db.run_gc.begin(cancellable, @@ -445,6 +448,9 @@ private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation public override async void execute(GLib.Cancellable cancellable) throws Error { + if (cancellable.is_cancelled()) + return; + // Run basic GC GenericAccount generic_account = (GenericAccount) account; yield generic_account.local.db.run_gc(cancellable); diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 3aaac788..75adcb10 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -17,8 +17,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { // we don't need to double check. private const int REFRESH_FOLDER_LIST_SEC = 15 * 60; - // Frequency of account cleanup work, performed when idle with the app - // backgrounded + /** Minimum interval between account storage cleanup work */ private const uint APP_BACKGROUNDED_CLEANUP_WORK_INTERVAL_MINUTES = 60 * 24; private const Geary.SpecialFolderType[] SUPPORTED_SPECIAL_FOLDERS = { From 43ecc3529856e91fe1f459f3e1bbde740aec4d2d Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 10 Jan 2020 18:08:37 +0100 Subject: [PATCH 37/57] Minor fix to last commit --- src/engine/imap-engine/imap-engine-account-synchronizer.vala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index cd27a670..ccaa4888 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -103,13 +103,13 @@ private class Geary.ImapEngine.AccountSynchronizer : } } - private void old_messages_background_cleanup(GLib.Cancellable cancellable) { + private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { if (this.account.is_open()) { SyncDetachMonitor monitor = new SyncDetachMonitor(); send_all(this.account.list_folders(), false, true, monitor); monitor.initialised = true; monitor.completed.connect((messages_detached) => { - if (cancellable.is_cancelled()) + if (cancellable != null && cancellable.is_cancelled()) return; // Run GC. Reap is forced if messages were detached. Vacuum From 0e96fa95fbb5ae3200da83290d8fa402c299143e Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Tue, 21 Jan 2020 17:57:33 +0100 Subject: [PATCH 38/57] Store last cleanup time in GarbageCollectionTable --- sql/meson.build | 1 + sql/version-026.sql | 4 ++ src/engine/api/geary-account.vala | 14 ++++- src/engine/imap-db/imap-db-account.vala | 51 +++++++++++++++++++ .../imap-engine-generic-account.vala | 5 ++ 5 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 sql/version-026.sql diff --git a/sql/meson.build b/sql/meson.build index 126f2982..9a629380 100644 --- a/sql/meson.build +++ b/sql/meson.build @@ -24,6 +24,7 @@ sql_files = [ 'version-023.sql', 'version-024.sql', 'version-025.sql', + 'version-026.sql', ] install_data(sql_files, diff --git a/sql/version-026.sql b/sql/version-026.sql new file mode 100644 index 00000000..55ab4fb0 --- /dev/null +++ b/sql/version-026.sql @@ -0,0 +1,4 @@ +-- +-- Track when account storage was last cleaned. +-- +ALTER TABLE GarbageCollectionTable ADD COLUMN last_cleanup_time_t INTEGER DEFAULT NULL; diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index 628a14ec..432fa364 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -155,7 +155,14 @@ public abstract class Geary.Account : BaseObject, Logging.Source { * (in ImapDB.GC) has past, a GC reap is performed * 3. GC vacuum is run if recommended */ - public DateTime? last_storage_cleanup { get; set; default = null; } + public GLib.DateTime? last_storage_cleanup { + get { return this._last_storage_cleanup; } + set { + this._last_storage_cleanup = value; + this.last_storage_cleanup_changed(value); + } + } + private GLib.DateTime? _last_storage_cleanup = null; public signal void opened(); @@ -272,6 +279,11 @@ public abstract class Geary.Account : BaseObject, Logging.Source { public signal void email_flags_changed(Geary.Folder folder, Gee.Map map); + /** + * Fired when last_storage_cleanup changes. + */ + public signal void last_storage_cleanup_changed(GLib.DateTime? new_value); + /** {@inheritDoc} */ public Logging.Flag logging_flags { get; protected set; default = Logging.Flag.ALL; diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index 2439df06..602b4c2d 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -382,6 +382,57 @@ private class Geary.ImapDB.Account : BaseObject { return create_local_folder(path, folder_id, properties); } + /** + * Fetch the last time the account cleanup was run. + */ + public async GLib.DateTime? fetch_last_cleanup_async(Cancellable? cancellable) + throws Error { + check_open(); + + int64 last_cleanup_time_t = -1; + yield db.exec_transaction_async(Db.TransactionType.RO, (cx) => { + Db.Result result = cx.query(""" + SELECT last_cleanup_time_t + FROM GarbageCollectionTable + WHERE id = 0 + """); + + if (result.finished) + return Db.TransactionOutcome.FAILURE; + + last_cleanup_time_t = !result.is_null_at(0) ? result.int64_at(0) : -1; + + return Db.TransactionOutcome.SUCCESS; + }, cancellable); + + return (last_cleanup_time_t >= 0) ? new DateTime.from_unix_local(last_cleanup_time_t) : null; + } + + /** + * Set the last time the account cleanup was run. + */ + public async void set_last_cleanup_async(GLib.DateTime? dt, Cancellable? cancellable) + throws Error { + check_open(); + + yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { + Db.Statement stmt = cx.prepare(""" + UPDATE GarbageCollectionTable + SET last_cleanup_time_t = ? + WHERE id = 0 + """); + if (dt != null) { + stmt.bind_int64(0, dt.to_unix()); + } else { + stmt.bind_null(0); + } + + stmt.exec(cancellable); + + return Db.TransactionOutcome.COMMIT; + }, cancellable); + } + private Geary.ImapDB.Folder? get_local_folder(Geary.FolderPath path) { FolderReference? folder_ref = folder_refs.get(path); if (folder_ref == null) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 75adcb10..89881c9b 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -140,6 +140,11 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { throw err; } + this.last_storage_cleanup = yield this.local.fetch_last_cleanup_async(cancellable); + this.last_storage_cleanup_changed.connect ((dt) => { + this.local.set_last_cleanup_async.begin(dt, cancellable); + }); + this.open = true; notify_opened(); From 4875b6648520971b5a4311ce0dacfc0fa807e25d Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Tue, 21 Jan 2020 18:22:11 +0100 Subject: [PATCH 39/57] Bump schema version in database test case --- test/engine/imap-db/imap-db-database-test.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/engine/imap-db/imap-db-database-test.vala b/test/engine/imap-db/imap-db-database-test.vala index 1341f2d8..b72e58f5 100644 --- a/test/engine/imap-db/imap-db-database-test.vala +++ b/test/engine/imap-db/imap-db-database-test.vala @@ -106,7 +106,7 @@ class Geary.ImapDB.DatabaseTest : TestCase { ); db.open.end(async_result()); - assert_int(25, db.get_schema_version(), "Post-upgrade version"); + assert_int(26, db.get_schema_version(), "Post-upgrade version"); // Since schema v22 deletes the re-creates all attachments, // attachment 12 should no longer exist on the file system and From 150417727e6abf77c94fda20e00c6bdf099fc3f9 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 22 Jan 2020 10:23:13 +0100 Subject: [PATCH 40/57] Test case for detach_emails_before_timestamp ie. ImapDB.Folder::detach_emails_before_timestamp. --- test/engine/imap-db/imap-db-folder-test.vala | 65 ++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/engine/imap-db/imap-db-folder-test.vala b/test/engine/imap-db/imap-db-folder-test.vala index 496afd10..11035cb8 100644 --- a/test/engine/imap-db/imap-db-folder-test.vala +++ b/test/engine/imap-db/imap-db-folder-test.vala @@ -26,6 +26,7 @@ class Geary.ImapDB.FolderTest : TestCase { //add_test("merge_existing_preview", merge_existing_preview); add_test("set_flags", set_flags); add_test("set_flags_on_deleted", set_flags_on_deleted); + add_test("detach_emails_before_timestamp", detach_emails_before_timestamp); } public override void set_up() throws GLib.Error { @@ -323,6 +324,70 @@ class Geary.ImapDB.FolderTest : TestCase { assert_flags(test, test_flags); } + public void detach_emails_before_timestamp() throws GLib.Error { + // Ensures that messages outside the folder and within the epoch aren't + // removed, and that messages meeting the criteria are removed. + + this.account.db.exec( + "INSERT INTO FolderTable (id, name) VALUES (2, 'other');" + ); + + GLib.DateTime threshold = new GLib.DateTime.local(2020, 1, 1, 0, 0, 0); + GLib.DateTime beyond_threshold = new GLib.DateTime.local(2019, 1, 1, 0, 0, 0); + GLib.DateTime within_threshold = new GLib.DateTime.local(2021, 1, 1, 0, 0, 0); + + Email.Field fixture_fields = Email.Field.RECEIVERS; + string fixture_to = "test1@example.com"; + this.account.db.exec( + "INSERT INTO MessageTable (id, fields, to_field, internaldate_time_t) " + + "VALUES (1, %d, '%s', %s);".printf(fixture_fields, + fixture_to, + within_threshold.to_unix().to_string()) + ); + this.account.db.exec( + "INSERT INTO MessageTable (id, fields, to_field, internaldate_time_t) " + + "VALUES (2, %d, '%s', %s);".printf(fixture_fields, + fixture_to, + within_threshold.to_unix().to_string()) + ); + this.account.db.exec( + "INSERT INTO MessageTable (id, fields, to_field, internaldate_time_t) " + + "VALUES (3, %d, '%s', %s);".printf(fixture_fields, + fixture_to, + beyond_threshold.to_unix().to_string()) + ); + + this.account.db.exec(""" + INSERT INTO MessageLocationTable + (id, message_id, folder_id, ordering, remove_marker) + VALUES + (1, 1, 1, 1, 1), + (2, 2, 2, 1, 1), + (3, 3, 1, 2, 1); + """); + + this.folder.detach_emails_before_timestamp.begin( + threshold, + null, + (obj, ret) => { async_complete(ret); } + ); + this.folder.detach_emails_before_timestamp.end(async_result()); + + int64[] expected = { 2, 3 }; + Db.Result result = this.account.db.query( + "SELECT id FROM MessageLocationTable" + ); + + int i = 0; + while (!result.finished) { + assert_true(i < expected.length, "Too many rows"); + assert_int64(expected[i], result.int64_at(0)); + i++; + result.next(); + } + assert_true(i == expected.length, "Not enough rows"); + } + private Email new_mock_remote_email(int64 uid, string? subject = null, Geary.EmailFlags? flags = null) { From e7d3b46b37ac97efd7cdc694bafd9e05751e2dc5 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 22 Jan 2020 12:19:16 +0100 Subject: [PATCH 41/57] Temporarily disable test --- test/engine/imap-db/imap-db-folder-test.vala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/engine/imap-db/imap-db-folder-test.vala b/test/engine/imap-db/imap-db-folder-test.vala index 11035cb8..1c5bc53c 100644 --- a/test/engine/imap-db/imap-db-folder-test.vala +++ b/test/engine/imap-db/imap-db-folder-test.vala @@ -26,7 +26,8 @@ class Geary.ImapDB.FolderTest : TestCase { //add_test("merge_existing_preview", merge_existing_preview); add_test("set_flags", set_flags); add_test("set_flags_on_deleted", set_flags_on_deleted); - add_test("detach_emails_before_timestamp", detach_emails_before_timestamp); + // TODO temporarily disabled until fixed + //add_test("detach_emails_before_timestamp", detach_emails_before_timestamp); } public override void set_up() throws GLib.Error { From 01e114f09aa54f310d1c54e0eb1f516043cc4f63 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Mon, 10 Feb 2020 16:04:13 +0100 Subject: [PATCH 42/57] Fix detach_emails_before_timestamp test case That's ImapDB.Folder::detach_emails_before_timestamp, fixed and enabled. --- test/engine/imap-db/imap-db-folder-test.vala | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/engine/imap-db/imap-db-folder-test.vala b/test/engine/imap-db/imap-db-folder-test.vala index 1c5bc53c..7bd02384 100644 --- a/test/engine/imap-db/imap-db-folder-test.vala +++ b/test/engine/imap-db/imap-db-folder-test.vala @@ -26,8 +26,7 @@ class Geary.ImapDB.FolderTest : TestCase { //add_test("merge_existing_preview", merge_existing_preview); add_test("set_flags", set_flags); add_test("set_flags_on_deleted", set_flags_on_deleted); - // TODO temporarily disabled until fixed - //add_test("detach_emails_before_timestamp", detach_emails_before_timestamp); + add_test("detach_emails_before_timestamp", detach_emails_before_timestamp); } public override void set_up() throws GLib.Error { @@ -374,7 +373,7 @@ class Geary.ImapDB.FolderTest : TestCase { ); this.folder.detach_emails_before_timestamp.end(async_result()); - int64[] expected = { 2, 3 }; + int64[] expected = { 1, 2 }; Db.Result result = this.account.db.query( "SELECT id FROM MessageLocationTable" ); From 4d4c1943cfead30deccea11fc77a32d0d9c08b40 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 7 May 2020 12:26:12 +1000 Subject: [PATCH 43/57] Whitespace --- src/engine/imap-db/imap-db-database.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 36a32b54..5e32fc6c 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -66,7 +66,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { this.attachments_path = attachments_path; this.upgrade_monitor = upgrade_monitor; this.vacuum_monitor = vacuum_monitor; - } + } /** * Prepares the ImapDB database for use. From 9bc128ee78aec66fe3ca24251b48b8806a28e63e Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 7 May 2020 15:48:45 +1000 Subject: [PATCH 44/57] Demote log message --- src/engine/imap-db/imap-db-database.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index 5e32fc6c..f3dfc6b9 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -96,7 +96,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { throws Error { if (this.gc != null) { - message("GC abandoned, possibly already running"); + debug("GC abandoned, possibly already running"); return; } From 6464393c32475c72c64399246e3656d8a3332858 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 7 May 2020 15:58:29 +1000 Subject: [PATCH 45/57] Improve Geary.ImapDB.Database.run_gc method interface --- src/engine/imap-db/imap-db-database.vala | 56 ++++++++++++------- .../imap-engine-account-synchronizer.vala | 15 +++-- .../imap-engine-generic-account.vala | 2 +- .../imap-engine-minimal-folder.vala | 3 +- 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index f3dfc6b9..c119b9b6 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -18,6 +18,26 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { /** SQLite UTF-8 collation name. */ public const string UTF8_COLLATE = "UTF8COLL"; + /** Options to use when running garbage collection. */ + [Flags] + public enum GarbageCollectionOptions { + + /** Reaping will not be forced and vacuuming not permitted. */ + NONE, + + /** + * Reaping will be performed, regardless of recommendation. + */ + FORCE_REAP, + + /** + * Whether to permit database vacuum. + * + * Vacuuming is performed in the foreground. + */ + ALLOW_VACUUM; + } + public bool want_background_vacuum { get; set; default = false; } @@ -57,6 +77,9 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private GC? gc = null; private Cancellable gc_cancellable = new Cancellable(); + + + public Database(GLib.File db_file, GLib.File schema_dir, GLib.File attachments_path, @@ -75,7 +98,8 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { throws Error { yield base.open(flags, cancellable); - yield run_gc(cancellable); + Geary.ClientService services_to_pause[] = {}; + yield run_gc(NONE, services_to_pause, cancellable); } /** @@ -83,16 +107,10 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { * * Reap should only be forced when there is known cleanup to perform and * the interval based recommendation should be bypassed. - * - * TODO Passing of the services is a WIP hack. It is currently used to both - * signify that it's an appropriate time to run a vacuum (ie. we're - * idle in the background) and provide access for stopping IMAP. */ - public async void run_gc(GLib.Cancellable? cancellable, - bool force_reap = false, - bool allow_vacuum = false, - Geary.Imap.ClientService? imap_service = null, - Geary.Smtp.ClientService? smtp_service = null) + public async void run_gc(GarbageCollectionOptions options, + Geary.ClientService[] services_to_pause, + GLib.Cancellable? cancellable) throws Error { if (this.gc != null) { @@ -116,12 +134,11 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { // VACUUM needs to execute in the foreground with the user given a busy prompt (and cannot // be run at the same time as REAP) if ((recommended & GC.RecommendedOperation.VACUUM) != 0) { - if (allow_vacuum) { + if (GarbageCollectionOptions.ALLOW_VACUUM in options) { this.want_background_vacuum = false; - if (imap_service != null) - yield imap_service.stop(gc_cancellable); - if (smtp_service != null) - yield smtp_service.stop(gc_cancellable); + foreach (ClientService service in services_to_pause) { + yield service.stop(gc_cancellable); + } if (!vacuum_monitor.is_in_progress) vacuum_monitor.notify_start(); @@ -138,10 +155,9 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { vacuum_monitor.notify_finish(); } - if (imap_service != null) - yield imap_service.start(gc_cancellable); - if (smtp_service != null) - yield smtp_service.start(gc_cancellable); + foreach (ClientService service in services_to_pause) { + yield service.start(gc_cancellable); + } } else { // Flag a vacuum to run later when we've been idle in the background debug("Flagging desire to GC vacuum"); @@ -156,7 +172,7 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { } // REAP can run in the background while the application is executing - if (force_reap || (recommended & GC.RecommendedOperation.REAP) != 0) { + if (GarbageCollectionOptions.FORCE_REAP in options || (recommended & GC.RecommendedOperation.REAP) != 0) { // run in the background and allow application to continue running this.gc.reap_async.begin(gc_cancellable, on_reap_async_completed); } else { diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 8ddd353f..86a72967 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -109,11 +109,13 @@ private class Geary.ImapEngine.AccountSynchronizer : // Run GC. Reap is forced if messages were detached. Vacuum // is allowed as we're running in the background. - account.local.db.run_gc.begin(cancellable, - messages_detached, - true, - account.imap, - account.smtp); + Geary.ImapDB.Database.GarbageCollectionOptions options = ALLOW_VACUUM; + if (messages_detached) { + options |= FORCE_REAP; + } + account.local.db.run_gc.begin(options, + {account.imap, account.smtp}, + cancellable); }); } } @@ -448,7 +450,8 @@ private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation // Run basic GC GenericAccount generic_account = (GenericAccount) account; - yield generic_account.local.db.run_gc(cancellable); + Geary.ClientService services_to_pause[] = {}; + yield generic_account.local.db.run_gc(NONE, services_to_pause, cancellable); } public override bool equal_to(AccountOperation op) { diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 1c3097b0..a192ca47 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -583,7 +583,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { this.old_messages_background_cleanup_request(cancellable); } else if (local.db.want_background_vacuum) { // Vacuum has been flagged as needed, run it - local.db.run_gc.begin(cancellable, false, true, this.imap, this.smtp); + local.db.run_gc.begin(ALLOW_VACUUM, {this.imap, this.smtp}, cancellable); } } diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala b/src/engine/imap-engine/imap-engine-minimal-folder.vala index cc510b1f..20fdc2c2 100644 --- a/src/engine/imap-engine/imap-engine-minimal-folder.vala +++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala @@ -1299,7 +1299,8 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport // expunge from the remote yield this.replay_queue.checkpoint(cancellable); - yield this._account.local.db.run_gc(cancellable); + Geary.ClientService services_to_pause[] = {}; + yield this._account.local.db.run_gc(NONE, services_to_pause, cancellable); } private void check_open(string method) throws EngineError { From 7153b69bd755f01741500be87fb580845ca71432 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 7 May 2020 19:51:15 +1000 Subject: [PATCH 46/57] Change Geary.Account.last_storage_cleanup to automatic property --- src/engine/api/geary-account.vala | 15 +-------------- .../imap-engine/imap-engine-generic-account.vala | 11 ++++++++--- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala index 34d67a30..4a4850a5 100644 --- a/src/engine/api/geary-account.vala +++ b/src/engine/api/geary-account.vala @@ -155,15 +155,7 @@ public abstract class Geary.Account : BaseObject, Logging.Source { * (in ImapDB.GC) has past, a GC reap is performed * 3. GC vacuum is run if recommended */ - public GLib.DateTime? last_storage_cleanup { - get { return this._last_storage_cleanup; } - set { - this._last_storage_cleanup = value; - this.last_storage_cleanup_changed(value); - } - } - private GLib.DateTime? _last_storage_cleanup = null; - + public GLib.DateTime? last_storage_cleanup { get; set; } public signal void opened(); @@ -279,11 +271,6 @@ public abstract class Geary.Account : BaseObject, Logging.Source { public signal void email_flags_changed(Geary.Folder folder, Gee.Map map); - /** - * Fired when last_storage_cleanup changes. - */ - public signal void last_storage_cleanup_changed(GLib.DateTime? new_value); - /** {@inheritDoc} */ public Logging.Source? logging_parent { get { return null; } } diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index a192ca47..6c5f1d2d 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -141,9 +141,7 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { } this.last_storage_cleanup = yield this.local.fetch_last_cleanup_async(cancellable); - this.last_storage_cleanup_changed.connect ((dt) => { - this.local.set_last_cleanup_async.begin(dt, cancellable); - }); + this.notify["last_storage_cleanup"].connect(on_last_storage_cleanup_notify); this.open = true; notify_opened(); @@ -1004,6 +1002,13 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { } } + private void on_last_storage_cleanup_notify() { + this.local.set_last_cleanup_async.begin( + this.last_storage_cleanup, + this.open_cancellable + ); + } + } From cfd88f7f177a266a4ec633143bab79cc83423ada Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Thu, 7 May 2020 19:59:16 +1000 Subject: [PATCH 47/57] Remove unnecessary var in Application.Controller --- src/client/application/application-controller.vala | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/client/application/application-controller.vala b/src/client/application/application-controller.vala index dc9aa23f..6919328a 100644 --- a/src/client/application/application-controller.vala +++ b/src/client/application/application-controller.vala @@ -95,9 +95,6 @@ internal class Application.Controller : Geary.BaseObject { // Timeout to do work in idle after all windows have been sent to the background private Geary.TimeoutManager all_windows_backgrounded_timeout; - // Track whether storage cleanup is running - private bool storage_cleanup_running = false; - private GLib.Cancellable? storage_cleanup_cancellable; @@ -1713,13 +1710,12 @@ internal class Application.Controller : Geary.BaseObject { this.all_windows_backgrounded_timeout.reset(); window_focus_out(); - if (!storage_cleanup_running) + if (this.storage_cleanup_cancellable == null) do_background_storage_cleanup.begin(); } private async void do_background_storage_cleanup() { debug("Checking for backgrounded idle work"); - storage_cleanup_running = true; this.storage_cleanup_cancellable = new GLib.Cancellable(); foreach (AccountContext context in this.accounts.values) { @@ -1730,7 +1726,7 @@ internal class Application.Controller : Geary.BaseObject { break; context.cancellable.cancelled.disconnect(this.storage_cleanup_cancellable.cancel); } - storage_cleanup_running = false; + this.storage_cleanup_cancellable = null; } } From cce66cba8f3880629fd8aed390050477db57a75e Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Mon, 11 May 2020 18:52:06 +1000 Subject: [PATCH 48/57] Don't wait for GC reap to clean search table after old message cleanup --- src/engine/imap-db/imap-db-folder.vala | 53 +++++++++++++++++++------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 80a96c88..1ae905c4 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -886,7 +886,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { public async Gee.Collection? detach_emails_before_timestamp(DateTime cutoff, GLib.Cancellable? cancellable) throws Error { debug("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); - Gee.Collection? deleted_email_ids = null; + Gee.ArrayList? deleted_email_ids = null; Gee.ArrayList deleted_primary_keys = null; yield db.exec_transaction_async(Db.TransactionType.RO, (cx) => { @@ -913,7 +913,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { while (!results.finished) { if (deleted_email_ids == null) { - deleted_email_ids = new Gee.ArrayList(); + deleted_email_ids = new Gee.ArrayList(); deleted_primary_keys = new Gee.ArrayList(); } @@ -935,25 +935,50 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { while (delete_index < deleted_primary_keys.size) { int batch_counter = 0; - yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { - StringBuilder ids_sql_sublist = new StringBuilder(); - while (delete_index < deleted_primary_keys.size - && batch_counter < OLD_MSG_DETACH_BATCH_SIZE) { - if (batch_counter > 0) - ids_sql_sublist.append(","); - ids_sql_sublist.append( - deleted_primary_keys.get(delete_index) - ); - delete_index++; - batch_counter++; + StringBuilder message_location_ids_sql_sublist = new StringBuilder(); + StringBuilder message_ids_sql_sublist = new StringBuilder(); + while (delete_index < deleted_primary_keys.size + && batch_counter < OLD_MSG_DETACH_BATCH_SIZE) { + if (batch_counter > 0) { + message_location_ids_sql_sublist.append(","); + message_ids_sql_sublist.append(","); } + message_location_ids_sql_sublist.append( + deleted_primary_keys.get(delete_index) + ); + message_ids_sql_sublist.append( + deleted_email_ids.get(delete_index).message_id.to_string() + ); + delete_index++; + batch_counter++; + } + + yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { + StringBuilder sql = new StringBuilder(); sql.append(""" DELETE FROM MessageLocationTable WHERE id IN ( """); - sql.append(ids_sql_sublist.str); + sql.append(message_location_ids_sql_sublist.str); + sql.append(")"); + Db.Statement stmt = cx.prepare(sql.str); + + stmt.exec(cancellable); + + return Db.TransactionOutcome.COMMIT; + + }, cancellable); + + yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { + + StringBuilder sql = new StringBuilder(); + sql.append(""" + DELETE FROM MessageSearchTable + WHERE docid IN ( + """); + sql.append(message_ids_sql_sublist.str); sql.append(")"); Db.Statement stmt = cx.prepare(sql.str); From 19353900161c458c7db255c42d1afacc60c5ec12 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Mon, 11 May 2020 18:53:00 +1000 Subject: [PATCH 49/57] Fix log typo --- src/engine/imap-db/imap-db-folder.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 1ae905c4..16750597 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -885,7 +885,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { public async Gee.Collection? detach_emails_before_timestamp(DateTime cutoff, GLib.Cancellable? cancellable) throws Error { - debug("Detaching emails before %s for folder ID %", cutoff.to_string(), this.folder_id.to_string()); + debug("Detaching emails before %s for folder ID %s", cutoff.to_string(), this.folder_id.to_string()); Gee.ArrayList? deleted_email_ids = null; Gee.ArrayList deleted_primary_keys = null; From 745e4bef5846f962c29038e6e0696012788c0962 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 13 May 2020 10:11:16 +1000 Subject: [PATCH 50/57] Oops, remove whitespace --- src/engine/imap-db/imap-db-database.vala | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index c119b9b6..921189d1 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -77,9 +77,6 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { private GC? gc = null; private Cancellable gc_cancellable = new Cancellable(); - - - public Database(GLib.File db_file, GLib.File schema_dir, GLib.File attachments_path, From 230502aaa906590a1ce27ae849582f8724daa768 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 13 May 2020 10:15:18 +1000 Subject: [PATCH 51/57] Combine split old message detachment transactions into single --- src/engine/imap-db/imap-db-folder.vala | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 16750597..9bfd6c8e 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -954,8 +954,6 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { } yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { - - StringBuilder sql = new StringBuilder(); sql.append(""" DELETE FROM MessageLocationTable @@ -967,25 +965,18 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { stmt.exec(cancellable); - return Db.TransactionOutcome.COMMIT; - - }, cancellable); - - yield db.exec_transaction_async(Db.TransactionType.WO, (cx) => { - - StringBuilder sql = new StringBuilder(); + sql = new StringBuilder(); sql.append(""" DELETE FROM MessageSearchTable WHERE docid IN ( """); sql.append(message_ids_sql_sublist.str); sql.append(")"); - Db.Statement stmt = cx.prepare(sql.str); + stmt = cx.prepare(sql.str); stmt.exec(cancellable); return Db.TransactionOutcome.COMMIT; - }, cancellable); } } From b81389be33286c2dc4effff2e52497ceaafa035b Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 13 May 2020 15:17:51 +1000 Subject: [PATCH 52/57] Improvements to lifecycle of SyncDetachMonitor Reference counts are now sane for a clean run where we don't exit while the monitor is waiting --- .../imap-engine-account-synchronizer.vala | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index 86a72967..ac08c937 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -16,6 +16,7 @@ private class Geary.ImapEngine.AccountSynchronizer : private DateTime max_epoch = new DateTime( new TimeZone.local(), 2000, 1, 1, 0, 0, 0.0 ); + private SyncDetachMonitor sync_detach_monitor = null; public AccountSynchronizer(GenericAccount account) { @@ -99,13 +100,15 @@ private class Geary.ImapEngine.AccountSynchronizer : } private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { - if (this.account.is_open()) { - SyncDetachMonitor monitor = new SyncDetachMonitor(); - send_all(this.account.list_folders(), false, true, monitor); - monitor.initialised = true; - monitor.completed.connect((messages_detached) => { - if (cancellable != null && cancellable.is_cancelled()) + if (this.account.is_open() && this.sync_detach_monitor == null) { + this.sync_detach_monitor = new SyncDetachMonitor(); + send_all(this.account.list_folders(), false, true, this.sync_detach_monitor); + this.sync_detach_monitor.initialised = true; + this.sync_detach_monitor.completed.connect((messages_detached) => { + if (cancellable != null && cancellable.is_cancelled()) { + this.sync_detach_monitor = null; return; + } // Run GC. Reap is forced if messages were detached. Vacuum // is allowed as we're running in the background. @@ -116,6 +119,7 @@ private class Geary.ImapEngine.AccountSynchronizer : account.local.db.run_gc.begin(options, {account.imap, account.smtp}, cancellable); + this.sync_detach_monitor = null; }); } } @@ -483,8 +487,8 @@ private class SyncDetachMonitor: Geary.BaseObject { operation.old_message_detached.connect(() => { this.messages_detached = true; }); - operation.completed.connect(() => { - this.operations.remove(operation); + operation.completed.connect((op) => { + operations.remove((Geary.ImapEngine.CheckFolderSync) op); if (initialised && operations.length() == 0) { this.completed(this.messages_detached); } From d34832cb366fb7de44e559a962ef1f9900a10419 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Wed, 13 May 2020 15:43:40 +1000 Subject: [PATCH 53/57] Improved declaration --- src/engine/imap-engine/imap-engine-account-synchronizer.vala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index ac08c937..cded00bd 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -16,7 +16,7 @@ private class Geary.ImapEngine.AccountSynchronizer : private DateTime max_epoch = new DateTime( new TimeZone.local(), 2000, 1, 1, 0, 0, 0.0 ); - private SyncDetachMonitor sync_detach_monitor = null; + private SyncDetachMonitor? sync_detach_monitor = null; public AccountSynchronizer(GenericAccount account) { From a8553599647062082dbe8bfd193300f5c0c40ad8 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Sun, 17 May 2020 12:00:19 +1000 Subject: [PATCH 54/57] Switch monitor of background message cleanup to an AccountOperation --- .../imap-engine-account-synchronizer.vala | 109 ++++++++++-------- 1 file changed, 60 insertions(+), 49 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index cded00bd..f175ceea 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -16,7 +16,7 @@ private class Geary.ImapEngine.AccountSynchronizer : private DateTime max_epoch = new DateTime( new TimeZone.local(), 2000, 1, 1, 0, 0, 0.0 ); - private SyncDetachMonitor? sync_detach_monitor = null; + private bool background_idle_gc_scheduled = false; public AccountSynchronizer(GenericAccount account) { @@ -49,8 +49,9 @@ private class Geary.ImapEngine.AccountSynchronizer : private void send_all(Gee.Collection folders, bool became_available, bool for_storage_clean=false, - SyncDetachMonitor? monitor=null) { + GarbageCollectPostIdleMessageDetach? post_idle_detach_op=null) { + bool add_post_idle_detach_op = false; foreach (Folder folder in folders) { // Only sync folders that: // 1. Can actually be opened (i.e. are selectable) @@ -72,11 +73,13 @@ private class Geary.ImapEngine.AccountSynchronizer : this.account, imap_folder, this.max_epoch, - for_storage_clean + for_storage_clean, + post_idle_detach_op ); - if (monitor != null) - monitor.add(check_op); op = check_op; + if (post_idle_detach_op != null) { + add_post_idle_detach_op = true; + } } else { op = new RefreshFolderSync(this.account, imap_folder); } @@ -88,6 +91,15 @@ private class Geary.ImapEngine.AccountSynchronizer : } } } + + // Add GC operation after message removal during background cleanup + if (add_post_idle_detach_op) { + try { + this.account.queue_operation(post_idle_detach_op); + } catch (Error err) { + warning("Failed to queue sync operation: %s", err.message); + } + } } private void do_prefetch_changed() { @@ -100,27 +112,18 @@ private class Geary.ImapEngine.AccountSynchronizer : } private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { - if (this.account.is_open() && this.sync_detach_monitor == null) { - this.sync_detach_monitor = new SyncDetachMonitor(); - send_all(this.account.list_folders(), false, true, this.sync_detach_monitor); - this.sync_detach_monitor.initialised = true; - this.sync_detach_monitor.completed.connect((messages_detached) => { - if (cancellable != null && cancellable.is_cancelled()) { - this.sync_detach_monitor = null; - return; - } - // Run GC. Reap is forced if messages were detached. Vacuum - // is allowed as we're running in the background. - Geary.ImapDB.Database.GarbageCollectionOptions options = ALLOW_VACUUM; - if (messages_detached) { - options |= FORCE_REAP; - } - account.local.db.run_gc.begin(options, - {account.imap, account.smtp}, - cancellable); - this.sync_detach_monitor = null; + if (this.account.is_open() && !this.background_idle_gc_scheduled) { + this.background_idle_gc_scheduled = true; + GarbageCollectPostIdleMessageDetach op = + new GarbageCollectPostIdleMessageDetach(account); + op.completed.connect(() => { + this.background_idle_gc_scheduled = false; }); + cancellable.cancelled.connect(() => { + this.background_idle_gc_scheduled = false; + }); + send_all(this.account.list_folders(), false, true, op); } } @@ -254,19 +257,20 @@ private class Geary.ImapEngine.RefreshFolderSync : FolderOperation { */ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { - public signal void old_message_detached(GLib.Cancellable cancellable); - private DateTime sync_max_epoch; private bool for_storage_clean; + private GarbageCollectPostIdleMessageDetach? post_idle_detach_op; internal CheckFolderSync(GenericAccount account, MinimalFolder folder, DateTime sync_max_epoch, - bool for_storage_clean) { + bool for_storage_clean, + GarbageCollectPostIdleMessageDetach? post_idle_detach_op) { base(account, folder); this.sync_max_epoch = sync_max_epoch; this.for_storage_clean = for_storage_clean; + this.post_idle_detach_op = post_idle_detach_op; } protected override async void sync_folder(Cancellable cancellable) @@ -291,7 +295,9 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { cancellable); if (detached_ids != null) { this.folder.email_locally_removed(detached_ids); - old_message_detached(cancellable); + if (post_idle_detach_op != null) { + post_idle_detach_op.messages_detached(); + } if (!for_storage_clean) { GenericAccount imap_account = (GenericAccount) account; @@ -439,7 +445,8 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { * * Queues a basic GC run which will run if old messages were detached * after a folder became available. Not used for backgrounded account - * storage operations, which are handled instead by the {@link SyncDetachMonitor}. + * storage operations, which are handled instead by the + * {@link GarbageCollectPostIdleMessageDetach}. */ private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation { @@ -467,31 +474,35 @@ private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation } /** - * Monitor account sync operations until completion. + * Performs garbage collection after old messages have been removed during + * backgrounded idle cleanup. * - * Monitors added {@link CheckFolderSync} operations until completed - * and signals whether messages were detached. + * Queues a GC run after a cleanup of messages has occurred while the + * app is idle in the background. Vacuuming will be permitted and if + * messages have been removed a reap will be forced. */ -private class SyncDetachMonitor: Geary.BaseObject { +private class Geary.ImapEngine.GarbageCollectPostIdleMessageDetach: AccountOperation { - public bool messages_detached { get; private set; default = false; } - public bool initialised { get; set; default = false; } + // Vacuum is allowed as we're running in the background + private Geary.ImapDB.Database.GarbageCollectionOptions options = ALLOW_VACUUM; - private List operations = - new List(); + internal GarbageCollectPostIdleMessageDetach(GenericAccount account) { + base(account); + } - public signal void completed(bool messages_detached); + public override async void execute(GLib.Cancellable cancellable) + throws Error { + if (cancellable.is_cancelled()) + return; - public void add(Geary.ImapEngine.CheckFolderSync operation) { - this.operations.append(operation); - operation.old_message_detached.connect(() => { - this.messages_detached = true; - }); - operation.completed.connect((op) => { - operations.remove((Geary.ImapEngine.CheckFolderSync) op); - if (initialised && operations.length() == 0) { - this.completed(this.messages_detached); - } - }); + GenericAccount generic_account = (GenericAccount) this.account; + generic_account.local.db.run_gc.begin(this.options, + {generic_account.imap, generic_account.smtp}, + cancellable); + } + + public void messages_detached() { + // Reap is forced if messages were detached + this.options |= FORCE_REAP; } } From 2049d7aa6b0bfe768c3768bb7a1a7e5579769d29 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Mon, 18 May 2020 17:57:13 +1000 Subject: [PATCH 55/57] Move queueing of idle garbage collection to calling method --- .../imap-engine-account-synchronizer.vala | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index f175ceea..d9b28cb7 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -51,7 +51,6 @@ private class Geary.ImapEngine.AccountSynchronizer : bool for_storage_clean=false, GarbageCollectPostIdleMessageDetach? post_idle_detach_op=null) { - bool add_post_idle_detach_op = false; foreach (Folder folder in folders) { // Only sync folders that: // 1. Can actually be opened (i.e. are selectable) @@ -77,9 +76,6 @@ private class Geary.ImapEngine.AccountSynchronizer : post_idle_detach_op ); op = check_op; - if (post_idle_detach_op != null) { - add_post_idle_detach_op = true; - } } else { op = new RefreshFolderSync(this.account, imap_folder); } @@ -91,15 +87,6 @@ private class Geary.ImapEngine.AccountSynchronizer : } } } - - // Add GC operation after message removal during background cleanup - if (add_post_idle_detach_op) { - try { - this.account.queue_operation(post_idle_detach_op); - } catch (Error err) { - warning("Failed to queue sync operation: %s", err.message); - } - } } private void do_prefetch_changed() { @@ -117,13 +104,22 @@ private class Geary.ImapEngine.AccountSynchronizer : this.background_idle_gc_scheduled = true; GarbageCollectPostIdleMessageDetach op = new GarbageCollectPostIdleMessageDetach(account); + op.completed.connect(() => { this.background_idle_gc_scheduled = false; }); cancellable.cancelled.connect(() => { this.background_idle_gc_scheduled = false; }); + send_all(this.account.list_folders(), false, true, op); + + // Add GC operation after message removal during background cleanup + try { + this.account.queue_operation(op); + } catch (Error err) { + warning("Failed to queue sync operation: %s", err.message); + } } } From 95c68710d5928ea8e917a9700283a34e4341d084 Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Mon, 18 May 2020 18:04:22 +1000 Subject: [PATCH 56/57] Simplified GC class naming --- .../imap-engine-account-synchronizer.vala | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index d9b28cb7..b79c4312 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -49,7 +49,7 @@ private class Geary.ImapEngine.AccountSynchronizer : private void send_all(Gee.Collection folders, bool became_available, bool for_storage_clean=false, - GarbageCollectPostIdleMessageDetach? post_idle_detach_op=null) { + IdleGarbageCollection? post_idle_detach_op=null) { foreach (Folder folder in folders) { // Only sync folders that: @@ -102,8 +102,7 @@ private class Geary.ImapEngine.AccountSynchronizer : if (this.account.is_open() && !this.background_idle_gc_scheduled) { this.background_idle_gc_scheduled = true; - GarbageCollectPostIdleMessageDetach op = - new GarbageCollectPostIdleMessageDetach(account); + IdleGarbageCollection op = new IdleGarbageCollection(account); op.completed.connect(() => { this.background_idle_gc_scheduled = false; @@ -255,14 +254,14 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { private DateTime sync_max_epoch; private bool for_storage_clean; - private GarbageCollectPostIdleMessageDetach? post_idle_detach_op; + private IdleGarbageCollection? post_idle_detach_op; internal CheckFolderSync(GenericAccount account, MinimalFolder folder, DateTime sync_max_epoch, bool for_storage_clean, - GarbageCollectPostIdleMessageDetach? post_idle_detach_op) { + IdleGarbageCollection? post_idle_detach_op) { base(account, folder); this.sync_max_epoch = sync_max_epoch; this.for_storage_clean = for_storage_clean; @@ -297,8 +296,8 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { if (!for_storage_clean) { GenericAccount imap_account = (GenericAccount) account; - GarbageCollectPostMessageDetach op = - new GarbageCollectPostMessageDetach(imap_account); + ForegroundGarbageCollection op = + new ForegroundGarbageCollection(imap_account); try { imap_account.queue_operation(op); } catch (Error err) { @@ -442,11 +441,11 @@ private class Geary.ImapEngine.CheckFolderSync : RefreshFolderSync { * Queues a basic GC run which will run if old messages were detached * after a folder became available. Not used for backgrounded account * storage operations, which are handled instead by the - * {@link GarbageCollectPostIdleMessageDetach}. + * {@link IdleGarbageCollection}. */ -private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation { +private class Geary.ImapEngine.ForegroundGarbageCollection: AccountOperation { - internal GarbageCollectPostMessageDetach(GenericAccount account) { + internal ForegroundGarbageCollection(GenericAccount account) { base(account); } @@ -477,12 +476,12 @@ private class Geary.ImapEngine.GarbageCollectPostMessageDetach: AccountOperation * app is idle in the background. Vacuuming will be permitted and if * messages have been removed a reap will be forced. */ -private class Geary.ImapEngine.GarbageCollectPostIdleMessageDetach: AccountOperation { +private class Geary.ImapEngine.IdleGarbageCollection: AccountOperation { // Vacuum is allowed as we're running in the background private Geary.ImapDB.Database.GarbageCollectionOptions options = ALLOW_VACUUM; - internal GarbageCollectPostIdleMessageDetach(GenericAccount account) { + internal IdleGarbageCollection(GenericAccount account) { base(account); } From b080f8a49e68d2387118af2bf0fe300e8ffa790a Mon Sep 17 00:00:00 2001 From: Chris Heywood <15127-creywood@users.noreply.gitlab.gnome.org> Date: Fri, 29 May 2020 16:51:27 +1000 Subject: [PATCH 57/57] Don't avoid scheduling a second set of background idle GC work This only happens once a day --- .../imap-engine/imap-engine-account-synchronizer.vala | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala b/src/engine/imap-engine/imap-engine-account-synchronizer.vala index b79c4312..a5c07d1d 100644 --- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala +++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala @@ -16,7 +16,6 @@ private class Geary.ImapEngine.AccountSynchronizer : private DateTime max_epoch = new DateTime( new TimeZone.local(), 2000, 1, 1, 0, 0, 0.0 ); - private bool background_idle_gc_scheduled = false; public AccountSynchronizer(GenericAccount account) { @@ -100,17 +99,9 @@ private class Geary.ImapEngine.AccountSynchronizer : private void old_messages_background_cleanup(GLib.Cancellable? cancellable) { - if (this.account.is_open() && !this.background_idle_gc_scheduled) { - this.background_idle_gc_scheduled = true; + if (this.account.is_open()) { IdleGarbageCollection op = new IdleGarbageCollection(account); - op.completed.connect(() => { - this.background_idle_gc_scheduled = false; - }); - cancellable.cancelled.connect(() => { - this.background_idle_gc_scheduled = false; - }); - send_all(this.account.list_folders(), false, true, op); // Add GC operation after message removal during background cleanup