From 63b929e59b7fd08d49dbb8f25d45cc501b43f18c Mon Sep 17 00:00:00 2001
From: Juan Tejada <juans.tejada@gmail.com>
Date: Sun, 26 Jun 2016 09:52:03 -0700
Subject: [PATCH] Fix missing promise based error handling in sync-worker

---
 packages/nylas-core/imap-connection.js        | 20 +++++++------
 .../imap/fetch-messages-in-category.js        |  1 +
 packages/nylas-sync/sync-worker.js            | 28 ++++++++++---------
 3 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/packages/nylas-core/imap-connection.js b/packages/nylas-core/imap-connection.js
index e00dda53b..e1fabd2a6 100644
--- a/packages/nylas-core/imap-connection.js
+++ b/packages/nylas-core/imap-connection.js
@@ -166,7 +166,7 @@ class IMAPConnection extends EventEmitter {
     if (this._settings.refresh_token) {
       const xoauthFields = ['client_id', 'client_secret', 'imap_username', 'refresh_token'];
       if (Object.keys(_.pick(this._settings, xoauthFields)).length !== 4) {
-        throw new Error(`IMAPConnection: Expected ${xoauthFields.join(',')} when given refresh_token`)
+        return Promise.reject(new Error(`IMAPConnection: Expected ${xoauthFields.join(',')} when given refresh_token`))
       }
       return new Promise((resolve, reject) => {
         xoauth2.createXOAuth2Generator({
@@ -223,6 +223,7 @@ class IMAPConnection extends EventEmitter {
   end() {
     this._queue = [];
     this._imap.end();
+    this._imap = null;
   }
 
   serverSupports(capability) {
@@ -251,29 +252,30 @@ class IMAPConnection extends EventEmitter {
   }
 
   processNextOperation() {
-    if (this._currentOperation) { return; }
+    if (this._currentOperation) { return }
     this._currentOperation = this._queue.shift();
     if (!this._currentOperation) {
       this.emit('queue-empty');
-      return;
+      return
     }
 
     const {operation, resolve, reject} = this._currentOperation;
     console.log(`Starting task ${operation.description()}`)
     const result = operation.run(this._db, this);
     if (result instanceof Promise === false) {
-      throw new Error(`Expected ${operation.constructor.name} to return promise.`);
+      reject(new Error(`Expected ${operation.constructor.name} to return promise.`))
     }
-    result.catch((err) => {
-      this._currentOperation = null;
-      console.error(err);
-      reject();
-    })
+    result
     .then(() => {
       this._currentOperation = null;
       console.log(`Finished task ${operation.description()}`)
       resolve();
     })
+    .catch((err) => {
+      this._currentOperation = null;
+      console.error(err);
+      reject(err);
+    })
     .finally(() => {
       this.processNextOperation();
     });
diff --git a/packages/nylas-sync/imap/fetch-messages-in-category.js b/packages/nylas-sync/imap/fetch-messages-in-category.js
index 600a6adb1..dbb1ddcd0 100644
--- a/packages/nylas-sync/imap/fetch-messages-in-category.js
+++ b/packages/nylas-sync/imap/fetch-messages-in-category.js
@@ -114,6 +114,7 @@ class FetchMessagesInCategory {
 
   _fetchMessagesAndQueueForProcessing(range) {
     const messagesObservable = this._box.fetch(range)
+    // TODO Error handling here
     messagesObservable.subscribe(this._processMessage.bind(this))
     return messagesObservable.toPromise()
   }
diff --git a/packages/nylas-sync/sync-worker.js b/packages/nylas-sync/sync-worker.js
index a8ae9d0a2..11b8cd03d 100644
--- a/packages/nylas-sync/sync-worker.js
+++ b/packages/nylas-sync/sync-worker.js
@@ -22,9 +22,8 @@ class SyncWorker {
 
     this.syncNow();
 
-    this._listener = PubsubConnector.observableForAccountChanges(account.id).subscribe(() => {
-      this.onAccountChanged();
-    });
+    this._listener = PubsubConnector.observableForAccountChanges(account.id)
+    .subscribe(() => this.onAccountChanged())
   }
 
   cleanup() {
@@ -47,18 +46,19 @@ class SyncWorker {
     const {afterSync} = this._account.syncPolicy;
 
     if (afterSync === 'idle') {
-      this.getInboxCategory().then((inboxCategory) => {
-        this._conn.openBox(inboxCategory.name, true).then(() => {
+      return this.getInboxCategory()
+      .then((inboxCategory) => {
+        this._conn.openBox(inboxCategory.name).then(() => {
           console.log("SyncWorker: - Idling on inbox category");
-        });
+        })
       });
     } else if (afterSync === 'close') {
       console.log("SyncWorker: - Closing connection");
       this._conn.end();
       this._conn = null;
-    } else {
-      throw new Error(`onSyncDidComplete: Unknown afterSync behavior: ${afterSync}`)
+      return Promise.resolve()
     }
+    return Promise.reject(new Error(`onSyncDidComplete: Unknown afterSync behavior: ${afterSync}`))
   }
 
   onConnectionIdleUpdate() {
@@ -73,15 +73,15 @@ class SyncWorker {
     if (this._conn) {
       return this._conn.connect();
     }
-    return new Promise((resolve) => {
+    return new Promise((resolve, reject) => {
       const settings = this._account.connectionSettings;
       const credentials = this._account.decryptedCredentials();
 
       if (!settings || !settings.imap_host) {
-        throw new Error("ensureConnection: There are no IMAP connection settings for this account.")
+        return reject(new Error("ensureConnection: There are no IMAP connection settings for this account."))
       }
       if (!credentials) {
-        throw new Error("ensureConnection: There are no IMAP connection credentials for this account.")
+        return reject(new Error("ensureConnection: There are no IMAP connection credentials for this account."))
       }
 
       const conn = new IMAPConnection(this._db, Object.assign({}, settings, credentials));
@@ -145,11 +145,13 @@ class SyncWorker {
     .then(this.fetchCategoryList.bind(this))
     .then(this.syncbackMessageActions.bind(this))
     .then(this.fetchMessagesInCategory.bind(this))
+    // TODO Update account sync state in this error handler
     .catch(console.error)
     .finally(() => {
       this._lastSyncTime = Date.now()
-      this.onSyncDidComplete();
-      this.scheduleNextSync();
+      this.onSyncDidComplete()
+      .catch((error) => console.error('SyncWorker.syncNow: Unhandled error while cleaning up sync: ', error))
+      .finally(() => this.scheduleNextSync())
     });
   }