fix(databse): fix memory leak on DatabaseStore.atomically

Summary:
The Promise chain we were creating was never cleared and created a memory
leak. We instead use a `PromiseQueue` to cleanup finished promises.

Also added several more tests and verified that the memory leak is gone
with the Chrome profiler

Test Plan: new tests

Reviewers: bengotow

Reviewed By: bengotow

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

package.json

@@ -50,6 +50,7 @@
     "optimist": "0.4.0",
     "pathwatcher": "^4.4.0",
     "property-accessors": "^1",
+    "promise-queue": "2.1.1",
     "q": "^1.0.1",
     "raven": "0.7.2",
     "react": "^0.13.2",

spec/stores/database-store-spec.coffee

@@ -343,13 +343,22 @@ describe "DatabaseStore", ->
           expect(@performed[1].query).toBe "TEST"
           expect(@performed[2].query).toBe "COMMIT"
 
-    it "resolves, but doesn't fire a commit on failure", ->
+    it "preserves resolved values", ->
+      waitsForPromise =>
+        DatabaseStore.atomically( =>
+          DatabaseStore._query("TEST")
+          return Promise.resolve("myValue")
+        ).then (myValue) =>
+          expect(myValue).toBe "myValue"
+
+    it "always fires a COMMIT, even if the promise fails", ->
       waitsForPromise =>
         DatabaseStore.atomically( =>
           throw new Error("BOOO")
         ).catch =>
-          expect(@performed.length).toBe 1
+          expect(@performed.length).toBe 2
           expect(@performed[0].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
+          expect(@performed[1].query).toBe "COMMIT"
 
     it "can be called multiple times and get queued", ->
       waitsForPromise =>
@@ -366,6 +375,64 @@ describe "DatabaseStore", ->
           expect(@performed[4].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
           expect(@performed[5].query).toBe "COMMIT"
 
+    it "carries on if one of them fails, but still calls the COMMIT for the failed block", ->
+      caughtError = false
+      DatabaseStore.atomically( => DatabaseStore._query("ONE") )
+      DatabaseStore.atomically( => throw new Error("fail") ).catch ->
+        caughtError = true
+      DatabaseStore.atomically( => DatabaseStore._query("THREE") )
+      advanceClock(100)
+      expect(@performed.length).toBe 8
+      expect(@performed[0].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
+      expect(@performed[1].query).toBe "ONE"
+      expect(@performed[2].query).toBe "COMMIT"
+      expect(@performed[3].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
+      expect(@performed[4].query).toBe "COMMIT"
+      expect(@performed[5].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
+      expect(@performed[6].query).toBe "THREE"
+      expect(@performed[7].query).toBe "COMMIT"
+      expect(caughtError).toBe true
+
+    it "is actually running in series and blocks on never-finishing specs", ->
+      resolver = null
+      DatabaseStore.atomically( -> )
+      advanceClock(100)
+      expect(@performed.length).toBe 2
+      expect(@performed[0].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
+      expect(@performed[1].query).toBe "COMMIT"
+      DatabaseStore.atomically( -> new Promise (resolve, reject) -> resolver = resolve)
+      advanceClock(100)
+      blockedPromiseDone = false
+      DatabaseStore.atomically( -> ).then =>
+        blockedPromiseDone = true
+      advanceClock(100)
+      expect(@performed.length).toBe 3
+      expect(@performed[2].query).toBe "BEGIN EXCLUSIVE TRANSACTION"
+      expect(blockedPromiseDone).toBe false
+
+      # Now that we've made our assertion about blocking, we need to clean up
+      # our test and actually resolve that blocked promise now, otherwise
+      # remaining tests won't run properly.
+      advanceClock(100)
+      resolver()
+      advanceClock(100)
+      expect(blockedPromiseDone).toBe true
+      advanceClock(100)
+
+    it "can be called multiple times and preserve return values", ->
+      waitsForPromise =>
+        v1 = null
+        v2 = null
+        v3 = null
+        Promise.all([
+          DatabaseStore.atomically( -> "a" ).then (val) -> v1 = val
+          DatabaseStore.atomically( -> "b" ).then (val) -> v2 = val
+          DatabaseStore.atomically( -> "c" ).then (val) -> v3 = val
+        ]).then =>
+          expect(v1).toBe "a"
+          expect(v2).toBe "b"
+          expect(v3).toBe "c"
+
     it "can be called multiple times and get queued", ->
       waitsForPromise =>
         DatabaseStore.atomically( -> )

src/apm-wrapper.coffee

@@ -210,7 +210,6 @@ class APMWrapper
       if code is 0
         callback?()
       else
-        atom.packages.activatePackage(name) if activateOnFailure
         error = new Error(errorMessage)
         error.stdout = stdout
         error.stderr = stderr

src/flux/stores/database-store.coffee

@@ -8,6 +8,7 @@ Utils = require '../models/utils'
 Actions = require '../actions'
 ModelQuery = require '../models/query'
 NylasStore = require '../../global/nylas-store'
+PromiseQueue = require 'promise-queue'
 DatabaseSetupQueryBuilder = require './database-setup-query-builder'
 PriorityUICoordinator = require '../../priority-ui-coordinator'
 
@@ -468,18 +469,18 @@ class DatabaseStore extends NylasStore
       Promise.resolve(data)
 
   atomically: (fn) =>
-    @_atomicPromise ?= Promise.resolve()
+    maxConcurrent = 1
-    @_atomicPromise = @_atomicPromise.then =>
+    maxQueue = Infinity
-      @_atomically(fn)
+    @_promiseQueue ?= new PromiseQueue(maxConcurrent, maxQueue)
-    return @_atomicPromise
+    return @_promiseQueue.add(=> @_atomically(fn))
 
   _atomically: (fn) ->
     @_query("BEGIN EXCLUSIVE TRANSACTION")
     .then =>
       fn()
-    .then (val) =>
+    .finally (val) => @_query("COMMIT")
-      @_query("COMMIT").then =>
+    # NOTE: The value that `fn` resolves to is propagated all the way back to
-        Promise.resolve(val)
+    # the originally caller of `atomically`
 
   ########################################################################
   ########################### PRIVATE METHODS ############################