[iso-core] IMAPConnectionPool now correctly disposes connections onDone

Summary:
Previously, calling the IMAPConnectionPool `onDone` callback when checking out a connection would just return the connection to the pool without re-establishing it the next time we checked it out of the pool.

When the `onDone` callback was called in an error scenario, this had the unintended consequence of returning a bad connection to the pool, and then re-using this bad connection. The only scenario when the connection was re-established was if `onConnected` threw an error, but if the connection was kept open outside the scope of onConnected, this logic would never run.

To make things simpler and more consistent, `onDone` will always destroy all connections and connections will always be re-established from scratch every time they are checked out of the pool. The pool acts more as a limit to the number of connections per account, than an actual shared pool of connections.

Test Plan: manual

Reviewers: evan, spang, halla, mark

Reviewed By: halla, mark

Differential Revision: https://phab.nylas.com/D4360

packages/client-sync/src/local-sync-worker/sync-worker.es6

@@ -260,7 +260,7 @@ class SyncWorker {
       desiredCount: 1,
       logger: this._logger,
       socketTimeout: this._retryScheduler.currentDelay(),
-      onConnected: async ([listenerConn], done) => {
+      onConnected: async ([listenerConn], onDone) => {
         this._mailListenerIMAPConn = listenerConn;
         this._mailListenerIMAPConn._db = this._db;
 
@@ -276,7 +276,7 @@ class SyncWorker {
           this._onInboxUpdates(`There are flag updates on the inbox`);
         })
 
-        this._mailListenerIMAPConnDisposer = done
+        this._mailListenerIMAPConnDisposer = onDone
         // Return true to keep connection open
         return true
       },
@@ -616,7 +616,7 @@ class SyncWorker {
     } finally {
       this._lastSyncTime = Date.now()
       this._syncInProgress = false
-      this._disposeMainIMAPConnection({errored: false})
+      this._disposeMainIMAPConnection()
       await this._scheduleNextSync(error)
     }
   }

packages/isomorphic-core/src/imap-connection-pool.es6

@@ -53,9 +53,16 @@ class AccountConnectionPool {
 
     let conns = [];
     let keepOpen = false;
+    let calledOnDone = false;
 
-    const done = () => {
-      conns.filter(Boolean).forEach((conn) => conn.removeAllListeners());
+    const onDone = () => {
+      if (calledOnDone) { return }
+      calledOnDone = true
+      keepOpen = false;
+
+      conns.filter(Boolean).forEach(conn => conn.end());
+      conns.filter(Boolean).forEach(conn => conn.removeAllListeners());
+      conns.fill(null);
       this._availableConns = conns.concat(this._availableConns);
       if (this._queue.length > 0) {
         const resolveWaitForConnection = this._queue.shift();
@@ -67,20 +74,15 @@ class AccountConnectionPool {
       for (let i = 0; i < desiredCount; ++i) {
         conns.push(this._availableConns.shift());
       }
-      conns = await Promise.all(conns.map((c) => (c || this._genConnection(socketTimeout, logger))));
+      conns = await Promise.all(conns.map(() => this._genConnection(socketTimeout, logger)));
 
-      // TODO: Indicate which connections had errors so that we can selectively
-      // refresh them.
-      keepOpen = await onConnected(conns, done);
-    } catch (err) {
-      keepOpen = false;
-      conns.filter(Boolean).forEach(conn => conn.end());
-      conns.fill(null);
-      throw err;
-    } finally {
+      keepOpen = await onConnected(conns, onDone);
       if (!keepOpen) {
-        done();
+        onDone();
       }
+    } catch (err) {
+      onDone()
+      throw err;
     }
   }
 }