From d9e23d553edf0b80f79ece6fbb25ab4b888143f6 Mon Sep 17 00:00:00 2001 From: Michael James Gratton Date: Fri, 24 Nov 2017 15:39:14 +1100 Subject: [PATCH] Convert folder unseen update into an account op, only schedule if closed. If the folder is open, we should be getting notifications of flag changes from the server, so double-checking is kind of pointless, and since it's implemented using an IMAP STATUS, also expensive for the server. * src/engine/imap-engine/imap-engine-generic-account.vala (GenericAccount): Convert refresh_unseen_async method into a new RefreshFolderUnseen account operation, add a guard to ensure the folder is closed when executing. Remove hash map of timeouts and replace with a single timeout manager on MinimalFolder itself. * src/engine/imap-engine/imap-engine-minimal-folder.vala (MinimalFolder): Add timeout manager for unseen update, only start it running if closed, when triggered, schedule a RefreshFolderUnseen op on the account. --- .../imap-engine-generic-account.vala | 196 +++++++++--------- .../imap-engine-minimal-folder.vala | 47 ++++- 2 files changed, 142 insertions(+), 101 deletions(-) diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala b/src/engine/imap-engine/imap-engine-generic-account.vala index 078ad54e..336b7149 100644 --- a/src/engine/imap-engine/imap-engine-generic-account.vala +++ b/src/engine/imap-engine/imap-engine-generic-account.vala @@ -9,7 +9,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { private const int REFRESH_FOLDER_LIST_SEC = 2 * 60; - private const int REFRESH_UNSEEN_SEC = 1; private const Geary.SpecialFolderType[] SUPPORTED_SPECIAL_FOLDERS = { Geary.SpecialFolderType.DRAFTS, Geary.SpecialFolderType.SENT, @@ -28,9 +27,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { private Gee.HashMap folder_map = new Gee.HashMap< FolderPath, MinimalFolder>(); private Gee.HashMap local_only = new Gee.HashMap(); - private Gee.HashMap refresh_unseen_timeout_ids - = new Gee.HashMap(); - private Gee.HashSet in_refresh_unseen = new Gee.HashSet(); private AccountProcessor? processor; private AccountSynchronizer sync; private TimeoutManager refresh_folder_timer; @@ -111,28 +107,28 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { } } } - + protected override void notify_email_appended(Geary.Folder folder, Gee.Collection ids) { base.notify_email_appended(folder, ids); - reschedule_unseen_update(folder); + schedule_unseen_update(folder); } - + protected override void notify_email_inserted(Geary.Folder folder, Gee.Collection ids) { base.notify_email_inserted(folder, ids); - reschedule_unseen_update(folder); + schedule_unseen_update(folder); } - + protected override void notify_email_removed(Geary.Folder folder, Gee.Collection ids) { base.notify_email_removed(folder, ids); - reschedule_unseen_update(folder); + schedule_unseen_update(folder); } - + protected override void notify_email_flags_changed(Geary.Folder folder, Gee.Map flag_map) { base.notify_email_flags_changed(folder, flag_map); - reschedule_unseen_update(folder); + schedule_unseen_update(folder); } - + private void check_open() throws EngineError { if (!open) throw new EngineError.OPEN_REQUIRED("Account %s not opened", to_string()); @@ -348,93 +344,10 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { return all_folders; } - // Subclasses should implement this to return their flavor of a MinimalFolder with the - // appropriate interfaces attached. The returned folder should have its SpecialFolderType - // set using either the properties from the local folder or its path. - // - // This won't be called to build the Outbox or search folder, but for all others (including Inbox) it will. - protected abstract MinimalFolder new_folder(ImapDB.Folder local_folder); - - /** - * Hooks up and queues an {@link UpdateRemoteFolders} operation. - */ - private void update_remote_folders() { - UpdateRemoteFolders op = new UpdateRemoteFolders( - this, - this.remote, - this.local, - this.local_only.keys, - get_supported_special_folders() - ); - op.completed.connect(() => { - this.refresh_folder_timer.start(); - }); - try { - queue_operation(op); - } catch (Error err) { - // oh well - } - } - - private void reschedule_unseen_update(Geary.Folder folder) { - if (!folder_map.has_key(folder.path)) - return; - - if (refresh_unseen_timeout_ids.get(folder.path) != 0) - Source.remove(refresh_unseen_timeout_ids.get(folder.path)); - - refresh_unseen_timeout_ids.set(folder.path, - Timeout.add_seconds(REFRESH_UNSEEN_SEC, () => on_refresh_unseen(folder))); - } - - private bool on_refresh_unseen(Geary.Folder folder) { - // If we're in the process already, reschedule for later. - if (in_refresh_unseen.contains(folder)) - return true; - - // add here, remove in completed callback - in_refresh_unseen.add(folder); - - refresh_unseen_async.begin(folder, null, on_refresh_unseen_completed); - - refresh_unseen_timeout_ids.unset(folder.path); - return false; - } - - private void on_refresh_unseen_completed(Object? source, AsyncResult result) { - try { - refresh_unseen_async.end(result); - } catch (Error e) { - debug("Error refreshing unseen counts: %s", e.message); - } - } - - private async void refresh_unseen_async(Geary.Folder folder, Cancellable? cancellable) - throws Error { - debug("Refreshing unseen counts for %s", folder.to_string()); - - try { - Imap.Folder remote_folder = yield remote.fetch_folder_cached_async( - folder.path, - true, - cancellable - ); - - yield local.update_folder_status_async(remote_folder, false, true, cancellable); - } finally { - // added when call scheduled (above) - in_refresh_unseen.remove(folder); - } - } - public override Geary.ContactStore get_contact_store() { return local.contact_store; } - protected virtual Geary.SpecialFolderType[] get_supported_special_folders() { - return SUPPORTED_SPECIAL_FOLDERS; - } - public override async bool folder_exists_async(Geary.FolderPath path, Cancellable? cancellable = null) throws Error { check_open(); @@ -703,6 +616,48 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account { return minimal_folder; } + // Subclasses should implement this to return their flavor of a MinimalFolder with the + // appropriate interfaces attached. The returned folder should have its SpecialFolderType + // set using either the properties from the local folder or its path. + // + // This won't be called to build the Outbox or search folder, but for all others (including Inbox) it will. + protected abstract MinimalFolder new_folder(ImapDB.Folder local_folder); + + /** + * Hooks up and queues an {@link UpdateRemoteFolders} operation. + */ + private void update_remote_folders() { + UpdateRemoteFolders op = new UpdateRemoteFolders( + this, + this.remote, + this.local, + this.local_only.keys, + get_supported_special_folders() + ); + op.completed.connect(() => { + this.refresh_folder_timer.start(); + }); + try { + queue_operation(op); + } catch (Error err) { + // oh well + } + } + + /** + * Hooks up and queues an {@link RefreshFolderUnseen} operation. + */ + private void schedule_unseen_update(Geary.Folder folder) { + MinimalFolder? impl = folder as MinimalFolder; + if (impl != null) { + impl.refresh_unseen(); + } + } + + protected virtual Geary.SpecialFolderType[] get_supported_special_folders() { + return SUPPORTED_SPECIAL_FOLDERS; + } + private void compile_special_search_names() { /* * Compiles the list of names used to search for special @@ -1078,3 +1033,52 @@ internal class Geary.ImapEngine.UpdateRemoteFolders : AccountOperation { } } + +/** + * Account operation that updates a folder's unseen message count. + * + * This performs a IMAP STATUS on the folder, but only if it is not + * open - if it is open it is already maintaining its unseen count. + */ +internal class Geary.ImapEngine.RefreshFolderUnseen : AccountOperation { + + + private weak Geary.Folder folder; + private weak Imap.Account remote; + private weak ImapDB.Account local; + + + internal RefreshFolderUnseen(Geary.Folder folder, + Imap.Account remote, + ImapDB.Account local) { + this.folder = folder; + this.remote = remote; + this.local = local; + } + + public override bool equal_to(AccountOperation op) { + return ( + base.equal_to(op) && + this.folder.path.equal_to(((RefreshFolderUnseen) op).folder.path) + ); + } + + public override string to_string() { + return "%s(%s)".printf(base.to_string(), folder.path.to_string()); + } + + public override async void execute(Cancellable cancellable) throws Error { + if (this.folder.get_open_state() == Geary.Folder.OpenState.CLOSED) { + Imap.Folder remote_folder = yield remote.fetch_folder_cached_async( + folder.path, + true, + cancellable + ); + + yield local.update_folder_status_async( + remote_folder, false, true, cancellable + ); + } + } + +} diff --git a/src/engine/imap-engine/imap-engine-minimal-folder.vala b/src/engine/imap-engine/imap-engine-minimal-folder.vala index 5b902565..c1fbc40d 100644 --- a/src/engine/imap-engine/imap-engine-minimal-folder.vala +++ b/src/engine/imap-engine/imap-engine-minimal-folder.vala @@ -26,7 +26,10 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport.Copy, Geary.FolderSupport.Mark, Geary.FolderSupport.Move { + private const int FORCE_OPEN_REMOTE_TIMEOUT_SEC = 10; + private const int REFRESH_UNSEEN_TIMEOUT_SEC = 1; + public override Account account { get { return _account; } } @@ -69,6 +72,7 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport private int remote_count = -1; private Nonblocking.Mutex open_mutex = new Nonblocking.Mutex(); private Nonblocking.Mutex close_mutex = new Nonblocking.Mutex(); + private TimeoutManager refresh_unseen_timer; private Cancellable? open_cancellable = null; @@ -117,6 +121,10 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport this.replay_queue = new ReplayQueue(this); this.email_prefetcher = new EmailPrefetcher(this); + this.refresh_unseen_timer = new TimeoutManager.seconds( + REFRESH_UNSEEN_TIMEOUT_SEC, on_refresh_unseen + ); + // Notify now to ensure that wait_for_close_async does not // block if never opened. this.closed_semaphore.blind_notify(); @@ -573,15 +581,19 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport // first open gets to name the flags, but see note above this.open_flags = open_flags; - + // reset to force waiting in wait_for_open_async() - remote_semaphore.reset(); - + this.remote_semaphore.reset(); + // reset to force waiting in wait_for_close_async() - closed_semaphore.reset(); + this.closed_semaphore.reset(); + + // reset unseen count refresh since it will be updated when + // the remote opens + this.refresh_unseen_timer.reset(); this.open_cancellable = new Cancellable(); - + // Unless NO_DELAY is set, do NOT open the remote side here; wait for the ReplayQueue to // require a remote connection or wait_for_open_async() to be called ... this allows for // fast local-only operations to occur, local-only either because (a) the folder has all @@ -1576,6 +1588,31 @@ private class Geary.ImapEngine.MinimalFolder : Geary.Folder, Geary.FolderSupport remote_opened.to_string()); } + /** + * Schedules a refresh of the unseen count for the folder. + * + * This will only refresh folders that are not open, since if they + * are open or opening, they will already be updated. Hence it is safe to be called on closed folders. + */ + internal void refresh_unseen() { + if (this.open_count == 0) { + this.refresh_unseen_timer.start(); + } + } + + private void on_refresh_unseen() { + // We queue an account operation since the folder itself is + // closed and hence does not have a connection to use for it. + RefreshFolderUnseen op = new RefreshFolderUnseen( + this, this.remote, this.local + ); + try { + this._account.queue_operation(op); + } catch (Error err) { + // oh well + } + } + private void on_remote_ready() { start_open_remote(); }