From fda7fe18cc2615ebb7e7ab2840b2668c87120ae1 Mon Sep 17 00:00:00 2001 From: Juan Tejada Date: Tue, 6 Dec 2016 15:34:28 -0800 Subject: [PATCH] [local-sync] Ensure order correct order when running syncback requests When running syncback requests, if a task is meant to change the UID of a message (e.g. move it to a new folder), that task should be run after other tasks that don't affect the UID. Otherwise, when trying to run the other tasks, they would reference a UID that is no longer valid. This commit will make sure that we run any tasks that will change message uids last, /and/ make sure that we don't run more than 1 task that will affect the uids of the same messages in a row (i.e. without running a sync loop in between) --- .../src/local-sync-worker/sync-worker.js | 60 +++++++++++++++++-- ...k-factory.es6 => syncback-task-factory.js} | 8 --- .../syncback_tasks/create-category.imap.js | 4 ++ .../syncback_tasks/delete-folder.imap.js | 4 ++ .../syncback_tasks/delete-label.imap.js | 4 ++ .../syncback_tasks/delete-message.imap.js | 4 ++ .../mark-message-as-read.imap.js | 4 ++ .../mark-message-as-unread.imap.js | 4 ++ .../mark-thread-as-read.imap.js | 4 ++ .../mark-thread-as-unread.imap.js | 4 ++ .../move-message-to-folder.imap.js | 4 ++ .../move-thread-to-folder.imap.js | 4 ++ .../syncback_tasks/rename-folder.imap.js | 4 ++ .../syncback_tasks/rename-label.imap.js | 4 ++ .../syncback_tasks/save-sent-message.imap.js | 4 ++ .../syncback_tasks/set-message-labels.imap.js | 4 ++ .../set-thread-folder-and-labels.imap.js | 4 ++ .../syncback_tasks/set-thread-labels.imap.js | 4 ++ .../syncback_tasks/star-message.imap.js | 4 ++ .../syncback_tasks/star-thread.imap.js | 4 ++ .../syncback_tasks/syncback-task.js | 4 ++ .../syncback_tasks/unstar-message.imap.js | 4 ++ .../syncback_tasks/unstar-thread.imap.js | 4 ++ 23 files changed, 138 insertions(+), 14 deletions(-) rename packages/local-sync/src/local-sync-worker/{syncback-task-factory.es6 => syncback-task-factory.js} (93%) diff --git a/packages/local-sync/src/local-sync-worker/sync-worker.js b/packages/local-sync/src/local-sync-worker/sync-worker.js index 82f94886a..c70fce3ad 100644 --- a/packages/local-sync/src/local-sync-worker/sync-worker.js +++ b/packages/local-sync/src/local-sync-worker/sync-worker.js @@ -115,14 +115,62 @@ class SyncWorker { return await this._conn.connect(); } - async syncbackMessageActions() { - const {SyncbackRequest} = this._db; - const where = {where: {status: "NEW"}, limit: 100}; + async runNewSyncbackRequests() { + const {SyncbackRequest, Message} = this._db; + const where = { + limit: 100, + where: {status: "NEW"}, + order: [['createdAt', 'ASC']], + }; + // Make sure we run the tasks that affect IMAP uids last, and that we don't + // run 2 tasks that will affect the same set of UIDS together (i.e. without + // running a sync loop in between) const tasks = await SyncbackRequest.findAll(where) - .map((req) => SyncbackTaskFactory.create(this._account, req)); + .map((req) => SyncbackTaskFactory.create(this._account, req)) - return PromiseUtils.each(tasks, this.runSyncbackTask.bind(this)); + if (tasks.length === 0) { return Promise.resolve() } + + const tasksToProcess = tasks.filter(t => !t.affectsImapMessageUIDs()) + const tasksAffectingUIDs = tasks.filter(t => t.affectsImapMessageUIDs()) + + const changeFolderTasks = tasksAffectingUIDs.filter(t => + t.description() === 'RenameFolder' || t.description() === 'DeleteFolder' + ) + if (changeFolderTasks.length > 0) { + // If we are renaming or deleting folders, those are the only tasks we + // want to process before executing any other tasks that may change UIDs + const affectedFolderIds = new Set() + changeFolderTasks.forEach((task) => { + const {props: {folderId}} = task.syncbackRequestObject() + if (folderId && !affectedFolderIds.has(folderId)) { + tasksToProcess.push(task) + affectedFolderIds.add(folderId) + } + }) + return PromiseUtils.each(tasks, (task) => this.runSyncbackTask(task)); + } + + // Otherwise, make sure that we don't process more than 1 task that will affect + // the UID of the same message + const affectedMessageIds = new Set() + for (const task of tasksAffectingUIDs) { + const {props: {messageId, threadId}} = task.syncbackRequestObject() + if (messageId) { + if (!affectedMessageIds.has(messageId)) { + tasksToProcess.push(task) + affectedMessageIds.add(messageId) + } + } else if (threadId) { + const messageIds = await Message.findAll({where: {threadId}}).map(m => m.id) + const shouldIncludeTask = messageIds.every(id => !affectedMessageIds.has(id)) + if (shouldIncludeTask) { + tasksToProcess.push(task) + messageIds.forEach(id => affectedMessageIds.add(id)) + } + } + } + return PromiseUtils.each(tasks, (task) => this.runSyncbackTask(task)); } async runSyncbackTask(task) { @@ -176,7 +224,7 @@ class SyncWorker { try { await this._account.update({syncError: null}); await this.ensureConnection(); - await this.syncbackMessageActions(); + await this.runNewSyncbackRequests(); await this._conn.runOperation(new FetchFolderList(this._account.provider, this._logger)); await this.syncMessagesInAllFolders(); await this.onSyncDidComplete(); diff --git a/packages/local-sync/src/local-sync-worker/syncback-task-factory.es6 b/packages/local-sync/src/local-sync-worker/syncback-task-factory.js similarity index 93% rename from packages/local-sync/src/local-sync-worker/syncback-task-factory.es6 rename to packages/local-sync/src/local-sync-worker/syncback-task-factory.js index f86ca3a4c..8a24a630b 100644 --- a/packages/local-sync/src/local-sync-worker/syncback-task-factory.es6 +++ b/packages/local-sync/src/local-sync-worker/syncback-task-factory.js @@ -5,14 +5,6 @@ */ class SyncbackTaskFactory { - static TaskTypesAffectingMessageFolderUIDs = [ - 'MoveThreadToFolder', - 'MoveMessageToFolder', - 'SetThreadFolderAndLabels', - 'RenameFolder', - 'DeleteFolder', - ] - static create(account, syncbackRequest) { let Task = null; switch (syncbackRequest.type) { diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js index 3c1e2b76f..869f2a3a3 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/create-category.imap.js @@ -5,6 +5,10 @@ class CreateCategoryIMAP extends SyncbackTask { return `CreateCategory`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const syncbackRequestObject = this.syncbackRequestObject() const displayName = syncbackRequestObject.props.displayName diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js index 4629b0d3b..2771aff61 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-folder.imap.js @@ -5,6 +5,10 @@ class DeleteFolderIMAP extends SyncbackTask { return `DeleteFolder`; } + affectsImapMessageUIDs() { + return true + } + async run(db, imap) { const folderId = this.syncbackRequestObject().props.folderId const folder = await db.Folder.findById(folderId) diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js index 498cc7c09..a269e0f11 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-label.imap.js @@ -5,6 +5,10 @@ class DeleteLabelIMAP extends SyncbackTask { return `DeleteLabel`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const labelId = this.syncbackRequestObject().props.labelId const label = await db.Label.findById(labelId) diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-message.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-message.imap.js index a204e5b85..516f84080 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-message.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/delete-message.imap.js @@ -6,6 +6,10 @@ class DeleteMessageIMAP extends SyncbackTask { return `DeleteMessage`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId const {box, message} = await TaskHelpers.openMessageBox({messageId, db, imap}) diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-read.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-read.imap.js index 26dfb6faf..f1c8c8995 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-read.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-read.imap.js @@ -6,6 +6,10 @@ class MarkMessageAsReadIMAP extends SyncbackTask { return `MarkMessageAsRead`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-unread.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-unread.imap.js index f7f7a3484..ec4688914 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-unread.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-message-as-unread.imap.js @@ -6,6 +6,10 @@ class MarkMessageAsUnreadIMAP extends SyncbackTask { return `MarkMessageAsUnread`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-read.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-read.imap.js index c74ac249c..b90b107cf 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-read.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-read.imap.js @@ -6,6 +6,10 @@ class MarkThreadAsRead extends SyncbackTask { return `MarkThreadAsRead`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-unread.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-unread.imap.js index 72ae4068a..e0f214003 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-unread.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/mark-thread-as-unread.imap.js @@ -6,6 +6,10 @@ class MarkThreadAsUnread extends SyncbackTask { return `MarkThreadAsUnread`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/move-message-to-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/move-message-to-folder.imap.js index 5beecb32f..53376edfd 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/move-message-to-folder.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/move-message-to-folder.imap.js @@ -6,6 +6,10 @@ class MoveMessageToFolderIMAP extends SyncbackTask { return `MoveMessageToFolder`; } + affectsImapMessageUIDs() { + return true + } + async run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId const targetFolderId = this.syncbackRequestObject().props.folderId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/move-thread-to-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/move-thread-to-folder.imap.js index 1ab698215..345bdb72e 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/move-thread-to-folder.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/move-thread-to-folder.imap.js @@ -6,6 +6,10 @@ class MoveThreadToFolderIMAP extends SyncbackTask { return `MoveThreadToFolder`; } + affectsImapMessageUIDs() { + return true + } + async run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId const targetFolderId = this.syncbackRequestObject().props.folderId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js index 70728afbe..aea8f53df 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-folder.imap.js @@ -5,6 +5,10 @@ class RenameFolderIMAP extends SyncbackTask { return `RenameFolder`; } + affectsImapMessageUIDs() { + return true + } + async run(db, imap) { const folderId = this.syncbackRequestObject().props.folderId const newFolderName = this.syncbackRequestObject().props.displayName diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js index b0779d8ab..c5ca7448d 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/rename-label.imap.js @@ -5,6 +5,10 @@ class RenameLabelIMAP extends SyncbackTask { return `RenameLabel`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const labelId = this.syncbackRequestObject().props.labelId const newLabelName = this.syncbackRequestObject().props.displayName diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/save-sent-message.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/save-sent-message.imap.js index ec3871e59..4f5a5e22c 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/save-sent-message.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/save-sent-message.imap.js @@ -5,6 +5,10 @@ class SaveSentMessageIMAP extends SyncbackTask { return `SaveSentMessage`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const {rawMime, messageId} = this.syncbackRequestObject().props; diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/set-message-labels.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/set-message-labels.imap.js index 19aa1bf09..0b749dc1e 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/set-message-labels.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/set-message-labels.imap.js @@ -6,6 +6,10 @@ class SetMessageLabelsIMAP extends SyncbackTask { return `SetMessageLabels`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId const labelIds = this.syncbackRequestObject().props.labelIds diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-folder-and-labels.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-folder-and-labels.imap.js index 15f0adc0d..51dad5684 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-folder-and-labels.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-folder-and-labels.imap.js @@ -6,6 +6,10 @@ class SetThreadFolderAndLabelsIMAP extends SyncbackTask { return `SetThreadFolderAndLabels`; } + affectsImapMessageUIDs() { + return true + } + async run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId const labelIds = this.syncbackRequestObject().props.labelIds diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-labels.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-labels.imap.js index 4bc6c1351..b1de79855 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-labels.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/set-thread-labels.imap.js @@ -6,6 +6,10 @@ class SetThreadLabelsIMAP extends SyncbackTask { return `SetThreadLabels`; } + affectsImapMessageUIDs() { + return false + } + async run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId const labelIds = this.syncbackRequestObject().props.labelIds diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/star-message.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/star-message.imap.js index 8ae5a4f59..5ff066546 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/star-message.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/star-message.imap.js @@ -6,6 +6,10 @@ class StarMessageIMAP extends SyncbackTask { return `StarMessage`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/star-thread.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/star-thread.imap.js index 71083f96f..d6fd83072 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/star-thread.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/star-thread.imap.js @@ -6,6 +6,10 @@ class StarThread extends SyncbackTask { return `StarThread`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/syncback-task.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/syncback-task.js index 6379bc11b..c4d20bf74 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/syncback-task.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/syncback-task.js @@ -12,6 +12,10 @@ class SyncbackTask { throw new Error("Must return a description") } + affectsImapMessageUIDs() { + throw new Error("Must implement `affectsImapMessageUIDs`") + } + run() { throw new Error("Must implement a run method") } diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-message.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-message.imap.js index c4f9cbd85..944035dfd 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-message.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-message.imap.js @@ -6,6 +6,10 @@ class UnstarMessageIMAP extends SyncbackTask { return `UnstarMessage`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const messageId = this.syncbackRequestObject().props.messageId diff --git a/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-thread.imap.js b/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-thread.imap.js index c1652318c..ce91472ae 100644 --- a/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-thread.imap.js +++ b/packages/local-sync/src/local-sync-worker/syncback_tasks/unstar-thread.imap.js @@ -6,6 +6,10 @@ class UnstarThread extends SyncbackTask { return `UnstarThread`; } + affectsImapMessageUIDs() { + return false + } + run(db, imap) { const threadId = this.syncbackRequestObject().props.threadId