diff --git a/internal_packages/composer/lib/main.cjsx b/internal_packages/composer/lib/main.cjsx index 3619a49bd..c9513ec2e 100644 --- a/internal_packages/composer/lib/main.cjsx +++ b/internal_packages/composer/lib/main.cjsx @@ -35,12 +35,17 @@ class ComposerWithWindowProps extends React.Component _showInitialErrorDialog: (msg) -> remote = require('remote') dialog = remote.require('dialog') - dialog.showMessageBox remote.getCurrentWindow(), { - type: 'warning' - buttons: ['Okay'], - message: "Error" - detail: msg - } + # We delay so the view has time to update the restored draft. If we + # don't delay the modal may come up in a state where the draft looks + # like it hasn't been restored or has been lost. + _.delay -> + dialog.showMessageBox remote.getCurrentWindow(), { + type: 'warning' + buttons: ['Okay'], + message: "Error" + detail: msg + } + , 100 module.exports = activate: (@state={}) -> diff --git a/internal_packages/worker-ui/lib/developer-bar-task.cjsx b/internal_packages/worker-ui/lib/developer-bar-task.cjsx index c36c7b505..b19facabb 100644 --- a/internal_packages/worker-ui/lib/developer-bar-task.cjsx +++ b/internal_packages/worker-ui/lib/developer-bar-task.cjsx @@ -42,7 +42,14 @@ class DeveloperBarTask extends React.Component errCode = remoteError.statusCode ? "" errMessage = remoteError.body?.message ? remoteError?.message ? JSON.stringify(remoteError) - return "#{@props.task.constructor.name} #{errType} #{errCode} #{errMessage}" + id = @props.task.id[-4..-1] + + if qs.status + status = "#{qs.status} (#{qs.debugStatus})" + else + status = "#{qs.debugStatus}" + + return "#{@props.task.constructor.name} (ID: #{id}) #{status} #{errType} #{errCode} #{errMessage}" _classNames: => qs = @props.task.queueState ? {} diff --git a/internal_packages/worker-ui/lib/developer-bar.cjsx b/internal_packages/worker-ui/lib/developer-bar.cjsx index 6e7a29aea..aad65867b 100644 --- a/internal_packages/worker-ui/lib/developer-bar.cjsx +++ b/internal_packages/worker-ui/lib/developer-bar.cjsx @@ -84,8 +84,14 @@ class DeveloperBar extends React.Component queue = @state.queue.filter(matchingFilter) queueDivs = for i in [@state.queue.length - 1..0] by -1 task = @state.queue[i] + # We need to pass the task separately because we want to update + # when just that variable changes. Otherwise, since the `task` + # pointer doesn't change, the `DeveloperBarTask` doesn't know to + # update. + status = @state.queue[i].queueState.status queueCompleted = @state.completed.filter(matchingFilter) diff --git a/internal_packages/worker-ui/stylesheets/worker-ui.less b/internal_packages/worker-ui/stylesheets/worker-ui.less index b64234aa3..d204faafd 100755 --- a/internal_packages/worker-ui/stylesheets/worker-ui.less +++ b/internal_packages/worker-ui/stylesheets/worker-ui.less @@ -103,7 +103,7 @@ &.queue { padding: 0; - .btn { float:right; } + .btn { float:right; z-index: 10; } hr { margin: 1em 0; } diff --git a/spec/stores/draft-store-spec.coffee b/spec/stores/draft-store-spec.coffee index 82a3c515a..8a9c54b9d 100644 --- a/spec/stores/draft-store-spec.coffee +++ b/spec/stores/draft-store-spec.coffee @@ -685,12 +685,15 @@ describe "DraftStore", -> beforeEach -> DraftStore._draftSessions = {} DraftStore._draftsSending = {} + @forceCommit = false proxy = prepare: -> Promise.resolve(proxy) teardown: -> draft: -> {} changes: - commit: -> Promise.resolve() + commit: ({force}={}) => + @forceCommit = force + Promise.resolve() DraftStore._draftSessions[draftClientId] = proxy spyOn(DraftStore, "_doneWithSession").andCallThrough() spyOn(DraftStore, "trigger") @@ -744,6 +747,15 @@ describe "DraftStore", -> runs -> expect(atom.close).not.toHaveBeenCalled() + it "forces a commit to happen before sending", -> + spyOn(Actions, "queueTask") + runs -> + DraftStore._onSendDraft(draftClientId) + waitsFor -> + DraftStore._doneWithSession.calls.length > 0 + runs -> + expect(@forceCommit).toBe true + it "queues a SendDraftTask", -> spyOn(Actions, "queueTask") runs -> @@ -785,7 +797,7 @@ describe "DraftStore", -> spyOn(dialog, "showMessageBox") DraftStore._draftsSending[draftClientId] = true Actions.draftSendingFailed({errorMessage: "boohoo", draftClientId}) - advanceClock(10) + advanceClock(200) expect(DraftStore.isSendingDraft(draftClientId)).toBe false expect(DraftStore.trigger).toHaveBeenCalledWith(draftClientId) expect(dialog.showMessageBox).toHaveBeenCalled() diff --git a/spec/stores/task-queue-spec.coffee b/spec/stores/task-queue-spec.coffee index 3df3a4504..73c7fc74d 100644 --- a/spec/stores/task-queue-spec.coffee +++ b/spec/stores/task-queue-spec.coffee @@ -127,6 +127,8 @@ describe "TaskQueue", -> TaskQueue._queue = [obsoleteTask, otherTask] TaskQueue._dequeueObsoleteTasks(replacementTask) expect(TaskQueue._queue.length).toBe(1) + expect(obsoleteTask.queueState.status).toBe Task.Status.Continue + expect(obsoleteTask.queueState.debugStatus).toBe Task.DebugStatus.DequeuedObsolete expect(TaskQueue.dequeue).toHaveBeenCalledWith(obsoleteTask) expect(TaskQueue.dequeue.calls.length).toBe(1) @@ -175,7 +177,7 @@ describe "TaskQueue", -> it "doesn't process blocked tasks", -> class BlockedByTaskA extends Task - shouldWaitForTask: (other) -> other instanceof TaskSubclassA + isDependentTask: (other) -> other instanceof TaskSubclassA taskA = new TaskSubclassA() otherTask = new Task() @@ -197,9 +199,9 @@ describe "TaskQueue", -> expect(taskA.runRemote).toHaveBeenCalled() expect(blockedByTaskA.runRemote).not.toHaveBeenCalled() - it "doesn't block itself, even if the shouldWaitForTask method is implemented naively", -> + it "doesn't block itself, even if the isDependentTask method is implemented naively", -> class BlockingTask extends Task - shouldWaitForTask: (other) -> other instanceof BlockingTask + isDependentTask: (other) -> other instanceof BlockingTask blockedTask = new BlockingTask() spyOn(blockedTask, "runRemote").andCallFake -> Promise.resolve() @@ -215,3 +217,80 @@ describe "TaskQueue", -> TaskQueue._queue = [task] TaskQueue._processTask(task) expect(task.queueState.isProcessing).toBe true + + describe "handling task runRemote task errors", -> + spyAACallback = jasmine.createSpy("onDependentTaskError") + spyBBRemote = jasmine.createSpy("performRemote") + spyBBCallback = jasmine.createSpy("onDependentTaskError") + spyCCRemote = jasmine.createSpy("performRemote") + spyCCCallback = jasmine.createSpy("onDependentTaskError") + + beforeEach -> + testError = new Error("Test Error") + @testError = testError + class TaskAA extends Task + onDependentTaskError: spyAACallback + performRemote: -> + # We reject instead of `throw` because jasmine thinks this + # `throw` is in the context of the test instead of the context + # of the calling promise in task-queue.coffee + return Promise.reject(testError) + + class TaskBB extends Task + isDependentTask: (other) -> other instanceof TaskAA + onDependentTaskError: spyBBCallback + performRemote: spyBBRemote + + class TaskCC extends Task + isDependentTask: (other) -> other instanceof TaskBB + onDependentTaskError: (task, err) -> + spyCCCallback(task, err) + return Task.DO_NOT_DEQUEUE_ME + performRemote: spyCCRemote + + @taskAA = new TaskAA + @taskAA.queueState.localComplete = true + @taskBB = new TaskBB + @taskBB.queueState.localComplete = true + @taskCC = new TaskCC + @taskCC.queueState.localComplete = true + + spyOn(TaskQueue, 'trigger') + + # Don't keep processing the queue + spyOn(TaskQueue, '_updateSoon') + + it "catches the error and dequeues the task", -> + spyOn(TaskQueue, 'dequeue') + waitsForPromise => + TaskQueue._processTask(@taskAA).then => + expect(TaskQueue.dequeue).toHaveBeenCalledWith(@taskAA) + expect(spyAACallback).not.toHaveBeenCalled() + expect(@taskAA.queueState.remoteError.message).toBe "Test Error" + + it "calls `onDependentTaskError` on dependent tasks", -> + spyOn(TaskQueue, 'dequeue').andCallThrough() + TaskQueue._queue = [@taskAA, @taskBB, @taskCC] + waitsForPromise => + TaskQueue._processTask(@taskAA).then => + expect(TaskQueue.dequeue.calls.length).toBe 2 + # NOTE: The recursion goes depth-first. The leafs are called + # first + expect(TaskQueue.dequeue.calls[0].args[0]).toBe @taskBB + expect(TaskQueue.dequeue.calls[1].args[0]).toBe @taskAA + expect(spyAACallback).not.toHaveBeenCalled() + expect(spyBBCallback).toHaveBeenCalledWith(@taskAA, @testError) + expect(@taskAA.queueState.remoteError.message).toBe "Test Error" + expect(@taskBB.queueState.status).toBe Task.Status.Continue + expect(@taskBB.queueState.debugStatus).toBe Task.DebugStatus.DequeuedDependency + + it "dequeues all dependent tasks except those that return `Task.DO_NOT_DEQUEUE_ME` from their callbacks", -> + spyOn(TaskQueue, 'dequeue').andCallThrough() + TaskQueue._queue = [@taskAA, @taskBB, @taskCC] + waitsForPromise => + TaskQueue._processTask(@taskAA).then => + expect(TaskQueue._queue).toEqual [@taskCC] + expect(spyCCCallback).toHaveBeenCalledWith(@taskBB, @testError) + expect(@taskCC.queueState.status).toBe null + expect(@taskCC.queueState.debugStatus).toBe Task.DebugStatus.JustConstructed + diff --git a/spec/tasks/change-mail-task-spec.coffee b/spec/tasks/change-mail-task-spec.coffee index d99d10397..980a37010 100644 --- a/spec/tasks/change-mail-task-spec.coffee +++ b/spec/tasks/change-mail-task-spec.coffee @@ -205,15 +205,15 @@ describe "ChangeMailTask", -> @task.messages = [] waitsForPromise => @task.performRemote().then (code) => - expect(code).toEqual(Task.Status.Finished) + expect(code).toEqual(Task.Status.Success) describe "if performRequests resolves", -> - it "should resolve with Task.Status.Finished", -> + it "should resolve with Task.Status.Success", -> @task = new ChangeMailTask() spyOn(@task, 'performRequests').andReturn(Promise.resolve()) waitsForPromise => @task.performRemote().then (result) => - expect(result).toBe(Task.Status.Finished) + expect(result).toBe(Task.Status.Success) describe "if performRequests rejects with a permanent network error", -> beforeEach -> @@ -227,10 +227,10 @@ describe "ChangeMailTask", -> expect(@task.performLocal).toHaveBeenCalled() expect(@task._isReverting).toBe(true) - it "should resolve with finished after reverting", -> + it "should resolve with Task.Status.Failed after reverting", -> waitsForPromise => @task.performRemote().then (result) => - expect(result).toBe(Task.Status.Finished) + expect(result).toBe(Task.Status.Failed) describe "if performRequests rejects with a temporary network error", -> beforeEach -> @@ -450,7 +450,7 @@ describe "ChangeMailTask", -> @task.performLocal().then => expect(@task._lockAll).toHaveBeenCalled() - describe "when performRemote is returning Task.Status.Finished", -> + describe "when performRemote is returning Task.Status.Success", -> it "should clean up locks", -> spyOn(@task, 'performRequests').andReturn(Promise.resolve()) spyOn(@task, '_ensureLocksRemoved') @@ -458,7 +458,7 @@ describe "ChangeMailTask", -> @task.performRemote().then => expect(@task._ensureLocksRemoved).toHaveBeenCalled() - describe "when performRemote is returning Task.Status.Finished after reverting", -> + describe "when performRemote is returning Task.Status.Failed after reverting", -> it "should clean up locks", -> spyOn(@task, 'performRequests').andReturn(Promise.reject(new APIError(statusCode: 400))) spyOn(@task, '_ensureLocksRemoved') @@ -544,7 +544,7 @@ describe "ChangeMailTask", -> task._restoreValues = null expect( -> task.createUndoTask()).toThrow() - describe "shouldWaitForTask", -> + describe "isDependentTask", -> it "should return true if another, older ChangeMailTask involves the same threads", -> a = new ChangeMailTask() a.threads = ['t1', 't2', 't3'] @@ -555,8 +555,8 @@ describe "ChangeMailTask", -> c = new ChangeMailTask() c.threads = ['t0', 't7'] c.creationDate = new Date(3000) - expect(a.shouldWaitForTask(b)).toEqual(false) - expect(a.shouldWaitForTask(c)).toEqual(false) - expect(b.shouldWaitForTask(a)).toEqual(true) - expect(c.shouldWaitForTask(a)).toEqual(false) - expect(c.shouldWaitForTask(b)).toEqual(true) + expect(a.isDependentTask(b)).toEqual(false) + expect(a.isDependentTask(c)).toEqual(false) + expect(b.isDependentTask(a)).toEqual(true) + expect(c.isDependentTask(a)).toEqual(false) + expect(c.isDependentTask(b)).toEqual(true) diff --git a/spec/tasks/file-upload-task-spec.coffee b/spec/tasks/file-upload-task-spec.coffee index d36d71a40..209e176d5 100644 --- a/spec/tasks/file-upload-task-spec.coffee +++ b/spec/tasks/file-upload-task-spec.coffee @@ -115,9 +115,7 @@ describe "FileUploadTask", -> @taskExitStatus = null @runWithError = (simulatedError) => runs -> - @task.performRemote().catch (err) -> - console.log(err) - .then (status) => + @task.performRemote().then (status) => @taskExitStatus = status waitsFor -> @@ -132,7 +130,8 @@ describe "FileUploadTask", -> describe "if the error is permanent", -> beforeEach -> - @runWithError(new APIError(statusCode: 400)) + @apiError = new APIError(statusCode: 400) + @runWithError(@apiError) it "should broadcast `failed` if the error is permanent", -> runs -> @@ -140,9 +139,9 @@ describe "FileUploadTask", -> dataReceived = Actions.uploadStateChanged.calls[0].args[0] expect(_.isMatch(dataReceived, data)).toBe(true) - it "should resolve with `finished`", -> - runs -> - expect(@taskExitStatus).toBe(Task.Status.Finished) + it "should report Failed with the APIError", -> + runs => + expect(@taskExitStatus).toEqual([Task.Status.Failed, @apiError]) describe "if the error is temporary", -> beforeEach -> @@ -162,6 +161,10 @@ describe "FileUploadTask", -> dataReceived = Actions.uploadStateChanged.calls[0].args[0] expect(_.isMatch(dataReceived, data)).toBe(true) + it "should resolve with Task.Status.Failed", -> + runs -> + expect(@taskExitStatus).toBe(Task.Status.Failed) + describe "when the remote API request succeeds", -> beforeEach -> @simulateRequestSuccessImmediately = true diff --git a/spec/tasks/send-draft-spec.coffee b/spec/tasks/send-draft-spec.coffee index 6909c6f9b..ba4b5d2a4 100644 --- a/spec/tasks/send-draft-spec.coffee +++ b/spec/tasks/send-draft-spec.coffee @@ -1,6 +1,7 @@ NylasAPI = require '../../src/flux/nylas-api' Actions = require '../../src/flux/actions' SyncbackDraftTask = require '../../src/flux/tasks/syncback-draft' +FileUploadTask = require '../../src/flux/tasks/file-upload-task' SendDraftTask = require '../../src/flux/tasks/send-draft' DatabaseStore = require '../../src/flux/stores/database-store' {APIError} = require '../../src/flux/errors' @@ -10,7 +11,7 @@ SoundRegistry = require '../../src/sound-registry' _ = require 'underscore' describe "SendDraftTask", -> - describe "shouldWaitForTask", -> + describe "isDependentTask", -> it "should return true if there are SyncbackDraftTasks for the same draft", -> @draftA = new Message version: '1' @@ -36,7 +37,7 @@ describe "SendDraftTask", -> @saveB = new SyncbackDraftTask('localid-B') @sendA = new SendDraftTask('localid-A') - expect(@sendA.shouldWaitForTask(@saveA)).toBe(true) + expect(@sendA.isDependentTask(@saveA)).toBe(true) describe "performLocal", -> it "should throw an exception if the first parameter is not a clientId", -> @@ -275,3 +276,21 @@ describe "SendDraftTask", -> waitsForPromise => @task.performRemote().catch (error) -> expect(error).toBe "DB error" + + describe "failing dependent task", -> + it "notifies the user that the required draft save failed", -> + task = new SendDraftTask("local-1234") + syncback = new SyncbackDraftTask('local-1234') + spyOn(task, "_notifyUserOfError") + task.onDependentTaskError(syncback, new Error("Oh no")) + expect(task._notifyUserOfError).toHaveBeenCalled() + expect(task._notifyUserOfError.calls.length).toBe 1 + + it "notifies the user that the required file upload failed", -> + task = new SendDraftTask("local-1234") + fileUploadTask = new FileUploadTask('/dev/null', 'local-1234') + spyOn(task, "_notifyUserOfError") + task.onDependentTaskError(fileUploadTask, new Error("Oh no")) + expect(task._notifyUserOfError).toHaveBeenCalled() + expect(task._notifyUserOfError.calls.length).toBe 1 + diff --git a/spec/tasks/syncback-draft-spec.coffee b/spec/tasks/syncback-draft-spec.coffee index a400bd777..802e6fecc 100644 --- a/spec/tasks/syncback-draft-spec.coffee +++ b/spec/tasks/syncback-draft-spec.coffee @@ -48,6 +48,84 @@ describe "SyncbackDraftTask", -> fn() return Promise.resolve() + describe "queueing multiple tasks", -> + beforeEach -> + @taskA = new SyncbackDraftTask("draft-123") + @taskB = new SyncbackDraftTask("draft-123") + @taskC = new SyncbackDraftTask("draft-123") + @taskOther = new SyncbackDraftTask("draft-456") + + now = Date.now() + @taskA.creationDate = now - 20 + @taskB.creationDate = now - 10 + @taskC.creationDate = now + TaskQueue._queue = [] + + it "dequeues other SyncbackDraftTasks that haven't started yet", -> + # Task A is taking forever, B is waiting on it, and C gets queued. + [@taskA, @taskB, @taskOther].forEach (t) -> + t.queueState.localComplete = true + + # taskA has already started This should NOT get dequeued + @taskA.queueState.isProcessing = true + + # taskB hasn't started yet! This should get dequeued + @taskB.queueState.isProcessing = false + + # taskOther, while unstarted, doesn't match the draftId and should + # not get dequeued + @taskOther.queueState.isProcessing = false + + TaskQueue._queue = [@taskA, @taskB, @taskOther] + spyOn(@taskC, "runLocal").andReturn Promise.resolve() + + TaskQueue.enqueue(@taskC) + + # Note that taskB is gone, taskOther was untouched, and taskC was + # added. + expect(TaskQueue._queue).toEqual = [@taskA, @taskOther, @taskC] + + expect(@taskC.runLocal).toHaveBeenCalled() + + it "waits for any other inflight tasks to finish or error", -> + @taskA.queueState.localComplete = true + @taskA.queueState.isProcessing = true + @taskB.queueState.localComplete = true + spyOn(@taskB, "runRemote").andReturn Promise.resolve() + + TaskQueue._queue = [@taskA, @taskB] + + # Since taskA has isProcessing set to true, it will just be passed + # over. We expect taskB to fail the `_taskIsBlocked` test + TaskQueue._processQueue() + advanceClock(100) + expect(TaskQueue._queue).toEqual [@taskA, @taskB] + expect(@taskA.queueState.isProcessing).toBe true + expect(@taskB.queueState.isProcessing).toBe false + expect(@taskB.runRemote).not.toHaveBeenCalled() + + it "does not get dequeued if dependent tasks fail", -> + @taskA.queueState.localComplete = true + @taskB.queueState.localComplete = true + + spyOn(@taskA, "performRemote").andReturn Promise.resolve(Task.Status.Failed) + spyOn(@taskB, "performRemote").andReturn Promise.resolve(Task.Status.Success) + + spyOn(TaskQueue, "dequeue").andCallThrough() + spyOn(TaskQueue, "trigger") + + TaskQueue._queue = [@taskA, @taskB] + TaskQueue._processQueue() + advanceClock(100) + TaskQueue._processQueue() + advanceClock(100) + expect(@taskA.performRemote).toHaveBeenCalled() + expect(@taskB.performRemote).toHaveBeenCalled() + expect(TaskQueue.dequeue.calls.length).toBe 2 + + expect(@taskA.queueState.debugStatus).not.toBe Task.DebugStatus.DequeuedDependency + expect(@taskA.queueState.debugStatus).not.toBe Task.DebugStatus.DequeuedDependency + describe "performRemote", -> beforeEach -> spyOn(NylasAPI, 'makeRequest').andCallFake (opts) -> @@ -124,14 +202,15 @@ describe "SyncbackDraftTask", -> expect(status).toBe Task.Status.Retry [500, 0].forEach (code) -> - it "Aborts on #{code} errors when we're PUT-ing", -> + it "Fails on #{code} errors when we're PUT-ing", -> stubAPI(code, "PUT") waitsForPromise => - @task.performRemote().then (status) => + @task.performRemote().then ([status, err]) => + expect(status).toBe Task.Status.Failed expect(@task.getLatestLocalDraft).toHaveBeenCalled() expect(@task.getLatestLocalDraft.calls.length).toBe 1 expect(@task.detatchFromRemoteID).not.toHaveBeenCalled() - expect(status).toBe Task.Status.Finished + expect(err.statusCode).toBe code describe 'when POST-ing', -> beforeEach -> @@ -140,20 +219,21 @@ describe "SyncbackDraftTask", -> spyOn(@task, "detatchFromRemoteID").andCallFake -> Promise.resolve(localDraft()) [400, 404, 409, 500, 0].forEach (code) -> - it "Aborts on #{code} errors when we're POST-ing", -> + it "Fails on #{code} errors when we're POST-ing", -> stubAPI(code, "POST") waitsForPromise => - @task.performRemote().then (status) => + @task.performRemote().then ([status, err]) => + expect(status).toBe Task.Status.Failed expect(@task.getLatestLocalDraft).toHaveBeenCalled() expect(@task.getLatestLocalDraft.calls.length).toBe 1 expect(@task.detatchFromRemoteID).not.toHaveBeenCalled() - expect(status).toBe Task.Status.Finished + expect(err.statusCode).toBe code - it "Aborts on unknown errors", -> + it "Fails on unknown errors", -> spyOn(NylasAPI, "makeRequest").andCallFake -> Promise.reject(new APIError()) waitsForPromise => - @task.performRemote().then (status) => + @task.performRemote().then ([status, err]) => + expect(status).toBe Task.Status.Failed expect(@task.getLatestLocalDraft).toHaveBeenCalled() expect(@task.getLatestLocalDraft.calls.length).toBe 1 expect(@task.detatchFromRemoteID).not.toHaveBeenCalled() - expect(status).toBe Task.Status.Finished diff --git a/spec/tasks/task-spec.coffee b/spec/tasks/task-spec.coffee index 56bce54b6..e7105e53e 100644 --- a/spec/tasks/task-spec.coffee +++ b/spec/tasks/task-spec.coffee @@ -12,13 +12,13 @@ describe "Task", -> describe "initial state", -> it "should set up queue state with additional information about local/remote", -> task = new Task() - expect(task.queueState).toEqual({ isProcessing : false, localError : null, localComplete : false, remoteError : null, remoteAttempts : 0, remoteComplete : false }) + expect(task.queueState).toEqual({ isProcessing : false, localError : null, localComplete : false, remoteError : null, remoteAttempts : 0, remoteComplete : false, status: null, debugStatus: Task.DebugStatus.JustConstructed}) describe "runLocal", -> beforeEach -> class APITestTask extends Task performLocal: -> Promise.resolve() - performRemote: -> Promise.resolve(Task.Status.Finished) + performRemote: -> Promise.resolve(Task.Status.Success) @task = new APITestTask() describe "when performLocal is not complete", -> @@ -73,10 +73,37 @@ describe "Task", -> advanceClock() expect(@task.performRemote).toHaveBeenCalled() + it "it should resolve Continue if it already ran", -> + @task.queueState.remoteComplete = true + waitsForPromise => + @task.runRemote().then (status) => + expect(@task.queueState.status).toBe Task.Status.Continue + expect(status).toBe Task.Status.Continue + + it "marks as complete if the task 'continue's", -> + spyOn(@task, 'performRemote').andCallFake -> + Promise.resolve(Task.Status.Continue) + @task.runRemote() + advanceClock() + expect(@task.performRemote).toHaveBeenCalled() + expect(@task.queueState.remoteError).toBe(null) + expect(@task.queueState.remoteComplete).toBe(true) + expect(@task.queueState.status).toBe(Task.Status.Continue) + + it "marks as failed if the task reverts", -> + spyOn(@task, 'performRemote').andCallFake -> + Promise.resolve(Task.Status.Failed) + @task.runRemote() + advanceClock() + expect(@task.performRemote).toHaveBeenCalled() + expect(@task.queueState.remoteError).toBe(null) + expect(@task.queueState.remoteComplete).toBe(true) + expect(@task.queueState.status).toBe(Task.Status.Failed) + describe "when performRemote resolves", -> beforeEach -> spyOn(@task, 'performRemote').andCallFake -> - Promise.resolve(Task.Status.Finished) + Promise.resolve(Task.Status.Success) it "should save that performRemote is complete with no errors", -> @task.runRemote() @@ -84,6 +111,7 @@ describe "Task", -> expect(@task.performRemote).toHaveBeenCalled() expect(@task.queueState.remoteError).toBe(null) expect(@task.queueState.remoteComplete).toBe(true) + expect(@task.queueState.status).toBe(Task.Status.Success) it "should only allow the performRemote method to return a Task.Status", -> result = null @@ -96,6 +124,7 @@ describe "Task", -> @ok.queueState.localComplete = true @ok.runRemote().then (r) -> result = r advanceClock() + expect(@ok.queueState.status).toBe(Task.Status.Retry) expect(result).toBe(Task.Status.Retry) class BadTask extends Task @@ -106,15 +135,10 @@ describe "Task", -> advanceClock() expect(err.message).toBe('performRemote returned lalal, which is not a Task.Status') - describe "when performRemote rejects", -> + describe "when performRemote rejects multiple times", -> beforeEach -> - @error = new APIError("Oh no!") - spyOn(@task, 'performRemote').andCallFake => Promise.reject(@error) - - it "should save the error to the queueState", -> - @task.runRemote().catch(noop) - advanceClock() - expect(@task.queueState.remoteError).toBe(@error) + spyOn(@task, 'performRemote').andCallFake => + Promise.resolve(Task.Status.Failed) it "should increment the number of attempts", -> runs -> @@ -125,3 +149,72 @@ describe "Task", -> @task.runRemote().catch(noop) waitsFor -> @task.queueState.remoteAttempts == 2 + + describe "when performRemote resolves with Task.Status.Failed", -> + beforeEach -> + spyOn(atom, "emitError") + @error = new APIError("Oh no!") + spyOn(@task, 'performRemote').andCallFake => + Promise.resolve(Task.Status.Failed) + + it "Should handle the error as a caught Failure", -> + waitsForPromise => + @task.runRemote().then -> + throw new Error("Should not resolve") + .catch (err) => + expect(@task.queueState.remoteError instanceof Error).toBe true + expect(@task.queueState.remoteAttempts).toBe(1) + expect(@task.queueState.status).toBe(Task.Status.Failed) + expect(atom.emitError).not.toHaveBeenCalled() + + describe "when performRemote resolves with Task.Status.Failed and an error", -> + beforeEach -> + spyOn(atom, "emitError") + @error = new APIError("Oh no!") + spyOn(@task, 'performRemote').andCallFake => + Promise.resolve([Task.Status.Failed, @error]) + + it "Should handle the error as a caught Failure", -> + waitsForPromise => + @task.runRemote().then -> + throw new Error("Should not resolve") + .catch (err) => + expect(@task.queueState.remoteError).toBe(@error) + expect(@task.queueState.remoteAttempts).toBe(1) + expect(@task.queueState.status).toBe(Task.Status.Failed) + expect(atom.emitError).not.toHaveBeenCalled() + + describe "when performRemote rejects with Task.Status.Failed", -> + beforeEach -> + spyOn(atom, "emitError") + @error = new APIError("Oh no!") + spyOn(@task, 'performRemote').andCallFake => + Promise.reject([Task.Status.Failed, @error]) + + it "Should handle the rejection as normal", -> + waitsForPromise => + @task.runRemote().then -> + throw new Error("Should not resolve") + .catch (err) => + expect(@task.queueState.remoteError).toBe(@error) + expect(@task.queueState.remoteAttempts).toBe(1) + expect(@task.queueState.status).toBe(Task.Status.Failed) + expect(atom.emitError).not.toHaveBeenCalled() + + describe "when performRemote throws an unknown error", -> + beforeEach -> + spyOn(atom, "emitError") + @error = new Error("Oh no!") + spyOn(@task, 'performRemote').andCallFake => + throw @error + + it "Should handle the error as an uncaught error", -> + waitsForPromise => + @task.runRemote().then -> + throw new Error("Should not resolve") + .catch (err) => + expect(@task.queueState.remoteError).toBe(@error) + expect(@task.queueState.remoteAttempts).toBe(1) + expect(@task.queueState.status).toBe(Task.Status.Failed) + expect(@task.queueState.debugStatus).toBe(Task.DebugStatus.UncaughtError) + expect(atom.emitError).toHaveBeenCalledWith(@error) diff --git a/src/atom.coffee b/src/atom.coffee index 62d24ad13..158ef36cb 100644 --- a/src/atom.coffee +++ b/src/atom.coffee @@ -276,6 +276,7 @@ class Atom extends Model @emitError(error) emitError: (error) -> + console.error(error) unless @inSpecMode() eventObject = {message: error.message, originalError: error} @emitter.emit('will-throw-error', eventObject) @emit('uncaught-error', error.message, null, null, null, error) diff --git a/src/flux/stores/draft-store-proxy.coffee b/src/flux/stores/draft-store-proxy.coffee index 330a151a2..f64fa3235 100644 --- a/src/flux/stores/draft-store-proxy.coffee +++ b/src/flux/stores/draft-store-proxy.coffee @@ -46,9 +46,12 @@ class DraftChangeSet clearTimeout(@_timer) if @_timer @_timer = setTimeout(@commit, 5000) - commit: => + # If force is true, then we'll always run the `_onCommit` callback + # regardless if there are _pending changes or not + commit: ({force}={}) => @_commitChain = @_commitChain.finally => - if Object.keys(@_pending).length is 0 + + if not force and Object.keys(@_pending).length is 0 return Promise.resolve(true) @_saving = @_pending diff --git a/src/flux/stores/draft-store.coffee b/src/flux/stores/draft-store.coffee index e24048333..199aac7d8 100644 --- a/src/flux/stores/draft-store.coffee +++ b/src/flux/stores/draft-store.coffee @@ -470,7 +470,15 @@ class DraftStore @_runExtensionsBeforeSend(session) # Immediately save any pending changes so we don't save after sending - session.changes.commit().then => + # + # It's important that we force commit the changes before sending. + # Once committed, we'll queue a `SyncbackDraftTask`. Since we may be + # sending a draft by its serverId, we need to make sure that the + # server has the latest changes. It's possible for the + # session.changes._pending to be empty if the last SyncbackDraftTask + # failed during its performRemote. When we send we should always try + # again. + session.changes.commit(force: true).then => task = new SendDraftTask(draftClientId, {fromPopout: @_isPopout()}) Actions.queueTask(task) @_doneWithSession(session) @@ -495,7 +503,10 @@ class DraftStore @_draftsSending[draftClientId] = false @trigger(draftClientId) if atom.isMainWindow() - _.defer -> + # We delay so the view has time to update the restored draft. If we + # don't delay the modal may come up in a state where the draft looks + # like it hasn't been restored or has been lost. + _.delay -> remote = require('remote') dialog = remote.require('dialog') dialog.showMessageBox remote.getCurrentWindow(), { @@ -504,5 +515,6 @@ class DraftStore message: "Error" detail: errorMessage } + , 100 module.exports = new DraftStore() diff --git a/src/flux/stores/task-queue.coffee b/src/flux/stores/task-queue.coffee index a5481d1dd..ed1f4ef13 100644 --- a/src/flux/stores/task-queue.coffee +++ b/src/flux/stores/task-queue.coffee @@ -143,6 +143,11 @@ class TaskQueue @_completed.shift() if @_completed.length > 1000 @_updateSoon() + dequeueTaskAndDependents: (taskOrId) -> + task = @_resolveTaskArgument(taskOrId) + if not task + throw new Error("Couldn't find task in queue to dequeue") + dequeueAll: => for task in @_queue by -1 @dequeue(task) @@ -164,8 +169,11 @@ class TaskQueue _processQueue: => for task in @_queue by -1 - continue if @_taskIsBlocked(task) - @_processTask(task) + if @_taskIsBlocked(task) + task.queueState.debugStatus = Task.DebugStatus.WaitingOnDependency + continue + else + @_processTask(task) _processTask: (task) => return if task.queueState.isProcessing @@ -178,8 +186,50 @@ class TaskQueue .then (status) => @dequeue(task) unless status is Task.Status.Retry .catch (err) => - console.warn("Task #{task.constructor.name} threw an error: #{err}.") - @dequeue(task) + @_seenDownstream = {} + @_notifyOfDependentError(task, err) + .then (responses) => + @_dequeueDownstreamTasks(responses) + @dequeue(task) + + # When we `_notifyOfDependentError`s, we collect a nested array of + # responses of the tasks we notified. We need to responses to determine + # whether or not we should dequeue that task. + _dequeueDownstreamTasks: (responses=[]) -> + # Responses are nested arrays due to the recursion + responses = _.flatten(responses) + + # A response may be `null` if it hit our infinite recursion check. + responses = _.filter responses, (r) -> r? + + responses.forEach (resp) => + if resp.returnValue is Task.DO_NOT_DEQUEUE_ME + return + else + resp.downstreamTask.queueState.status = Task.Status.Continue + resp.downstreamTask.queueState.debugStatus = Task.DebugStatus.DequeuedDependency + @dequeue(resp.downstreamTask) + + # Recursively notifies tasks of dependent errors + _notifyOfDependentError: (failedTask, err) -> + downstream = @_tasksDependingOn(failedTask) ? [] + Promise.map downstream, (downstreamTask) => + + return Promise.resolve(null) unless downstreamTask + + # Infinte recursion check! + # These will get removed later + return Promise.resolve(null) if @_seenDownstream[downstreamTask.id] + @_seenDownstream[downstreamTask.id] = true + + responseHash = Promise.props + returnValue: downstreamTask.onDependentTaskError(failedTask, err) + downstreamTask: downstreamTask + + return Promise.all([ + responseHash + @_notifyOfDependentError(downstreamTask, err) + ]) _dequeueObsoleteTasks: (task) => obsolete = _.filter @_queue, (otherTask) => @@ -191,11 +241,17 @@ class TaskQueue return task.shouldDequeueOtherTask(otherTask) for otherTask in obsolete + otherTask.queueState.status = Task.Status.Continue + otherTask.queueState.debugStatus = Task.DebugStatus.DequeuedObsolete @dequeue(otherTask) + _tasksDependingOn: (task) -> + _.filter @_queue, (otherTask) -> + otherTask.isDependentTask(task) and task isnt otherTask + _taskIsBlocked: (task) => _.any @_queue, (otherTask) -> - task.shouldWaitForTask(otherTask) and task isnt otherTask + task.isDependentTask(otherTask) and task isnt otherTask _resolveTaskArgument: (taskOrId) => if not taskOrId diff --git a/src/flux/tasks/change-folder-task.coffee b/src/flux/tasks/change-folder-task.coffee index 394f31cad..f5659b01f 100644 --- a/src/flux/tasks/change-folder-task.coffee +++ b/src/flux/tasks/change-folder-task.coffee @@ -41,7 +41,7 @@ class ChangeFolderTask extends ChangeMailTask else return "Moved objects#{folderText}" - shouldWaitForTask: (other) -> other instanceof SyncbackCategoryTask + isDependentTask: (other) -> other instanceof SyncbackCategoryTask performLocal: -> if not @folder diff --git a/src/flux/tasks/change-labels-task.coffee b/src/flux/tasks/change-labels-task.coffee index ba425747f..1f490fca3 100644 --- a/src/flux/tasks/change-labels-task.coffee +++ b/src/flux/tasks/change-labels-task.coffee @@ -33,7 +33,7 @@ class ChangeLabelsTask extends ChangeMailTask return "Removed #{@labelsToRemove[0].displayName} from #{@threads.length} #{type}" return "Changed labels on #{@threads.length} #{type}" - shouldWaitForTask: (other) -> other instanceof SyncbackCategoryTask + isDependentTask: (other) -> other instanceof SyncbackCategoryTask performLocal: -> if @labelsToAdd.length is 0 and @labelsToRemove.length is 0 diff --git a/src/flux/tasks/change-mail-task.coffee b/src/flux/tasks/change-mail-task.coffee index 71bb317fb..29737ea6a 100644 --- a/src/flux/tasks/change-mail-task.coffee +++ b/src/flux/tasks/change-mail-task.coffee @@ -156,13 +156,13 @@ class ChangeMailTask extends Task performRemote: -> @performRequests(@objectClass(), @objectArray()).then => @_ensureLocksRemoved() - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => if err.statusCode in NylasAPI.PermanentErrorCodes @_isReverting = true @performLocal().then => @_ensureLocksRemoved() - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Failed) else return Promise.resolve(Task.Status.Retry) @@ -198,9 +198,11 @@ class ChangeMailTask extends Task createUndoTask: -> if @_isUndoTask - throw new Error("ChangeMailTask::createUndoTask Cannot create an undo task from an undo task.") + err new Error("ChangeMailTask::createUndoTask Cannot create an undo task from an undo task.") + return Promise.resolve([Task.Status.Failed, err]) if not @_restoreValues - throw new Error("ChangeMailTask::createUndoTask Cannot undo a task which has not finished performLocal yet.") + err new Error("ChangeMailTask::createUndoTask Cannot undo a task which has not finished performLocal yet.") + return Promise.resolve([Task.Status.Failed, err]) task = @createIdenticalTask() task._restoreValues = @_restoreValues @@ -238,7 +240,7 @@ class ChangeMailTask extends Task # To ensure that complex offline actions are synced correctly, label/folder additions # and removals need to be applied in order. (For example, star many threads, # and then unstar one.) - shouldWaitForTask: (other) -> + isDependentTask: (other) -> # Only wait on other tasks that are older and also involve the same threads return unless other instanceof ChangeMailTask otherOlder = other.creationDate < @creationDate diff --git a/src/flux/tasks/create-metadata-task.coffee b/src/flux/tasks/create-metadata-task.coffee index 10bb2afe3..98fccccb5 100644 --- a/src/flux/tasks/create-metadata-task.coffee +++ b/src/flux/tasks/create-metadata-task.coffee @@ -40,12 +40,12 @@ class CreateMetadataTask extends Task value: @value success: => Actions.metadataCreated @type, @metadatum - resolve(Task.Status.Finished) + resolve(Task.Status.Success) error: (apiError) => Actions.metadataError _.extend @_baseErrorData(), errorType: "APIError" error: apiError - reject(apiError) + resolve(Task.Status.Failed) _baseErrorData: -> action: "create" diff --git a/src/flux/tasks/destroy-draft.coffee b/src/flux/tasks/destroy-draft.coffee index 7509ce7c6..382ef0472 100644 --- a/src/flux/tasks/destroy-draft.coffee +++ b/src/flux/tasks/destroy-draft.coffee @@ -24,7 +24,7 @@ class DestroyDraftTask extends Task else false - shouldWaitForTask: (other) -> + isDependentTask: (other) -> (other instanceof SyncbackDraftTask and other.draftClientId is @draftClientId) performLocal: -> @@ -44,8 +44,13 @@ class DestroyDraftTask extends Task # We don't need to do anything if we weren't able to find the draft # when we performed locally, or if the draft has never been synced to # the server (id is still self-assigned) - return Promise.resolve(Task.Status.Finished) unless @draft - return Promise.resolve(Task.Status.Finished) unless @draft.serverId and @draft.version? + if not @draft + err new Error("No valid draft to destroy!") + return Promise.resolve([Task.Status.Failed, err]) + + if not @draft.serverId or not @draft.version? + err new Error("Can't destroy draft without a version or serverId") + return Promise.resolve([Task.Status.Failed, err]) NylasAPI.makeRequest path: "/drafts/#{@draft.serverId}" @@ -55,21 +60,21 @@ class DestroyDraftTask extends Task version: @draft.version returnsModel: false .then => - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => inboxMsg = err.body?.message ? "" # Draft has already been deleted, this is not really an error if err.statusCode in [404, 409] - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Continue) # Draft has been sent, and can't be deleted. Not much we can do but finish if inboxMsg.indexOf("is not a draft") >= 0 - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Continue) if err.statusCode in NylasAPI.PermanentErrorCodes Actions.postNotification({message: "Unable to delete this draft. Restoring...", type: "error"}) DatabaseStore.persistModel(@draft).then => - return Promise.resolve(Task.Status.Finished) - - Promise.resolve(Task.Status.Retry) + Promise.resolve(Task.Status.Failed) + else + Promise.resolve(Task.Status.Retry) diff --git a/src/flux/tasks/destroy-metadata-task.coffee b/src/flux/tasks/destroy-metadata-task.coffee index 97d27be18..cc30c476e 100644 --- a/src/flux/tasks/destroy-metadata-task.coffee +++ b/src/flux/tasks/destroy-metadata-task.coffee @@ -57,12 +57,12 @@ class DestroyMetadataTask extends Task body: body success: => Actions.metadataDestroyed(@type) - resolve(Task.Status.Finished) + resolve(Task.Status.Success) error: (apiError) => Actions.metadataError _.extend @_baseErrorData(), errorType: "APIError" error: apiError - reject(apiError) + resolve(Task.Status.Failed) _baseErrorData: -> action: "destroy" diff --git a/src/flux/tasks/event-rsvp.coffee b/src/flux/tasks/event-rsvp.coffee index 72fbea0b3..db4ff4115 100644 --- a/src/flux/tasks/event-rsvp.coffee +++ b/src/flux/tasks/event-rsvp.coffee @@ -39,11 +39,14 @@ class EventRSVPTask extends Task } returnsModel: true .then => - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => ##TODO: event already accepted/declined/etc @event.participants = @_previousParticipantsState - DatabaseStore.persistModel(@event).then(resolve).catch(reject) + DatabaseStore.persistModel(@event).then -> + resolve(Task.Status.Failed) + .catch (err) -> + resolve(Task.Status.Failed) onOtherError: -> Promise.resolve() onTimeoutError: -> Promise.resolve() diff --git a/src/flux/tasks/file-upload-task.coffee b/src/flux/tasks/file-upload-task.coffee index ce5bbaaf0..708137c88 100644 --- a/src/flux/tasks/file-upload-task.coffee +++ b/src/flux/tasks/file-upload-task.coffee @@ -35,7 +35,10 @@ class FileUploadTask extends Task Actions.uploadStateChanged @_uploadData("started") DatabaseStore.findBy(Message, {clientId: @messageClientId}).then (draft) => - return Promise.resolve(Task.Status.Finished) unless draft + if not draft + err new Error("Can't find draft #{@messageClientId} in Database to upload file to") + return Promise.resolve([Task.Status.Failed, err]) + @_accountId = draft.accountId @_makeRequest() @@ -44,17 +47,17 @@ class FileUploadTask extends Task .then (file) => Actions.uploadStateChanged @_uploadData("completed") Actions.fileUploaded(file: file, uploadData: @_uploadData("completed")) - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => if err.statusCode in NylasAPI.PermanentErrorCodes msg = "There was a problem uploading this file. Please try again later." Actions.uploadStateChanged(@_uploadData("failed")) Actions.postNotification({message: msg, type: "error"}) - return Promise.resolve(Task.Status.Finished) + return Promise.resolve([Task.Status.Failed, err]) else if err.statusCode is NylasAPI.CancelledErrorCode Actions.uploadStateChanged(@_uploadData("aborted")) Actions.fileAborted(@_uploadData("aborted")) - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Failed) else return Promise.resolve(Task.Status.Retry) diff --git a/src/flux/tasks/mark-message-read.coffee b/src/flux/tasks/mark-message-read.coffee index f6024b197..ca6708abe 100644 --- a/src/flux/tasks/mark-message-read.coffee +++ b/src/flux/tasks/mark-message-read.coffee @@ -29,12 +29,12 @@ class MarkMessageReadTask extends Task unread: false returnsModel: true .then => - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => if err.statusCode in NylasAPI.PermanentErrorCodes # Run performLocal backwards to undo the tag changes @message.unread = @_previousUnreadState DatabaseStore.persistModel(@message).then => - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Failed) else return Promise.resolve(Task.Status.Retry) diff --git a/src/flux/tasks/send-draft.coffee b/src/flux/tasks/send-draft.coffee index 549d27ed9..656ffcacb 100644 --- a/src/flux/tasks/send-draft.coffee +++ b/src/flux/tasks/send-draft.coffee @@ -21,10 +21,17 @@ class SendDraftTask extends Task shouldDequeueOtherTask: (other) -> other instanceof SendDraftTask and other.draftClientId is @draftClientId - shouldWaitForTask: (other) -> + isDependentTask: (other) -> (other instanceof SyncbackDraftTask and other.draftClientId is @draftClientId) or (other instanceof FileUploadTask and other.messageClientId is @draftClientId) + onDependentTaskError: (task, err) -> + if task instanceof SyncbackDraftTask + msg = "Your message could not be sent because we could not save your draft. Please check your network connection and try again soon." + else if task instanceof FileUploadTask + msg = "Your message could not be sent because a file failed to upload. Please try re-uploading your file and try again." + @_notifyUserOfError(msg) if msg + performLocal: -> # When we send drafts, we don't update anything in the app until # it actually succeeds. We don't want users to think messages have @@ -42,6 +49,11 @@ class SendDraftTask extends Task if not draft return Promise.reject(new Error("We couldn't find the saved draft.")) + # Just before sending we ask the {DraftStoreProxy} to commit its + # changes. This will fire a {SyncbackDraftTask}. Since we will be + # sending the draft by its serverId, we must be ABSOLUTELY sure that + # the {SyncbackDraftTask} succeeded otherwise we will send an + # incomplete or obsolete message. if draft.serverId body = draft_id: draft.serverId @@ -81,7 +93,7 @@ class SendDraftTask extends Task draftClientId: @draftClientId newMessage: @draft - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch @_permanentError .catch APIError, (err) => @@ -92,16 +104,26 @@ class SendDraftTask extends Task body.thread_id = null body.reply_to_message_id = null return @_send(body) - else if (err.statusCode in NylasAPI.PermanentErrorCodes or - err.statusCode is NylasAPI.TimeoutErrorCode) - @_permanentError() + else if err.statusCode is 500 + msg = "Your message could not be sent at this time. Please try again soon." + return @_permanentError(err, msg) + else if err.statusCode in [400, 404] + msg = "Your message could not be sent at this time. Please try again soon." + atom.emitError(new Error("Sending a message responded with #{err.statusCode}!")) + return @_permanentError(err, msg) + else if err.statusCode is NylasAPI.TimeoutErrorCode + msg = "We lost internet connection just as we were trying to send your message! Please wait a little bit to see if it went through. If not, check your internet connection and try sending again." + return @_permanentError(err, msg) else return Promise.resolve(Task.Status.Retry) - _permanentError: => - msg = "Your draft could not be sent. Please check your network connection and try again." + _permanentError: (err, msg) => + @_notifyUserOfError(msg) + + return Promise.resolve([Task.Status.Failed, err]) + + _notifyUserOfError: (msg) => if @fromPopout Actions.composePopoutDraft(@draftClientId, {errorMessage: msg}) else Actions.draftSendingFailed({draftClientId: @draftClientId, errorMessage: msg}) - return Promise.resolve(Task.Status.Finished) diff --git a/src/flux/tasks/syncback-category-task.coffee b/src/flux/tasks/syncback-category-task.coffee index 77c14079b..a3d3a20e8 100644 --- a/src/flux/tasks/syncback-category-task.coffee +++ b/src/flux/tasks/syncback-category-task.coffee @@ -48,12 +48,12 @@ module.exports = class SyncbackCategoryTask extends Task @category.serverId = json.id DatabaseStore.persistModel @category .then -> - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => if err.statusCode in NylasAPI.PermanentErrorCodes @_isReverting = true @performLocal().then => - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Failed) else return Promise.resolve(Task.Status.Retry) diff --git a/src/flux/tasks/syncback-draft.coffee b/src/flux/tasks/syncback-draft.coffee index d1b71f41d..d5bd99aed 100644 --- a/src/flux/tasks/syncback-draft.coffee +++ b/src/flux/tasks/syncback-draft.coffee @@ -22,10 +22,19 @@ class SyncbackDraftTask extends Task super shouldDequeueOtherTask: (other) -> - other instanceof SyncbackDraftTask and other.draftClientId is @draftClientId and other.creationDate < @creationDate + other instanceof SyncbackDraftTask and + other.draftClientId is @draftClientId and + other.creationDate <= @creationDate - shouldWaitForTask: (other) -> - other instanceof SyncbackDraftTask and other.draftClientId is @draftClientId and other.creationDate < @creationDate + isDependentTask: (other) -> + other instanceof SyncbackDraftTask and + other.draftClientId is @draftClientId and + other.creationDate <= @creationDate + + # We want to wait for other SyncbackDraftTasks to run, but we don't want + # to get dequeued if they fail. + onDependentTaskError: -> + return Task.DO_NOT_DEQUEUE_ME performLocal: -> # SyncbackDraftTask does not do anything locally. You should persist your changes @@ -84,7 +93,7 @@ class SyncbackDraftTask extends Task DatabaseStore.persistModel(draft) .then => - return Promise.resolve(Task.Status.Finished) + return Promise.resolve(Task.Status.Success) .catch APIError, (err) => if err.statusCode in [400, 404, 409] and err.requestOptions?.method is 'PUT' @@ -92,12 +101,13 @@ class SyncbackDraftTask extends Task if not draft then draft = oldDraft @detatchFromRemoteID(draft).then -> return Promise.resolve(Task.Status.Retry) - - else if err.statusCode in NylasAPI.PermanentErrorCodes - return Promise.resolve(Task.Status.Finished) - else - return Promise.resolve(Task.Status.Finished) + # NOTE: There's no offline handling. If we're offline + # SyncbackDraftTasks should always fail. + # + # We don't roll anything back locally, but this failure + # ensures that SendDraftTasks can never succeed while offline. + Promise.resolve([Task.Status.Failed, err]) getLatestLocalDraft: => DatabaseStore.findBy(Message, clientId: @draftClientId).include(Message.attributes.body) diff --git a/src/flux/tasks/task.coffee b/src/flux/tasks/task.coffee index 48dca7e76..9ad10fa9d 100644 --- a/src/flux/tasks/task.coffee +++ b/src/flux/tasks/task.coffee @@ -1,53 +1,163 @@ _ = require 'underscore' {generateTempId} = require '../models/utils' - TaskStatus = - Finished: 'finished' - Retry: 'retry' + Retry: "RETRY" + Success: "SUCCESS" + Continue: "CONTINUE" + Failed: "FAILED" -# Public: Tasks represent individual changes to the datastore that -# alter the local cache and need to be synced back to the server. +TaskDebugStatus = + JustConstructed: "JUST CONSTRUCTED" + UncaughtError: "UNCAUGHT ERROR" + DequeuedObsolete: "DEQUEUED (Obsolete)" + DequeuedDependency: "DEQUEUED (Dependency Failure)" + WaitingOnQueue: "WAITING ON QUEUE" + WaitingOnDependency: "WAITING ON DEPENDENCY" + RunningLocal: "RUNNING LOCAL" + ProcessingRemote: "PROCESSING REMOTE" + +# Public: Tasks are a robust way to handle any mutating changes that need +# to interface with a remote API. # -# To create a new task, subclass Task and implement the following methods: +# Tasks help you handle and encapsulate optimistic updates, rollbacks, +# undo/redo, API responses, API errors, queuing, and multi-step +# dependencies. # -# - performLocal: -# Return a {Promise} that does work immediately. Must resolve or the task -# will be thrown out. Generally, you should optimistically update -# the local cache here. +# They are especially useful in offline mode. Users may have taken tons of +# actions that we've queued up to process when they come back online. # -# - performRemote: -# Do work that requires dependencies to have resolved and may need to be -# tried multiple times to succeed in case of network issues. +# Tasks represent individual changes to the datastore that alter the local +# cache and need to be synced back to the server. # -# performRemote must return a {Promise}, and it should always resolve with -# `Task.Status.Finished` or `Task.Status.Retry`. Rejections are considered -# exception cases and are logged to our server. +# To create your own task, subclass Task and implement the following +# required methods: # -# Returning `Task.Status.Retry` will cause the `TaskQueue` to leave your task -# on the queue and run it again later. You should only return `Task.Status.Retry` -# if your task encountered a transient error (for example, a `0` but not a `400`). +# - {Task::performLocal} +# - {Task::performRemote} # -# - shouldWaitForTask: -# Tasks may be arbitrarily dependent on other tasks. To ensure that -# `performRemote` is called at the right time, subclasses should implement -# `shouldWaitForTask(other)`. For example, the `SendDraft` task is dependent -# on the draft's files' `UploadFile` tasks completing. +# See their usage in the documentation below. # -# Tasks may also implement `shouldDequeueOtherTask(other)`. Returning true -# will cause the other event to be removed from the queue. This is useful in -# offline mode especially, when the user might `Save`,`Save`,`Save`,`Save`,`Send`. -# Each newly queued `Save` can cancel the (unstarted) save task in the queue. +# ## Task Dependencies # -# Tasks that need to support undo/redo should implement `canBeUndone`, `isUndo`, -# `createUndoTask`, and `createIdenticalTask`. +# The Task system handles dependencies between multiple queued tasks. For +# example, the {SendDraftTask} has a dependency on the {SyncbackDraftTask} +# (aka saving) succeeding. To establish dependencies between tasks, your +# subclass may implement one or more of the following methods: +# +# - {Task::isDependentTask} +# - {Task::onDependentTaskError} +# - {Task::shouldDequeueOtherTask} +# +# ## Undo / Redo +# +# The Task system also supports undo/redo handling. Your subclass must +# implement the following methods to enable this: +# +# - {Task::isUndo} +# - {Task::canBeUndone} +# - {Task::createUndoTask} +# - {Task::createIdenticalTask} +# +# ## Offline Considerations +# +# All tasks should gracefully handle the case when there is no network +# connection. +# +# If we're offline the common behavior is for a task to: +# +# 1. Perform its local change +# 2. Attempt the remote request and get a timeout or offline code +# 3. Have `performRemote` resolve a `Task.Status.Retry` +# 3. Sit queued up waiting to be retried +# 4. Wait for {Actions::longPollConnected} to restart the {TaskQueue} +# +# Remember that a user may be offline for hours and perform thousands of +# tasks in the meantime. It's important that your tasks implement +# `shouldDequeueOtherTask` and `isDependentTask` to make sure ordering +# always remains correct. +# +# ## Serialization and Window Considerations +# +# The whole {TaskQueue} and all of its Tasks are serialized and stored in +# the Database. This allows the {TaskQueue} to work across windows and +# ensures we don't lose any pending tasks if a user is offline for a while +# and quits and relaunches the application. +# +# All instance variables you create must be able to be serialized to a +# JSON string and re-inflated. Notably, **`function` objects will not be +# properly re-inflated**. +# +# If you have instance variables that are instances of core {Model} +# classes or {Task} classes, they will be automatically re-inflated to the +# correct class via {Utils::deserializeRegisteredObject}. If you create +# your own custom classes, they must be registered once per window via +# {TaskRegistry::register} +# +# ## Example Task +# +# **Task Definition**: +# +# ```coffee +# _ = require 'underscore' +# request = require 'request' +# {Task, DatabaseStore} = require('nylas-exports') +# +# class UpdateTodoTask extends Task +# constructor: (@existingTodo, @newData) -> +# super +# +# performLocal: -> +# @updatedTodo = _.extend(_.clone(@existingTodo), @newData) +# return DatabaseStore.persistModel(@updatedTodo) +# +# performRemote: -> +# new Promise (resolve, reject) => +# options = {url: "https://myapi.co", method: 'PUT', json: @newData} +# request options, (error, response, body) -> +# if error then resolve(Task.Status.Failed) +# else resolve(Task.Status.Success) +# +# module.exports = UpdateTodoTask +# ``` +# +# **Task Usage**: +# +# ```coffee +# {Actions} = require('nylas-exports') +# UpdateTodoTask = require('./update-todo-task') +# +# someMethod: -> +# ... +# +# task = new UpdateTodoTask(existingTodo, name: "Test") +# Actions.queueTask(task) +# +# ... +# +# ``` +# +# This example `UpdateTodoTask` does not handle undo/redo, nor does it +# rollback the changes if there's an API error. See examples in +# {Task::performLocal} for ideas on how to handle this. # class Task @Status: TaskStatus + @DebugStatus: TaskDebugStatus + # A constant that can be returned by `onDependentTaskError` to prevent + # this task from being dequeued + @DO_NOT_DEQUEUE_ME = "DO_NOT_DEQUEUE_ME" + + # Public: Override the constructor to pass initial args to your Task and + # initialize instance variables. + # + # **IMPORTANT:** If you override the constructor, be sure to call + # `super`. + # + # On construction, all Tasks instances are given a unique `id`. constructor: -> @_rememberedToCallSuper = true - @id = generateTempId() @creationDate = new Date() @queueState = @@ -57,8 +167,11 @@ class Task remoteError: null remoteAttempts: 0 remoteComplete: false + status: null + debugStatus: Task.DebugStatus.JustConstructed @ + # Private: This is a internal wrapper around `performLocal` runLocal: -> if not @_rememberedToCallSuper throw new Error("Your must call `super` from your Task's constructors") @@ -66,72 +179,350 @@ class Task if @queueState.localComplete return Promise.resolve() else - @performLocal() - .then => - @queueState.localComplete = true - @queueState.localError = null - return Promise.resolve() - .catch (err) => - @queueState.localError = err - return Promise.reject(err) + @queueState.debugStatus = Task.DebugStatus.RunningLocal + try + @performLocal() + .then => + @queueState.localComplete = true + @queueState.localError = null + @queueState.debugStatus = Task.DebugStatus.WaitingOnQueue + return Promise.resolve() + .catch(@_handleLocalError) + catch err + return @_handleLocalError(err) + _handleLocalError: (err) => + @queueState.localError = err + @queueState.status = Task.Status.Failed + @queueState.debugStatus = Task.DebugStatus.UncaughtError + atom.emitError(err) + return Promise.reject(err) + + # Private: This is an internal wrapper around `performRemote` runRemote: -> + @queueState.debugStatus = Task.DebugStatus.ProcessingRemote + if @queueState.localComplete is false throw new Error("runRemote called before performLocal complete, this is an assertion failure.") if @queueState.remoteComplete - return Promise.resolve(Task.Status.Finished) + @queueState.status = Task.Status.Continue + return Promise.resolve(Task.Status.Continue) - @performRemote() - .catch (err) => - @queueState.remoteAttempts += 1 - @queueState.remoteError = err - .then (status) => - if not (status in _.values(Task.Status)) - throw new Error("performRemote returned #{status}, which is not a Task.Status") - @queueState.remoteAttempts += 1 - @queueState.remoteComplete = status is Task.Status.Finished - @queueState.remoteError = null - return Promise.resolve(status) + try + @performRemote() + .then (compositeStatus) => + [status, err] = @_compositeStatus(compositeStatus) + if status is Task.Status.Failed + # We reject here to end up on the same path as people who may + # have manually `reject`ed the promise + return Promise.reject(compositeStatus) - ## Everything beneath here may be overridden in subclasses ## + @queueState.status = status + @queueState.remoteAttempts += 1 + @queueState.remoteComplete = status in [Task.Status.Success, Task.Status.Continue] + @queueState.remoteError = null + return Promise.resolve(status) + .catch (compositeStatus) => + [status, err] = @_compositeStatus(compositeStatus) + @_handleRemoteError(err, status) + catch err + return @_handleRemoteError(err) - # performLocal is called once when the task is queued. You must return - # a promise. If you resolve, the task is queued and performRemote will - # be called. If you reject, the task will not be queued. + # When resolving from performRemote, people can resolve one of the + # `Task.Status` constants. In the case of `Task.Status.Failed`, they can + # return an array with the constant as the first item and the error + # object as the second item. We are also resilient to accidentally + # getting passed malformed values or error objects. # + # This always returns in the form of `[status, err]` + _compositeStatus: (compositeStatus) -> + if compositeStatus instanceof Error + return [Task.Status.Failed, compositeStatus] + else if _.isString(compositeStatus) + if compositeStatus in _.values(Task.Status) + return [compositeStatus, null] + else + err = new Error("performRemote returned #{compositeStatus}, which is not a Task.Status") + return [Task.Status.Failed, err] + else if _.isArray(compositeStatus) + status = compositeStatus[0] + err = compositeStatus[1] + return [status, err] + else + err = new Error("performRemote returned #{compositeStatus}, which is not a Task.Status") + return [Task.Status.Failed, err] + + _handleRemoteError: (err, status) => + # Sometimes users just indicate that a task Failed, but don't provide + # the error object + err ?= new Error("Unexpected remote error in #{Task.constructor.name}") + + if status isnt Task.Status.Failed + @queueState.debugStatus = Task.DebugStatus.UncaughtError + atom.emitError(err) + + @queueState.status = Task.Status.Failed + @queueState.remoteAttempts += 1 + @queueState.remoteError = err + + return Promise.reject(err) + + ######################################################################## + ######################## METHODS TO OVERRIDE ########################### + ######################################################################## + + ##### REQUIRED METHODS ##### + + # Public: **Required** | Override to perform local, optimistic updates. + # + # Most tasks will put code in here that updates the {DatabaseStore} + # + # You should also implement the rollback behavior inside of + # `performLocal` or in some helper method. It's common practice (but not + # automatic) for `performLocal` to be re-called at the end of an API + # failure from `performRemote`. + # + # That rollback behavior is also likely the same when you want to undo a + # task. It's common practice (but not automatic) for `createUndoTask` to + # set some flag that `performLocal` will recognize to implement the + # rollback behavior. + # + # `performLocal` will complete BEFORE the task actually enters the + # {TaskQueue}. + # + # If you would like to do work after `performLocal` has run, you can use + # {TaskQueueStatusStore::waitForPerformLocal}. Pass it the task and it + # will return a Promise that resolves once the local action has + # completed. This is contained in the {TaskQueueStatusStore} so you can + # listen to tasks across windows. + # + # ## Examples: + # + # ### Simple Optimistic Updating: + # + # ```coffee + # class MyTask extends Task + # performLocal: -> + # @updatedModel = @_myModelUpdateCode() + # return DatabaseStore.persistModel(@updatedModel) + # ``` + # + # ### Handling rollback on API failure + # + # ```coffee + # class MyTask extends Task + # performLocal: -> + # if @_reverting + # @updatedModel = @_myModelRollbackCode() + # else + # @updatedModel = @_myModelUpdateCode() + # return DatabaseStore.persistModel(@updatedModel) + # + # performRemote: -> + # @_APIPutHelperMethod(@updatedModel).catch (apiError) => + # if apiError.statusCode is 500 + # @_reverting = true + # @performLocal() + # ``` + # + # ### Handling an undo task + # + # ```coffee + # class MyTask extends Task + # performLocal: -> + # if @_isUndoTask + # @updatedModel = @_myModelRollbackCode() + # else + # @updatedModel = @_myModelUpdateCode() + # return DatabaseStore.persistModel(@updatedModel) + # + # createUndoTask: -> + # undoTask = @createIdenticalTask() + # undoTask._isUndoTask = true + # return undoTask + # ``` + # + # Also see the documentation on the required undo methods + # + # Returns a {Promise} that resolves when your updates are complete. performLocal: -> Promise.resolve() + # Public: **Required** | Put the actual API request code here. + # + # You must return a {Promise} that resolves to one of the following + # status constants: + # + # - `Task.Status.Success` + # - `Task.Status.Retry` + # - `Task.Status.Continue` + # - `Task.Status.Failed` + # + # The resolved status will determine what the {TaskQueue} does with this + # task when it is finished. + # + # This is where you should put your actual API code. You can use the + # node `request` library to easily hit APIs, or use the {NylasAPI} class + # to talk to the [Nylas Platform API](https://nylas.com/docs). + # + # Here is a more detailed explanation of Task Statuses: + # + # ### Task.Status.Success + # + # Resolve to `Task.Status.Success` when the task successfully completes. + # Once done, the task will be dequeued and logged as a success. + # + # ### Task.Status.Retry + # + # If you resolve `Task.Status.Retry`, the task will remain on the queue + # and tried again later. Any other task dependent on the current one + # will also continue waiting. + # + # The queue is re-processed whenever a new task is enqueued, dequeued, + # or the internet connection comes back online via + # {Actions::longPollConnected}. + # + # `Task.Status.Retry` is useful if it looks like we're offline, or you + # get an API error code that indicates temporary failure. + # + # ### Task.Status.Continue + # + # Resolving `Task.Status.Continue` will silently dequeue the task, allow + # dependent tasks through, but not mark it as successfully resolved. + # + # This is useful if you get permanent API errors, but don't really care + # if the task failed. + # + # ### Task.Status.Failed + # + # If you catch a permanent API error code (like a 500), or something + # else goes wrong then resolve to `Task.Status.Failed`. + # + # Resolving `Task.Status.Failed` will dequeue this task, and **dequeue + # all dependent tasks**. + # + # You can optionally return the error object itself for debugging + # purposes by resolving an array of the form: `[Task.Status.Failed, + # errorObject]` + # + # You should not `throw` exceptions. Catch all cases yourself and + # determine which `Task.Status` to resolve to. If due to programmer + # error an exception is thrown, our {TaskQueue} will catch it, log it, + # and deal with the task as if it resolved `Task.Status.Failed`. + # + # Returns a {Promise} that resolves to a valid `Task.Status` type. performRemote: -> - Promise.resolve(Task.Status.Finished) + Promise.resolve(Task.Status.Success) - cancel: -> - # We ignore requests to cancel and carry on. Subclasses that want to support - # cancellation or dequeue requests while running should implement cancel. - canBeUndone: -> false + ##### DEPENDENCY METHODS ##### + # Public: determines which other tasks this one is dependent on. + # + # - `other` An instance of a {Task} you must test to see if it's a + # dependency of this one. + # + # Any task that passes the truth test will be considered a "dependency". + # + # If a "dependency" has a `Task.Status.Failed`, then all downstream + # tasks will get dequeued recursively. + # + # Returns `true` (is dependent on) or `false` (is not dependent on) + isDependentTask: (other) -> false + + # Public: called when a dependency errors out + # + # - `task` An instance of the dependent {Task} that errored. + # - `err` The Error object (if any) + # + # If a dependent task (anything for which {Task::isDependentTask} returns + # true) resolves with `Task.Status.Failed`, then this method will be + # called. + # + # This is an opportunity to cleanup or notify users of the error. + # + # By default, since a dependency failed, **this task will be dequeued** + # + # However, if you return the special `Task.DO_NOT_DEQUEUE_ME` constant, + # this task will not get dequeued and processed in turn. + # + # Returns if you return the `Task.DO_NOT_DEQUEUE_ME` constant, then this + # task will not get dequeued. Any other return value (including `false`) + # will proceed with the default behavior and dequeue this task. + onDependentTaskError: (task, err) -> + + # Public: determines which other tasks this one should dequeue. + # + # - `other` An instance of a {Task} you must test to see if it's now + # obsolete. + # + # Any task that passes the truth test will be considered "obsolete" and + # dequeued immediately. + # + # This is particularly useful in offline mode. Users may queue up tons + # of tasks but when we come back online to process them, we only want to + # process the latest one. + # + # Returns `true` (should dequeue) or `false` (should not dequeue) + shouldDequeueOtherTask: (other) -> false + + + ##### UNDO / REDO METHODS ##### + + # Public: It's up to you to determine how you want to indicate whether + # or not you have an instance of an "Undo Task". We commonly use a + # simple instance variable boolean flag. + # + # Returns `true` (is an Undo Task) or `false` (is not an Undo Task) isUndo: -> false + # Public: Determines whether or not this task can be undone via the + # {UndoRedoStore} + # + # Returns `true` (can be undone) or `false` (can't be undone) + canBeUndone: -> false + + # Public: Return from `createIdenticalTask` and set a flag so your + # `performLocal` and `performRemote` methods know that this is an undo + # task. createUndoTask: -> throw new Error("Unimplemented") + # Public: Return a deep-cloned task to be used for an undo task createIdenticalTask: -> json = @toJSON() delete json['queueState'] (new @.constructor).fromJSON(json) + + ##### OTHER METHODS ##### + + # Public: code to run if someone tries to dequeue your task while it is + # in flight. + # + # For example, the {FileUploadTask} implements `cancel` to `abort` the + # http request if someone dequeues it. Once `abort`ed, an error is + # thrown in `performRemote` and handled accordingly. + cancel: -> + + # Public: (optional) A string displayed to users when your task is run. + # + # When tasks are run, we automatically display a notification to users + # of the form "label (numberOfImpactedItems)". If this does not a return + # a string, no notification is displayed + label: -> + + # Public: A string displayed to users indicating how many items your + # task affected. numberOfImpactedItems: -> 1 - shouldDequeueOtherTask: (other) -> false - - shouldWaitForTask: (other) -> false - + # Private: Allows for serialization of tasks toJSON: -> @ + # Private: Allows for deserialization of tasks fromJSON: (json) -> for key,val of json @[key] = val diff --git a/src/serializable-registry.coffee b/src/serializable-registry.coffee index 0ffadf042..85307a91e 100644 --- a/src/serializable-registry.coffee +++ b/src/serializable-registry.coffee @@ -1,7 +1,7 @@ _ = require 'underscore' -# This keeps track of constructors so we know how to inflate serialized -# objects. +# Public: This keeps track of constructors so we know how to inflate +# serialized objects. # # If 3rd party packages want to register new inflatable models, they can # use `register` and pass the constructor along with the name.