Fix memory being retained until promise queue is completely empty (#1544

) Fix memory being retained until promise queue is completely empty (#1544)
petkaantonov · Sep 3, 2018 · f290da0 · f290da0
1 parent ad6d763
commit f290da0
Show file tree

Hide file tree

Showing 6 changed files with 116 additions and 11 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -14,7 +14,7 @@ matrix:
 git:
   depth: 5
 env:
-  - "NODE_FLAGS='' SCRIPT_FLAGS=''"
+  - "NODE_FLAGS='--expose-gc' SCRIPT_FLAGS=''"
 before_script:
 - git submodule update --init --recursive
 script: "node $NODE_FLAGS tools/test.js $SCRIPT_FLAGS"

diff --git a/package.json b/package.json
@@ -19,7 +19,7 @@
   ],
   "scripts": {
     "lint": "node scripts/jshint.js",
-    "test": "node tools/test.js",
+    "test": "node --expose-gc tools/test.js",
     "istanbul": "istanbul",
     "prepublish": "npm run generate-browser-core && npm run generate-browser-full",
     "generate-browser-full": "node tools/build.js --no-clean --no-debug --release --browser --minify",

diff --git a/src/async.js b/src/async.js
@@ -132,25 +132,31 @@ if (!util.hasDevTools) {
     };
 }
 
-Async.prototype._drainQueue = function(queue) {
+function _drainQueue(queue) {
     while (queue.length() > 0) {
-        var fn = queue.shift();
-        if (typeof fn !== "function") {
-            fn._settlePromises();
-            continue;
-        }
+        _drainQueueStep(queue);
+    }
+}
+
+// Shift the queue in a separate function to allow
+// garbage collection after each step
+function _drainQueueStep(queue) {
+    var fn = queue.shift();
+    if (typeof fn !== "function") {
+        fn._settlePromises();
+    } else {
         var receiver = queue.shift();
         var arg = queue.shift();
         fn.call(receiver, arg);
     }
-};
+}
 
 Async.prototype._drainQueues = function () {
     ASSERT(this._isTickUsed);
-    this._drainQueue(this._normalQueue);
+    _drainQueue(this._normalQueue);
     this._reset();
     this._haveDrainedQueues = true;
-    this._drainQueue(this._lateQueue);
+    _drainQueue(this._lateQueue);
 };
 
 Async.prototype._queueTick = function () {

diff --git a/src/debuggability.js b/src/debuggability.js
@@ -127,19 +127,22 @@ Promise.longStackTraces = function () {
     if (!config.longStackTraces && longStackTracesIsSupported()) {
         var Promise_captureStackTrace = Promise.prototype._captureStackTrace;
         var Promise_attachExtraTrace = Promise.prototype._attachExtraTrace;
+        var Promise_dereferenceTrace = Promise.prototype._dereferenceTrace;
         config.longStackTraces = true;
         disableLongStackTraces = function() {
             if (async.haveItemsQueued() && !config.longStackTraces) {
                 throw new Error(LONG_STACK_TRACES_ERROR);
             }
             Promise.prototype._captureStackTrace = Promise_captureStackTrace;
             Promise.prototype._attachExtraTrace = Promise_attachExtraTrace;
+            Promise.prototype._dereferenceTrace = Promise_dereferenceTrace;
             Context.deactivateLongStackTraces();
             async.enableTrampoline();
             config.longStackTraces = false;
         };
         Promise.prototype._captureStackTrace = longStackTracesCaptureStackTrace;
         Promise.prototype._attachExtraTrace = longStackTracesAttachExtraTrace;
+        Promise.prototype._dereferenceTrace = longStackTracesDereferenceTrace;
         Context.activateLongStackTraces();
         async.disableTrampolineIfNecessary();
     }
@@ -325,6 +328,7 @@ Promise.prototype._attachCancellationCallback = function(onCancel) {
 };
 Promise.prototype._captureStackTrace = function () {};
 Promise.prototype._attachExtraTrace = function () {};
+Promise.prototype._dereferenceTrace = function () {};
 Promise.prototype._clearCancellationData = function() {};
 Promise.prototype._propagateFrom = function (parent, flags) {
     USE(parent);
@@ -435,6 +439,10 @@ function longStackTracesAttachExtraTrace(error, ignoreSelf) {
     }
 }
 
+function longStackTracesDereferenceTrace() {
+    this._trace = undefined;
+}
+
 function checkForgottenReturns(returnValue, promiseCreated, name, promise,
                                parent) {
     if (returnValue === undefined && promiseCreated !== null &&

diff --git a/src/promise.js b/src/promise.js
@@ -681,6 +681,7 @@ Promise.prototype._fulfill = function (value) {
         } else {
             async.settlePromises(this);
         }
+        this._dereferenceTrace();
     }
 };
 

diff --git a/test/mocha/async.js b/test/mocha/async.js
@@ -152,4 +152,94 @@ describe("Async requirement", function() {
             });
         });
     }
+
+    if (testUtils.isRecentNode) {
+        describe("Frees memory of old values in promise chains", function () {
+            function getHeapUsed() {
+                global.gc();
+                return process.memoryUsage().heapUsed;
+            }
+
+            var initialHeapUsed;
+
+            before(function () {
+                if (typeof global.gc !== "function") {
+                    throw new Error("These tests require the --expose-gc flag");
+                }
+                initialHeapUsed = getHeapUsed();
+            });
+
+            specify(".then", function () {
+                return Promise.resolve()
+                    .then(function () {
+                        assert.ok(
+                            getHeapUsed() < initialHeapUsed * 1.1,
+                            "Promise.resolve uses minimal memory"
+                        );
+                        var rows = [];
+                        for (var i = 0; i < 1e6; i++) {
+                            rows.push(["Example " + i, i, i * 2]);
+                        }
+                        return rows;
+                    })
+                    .then(function (rows) {
+                        assert.ok(
+                            getHeapUsed() > initialHeapUsed * 12,
+                            "large array uses a large amount of memory"
+                        );
+                        return { len: rows.length };
+                    })
+                    .then(function (x) {
+                        // work around cancellation retaining previous result
+                        return x;
+                    })
+                    .then(function (summaryResult) {
+                        assert.ok(
+                            getHeapUsed() < initialHeapUsed * 1.1,
+                            "memory used by large array is freed"
+                        );
+                        assert.strictEqual(summaryResult.len, 1e6, "result");
+                    });
+            });
+
+            specify(".catch", function () {
+                return Promise.reject(new Error("error 1"))
+                    .catch(function () {
+                        assert.ok(
+                            getHeapUsed() < initialHeapUsed * 1.1,
+                            "Promise.reject uses minimal memory"
+                        );
+                        var rows = [];
+                        for (var i = 0; i < 1e6; i++) {
+                            rows.push(["Example " + i, i, i * 2]);
+                        }
+                        var error = new Error("error 2");
+                        error.result = rows;
+                        throw error;
+                    })
+                    .catch(function (err) {
+                        assert.ok(
+                            getHeapUsed() > initialHeapUsed * 12,
+                            "large array uses a large amount of memory"
+                        );
+                        var rows = err.result;
+                        var error = new Error("error 3");
+                        error.result = { len: rows.length };
+                        throw error;
+                    })
+                    .catch(function (err) {
+                        // work around cancellation retaining previous result
+                        throw err;
+                    })
+                    .catch(function (err) {
+                        assert.ok(
+                            getHeapUsed() < initialHeapUsed * 1.1,
+                            "memory used by large array is freed"
+                        );
+                        var summaryResult = err.result;
+                        assert.strictEqual(summaryResult.len, 1e6, "result");
+                    });
+            });
+        });
+    }
 });