From e35adb660cc45a9e38dfba5920b525f81ba12827 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 Aug 2018 13:11:45 -0400 Subject: [PATCH 1/3] Clean up memory correctly This makes sure that we clean up the DOM inside of the zone that was used to render the app. This ensure that callbacks made by `can-dom-mutate` reference the correct document, and all of the necessary memory is cleaned up. --- package.json | 4 +-- test/leak_test.js | 44 +++++++++++++++++++++++-------- test/tests/async/orders/orders.js | 3 ++- zones/canjs/cleanup.js | 12 ++++----- zones/canjs/globals.js | 23 +++++++++------- 5 files changed, 56 insertions(+), 30 deletions(-) diff --git a/package.json b/package.json index 97c024d..31d4876 100644 --- a/package.json +++ b/package.json @@ -56,8 +56,8 @@ "uglify-js": "^3.3.19" }, "dependencies": { - "can-dom-data-state": "^1.0.1", - "can-dom-mutate": "^1.0.2", + "can-dom-data-state": "^1.0.2", + "can-dom-mutate": "^1.2.1", "can-namespace": "^1.0.0", "can-reflect": "^1.11.0", "can-util": "^3.10.10", diff --git a/test/leak_test.js b/test/leak_test.js index c02d04c..8097dce 100644 --- a/test/leak_test.js +++ b/test/leak_test.js @@ -9,9 +9,23 @@ describe("Memory leaks", function(){ this.timeout(30000); before(function(done){ - this.oldXHR = global.XMLHttpRequest; - global.XMLHttpRequest = helpers.mockXHR( - '[ { "a": "a" }, { "b": "b" } ]'); + helpers.createServer(8070, function(req, res){ + switch(req.url) { + case "/bar": + var data = [ { "a": "a" }, { "b": "b" } ]; + break; + default: + throw new Error("No route for " + req.url); + } + res.setHeader("Content-Type", "application/json"); + res.end(JSON.stringify(data)); + }) + .then(server => { + this.server = server; + + // Render once so that everything is loaded + this.render("/").then(_ => done()); + }); var render = ssr({ config: "file:" + path.join(__dirname, "tests", "package.json!npm"), @@ -21,25 +35,33 @@ describe("Memory leaks", function(){ this.render = function(pth){ return new Promise(function(resolve, reject){ var stream = through(function(buffer){ - setTimeout(resolve, 300); + setTimeout(() => resolve(buffer), 300); }); stream.on("error", reject); render(pth).pipe(stream); }); }; - - // Render once so that everything is loaded - this.render("/").then(_ => done()); }); after(function(){ - global.XMLHttpRequest = this.oldXHR; + this.server.close(); }); - it("do not happen", function(done){ - + it("No leaks occur after 10 iterations", function(done){ + var debug = typeof process.env.DONE_SSR_DEBUG !== "undefined"; + var cnt = 0; iterate(10, () => { - return this.render("/"); + cnt++; + var thisIteration = cnt; + if(debug) { + console.error("Before render", thisIteration); + } + + return this.render("/").then(() => { + if(debug) { + console.error("After render", thisIteration); + } + }); }) .then(() => { done(); diff --git a/test/tests/async/orders/orders.js b/test/tests/async/orders/orders.js index fbc70d6..733d948 100644 --- a/test/tests/async/orders/orders.js +++ b/test/tests/async/orders/orders.js @@ -14,7 +14,8 @@ var ViewModel = DefineMap.extend({ resolve(data); }; xhr.onerror = function(err){ - console.error(err); + var error = err || new Error(this.responseText); + console.error(error); }; xhr.send(); }); diff --git a/zones/canjs/cleanup.js b/zones/canjs/cleanup.js index 87dccb8..95e486f 100644 --- a/zones/canjs/cleanup.js +++ b/zones/canjs/cleanup.js @@ -12,15 +12,15 @@ module.exports = function(data){ var domMutate = getDomMutate(); var docEl = data.document.documentElement; - var head = data.document.head; - var body = data.document.body; + + // Run the removal within the zone so that the globals point to our globals. + var removeDocumentElement = this.wrap(function() { + domMutate.removeChild.call(data.document, docEl); + }); // Do this some time later to prevent extra mutations // In the mutation stream - later(function(){ - domMutate.removeChild.call(docEl, head); - domMutate.removeChild.call(docEl, body); - }); + later(removeDocumentElement); } }; }; diff --git a/zones/canjs/globals.js b/zones/canjs/globals.js index d45303f..423539a 100644 --- a/zones/canjs/globals.js +++ b/zones/canjs/globals.js @@ -7,21 +7,24 @@ module.exports = function(data){ } var getLocation = getEither.bind(null, "LOCATION", "can-globals/location/location"); - var getDocument = getEither.bind(null, "DOCUMENT", "can-globals/document/document") + var getDocument = getEither.bind(null, "DOCUMENT", "can-globals/document/document"); var oldLocation, oldDocument; - return { - beforeTask: function(){ - var LOCATION = getLocation(); - var DOCUMENT = getDocument(); + function setCanGlobals() { + var LOCATION = getLocation(); + var DOCUMENT = getDocument(); + + oldLocation = LOCATION(); + LOCATION(data.window.location); - oldLocation = LOCATION(); - LOCATION(window.location); + oldDocument = DOCUMENT(); + DOCUMENT(data.window.document); + } - oldDocument = DOCUMENT(); - DOCUMENT(window.document); - }, + return { + afterStealDone: setCanGlobals, + beforeTask: setCanGlobals, afterTask: function(){ getLocation()(oldLocation); getDocument()(oldDocument); From 55931d2db1c68d99b5d5d58d37e2a0fbb40f1aa0 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Wed, 6 Jun 2018 10:53:43 -0400 Subject: [PATCH 2/3] Upgrade leakage --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 31d4876..a93dd1b 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,7 @@ "he": "^1.1.1", "jquery": "2.x - 3.x", "jshint": "^2.8.0", - "leakage": "0.3.0-beta", + "leakage": "0.4.0", "nock": "^9.0.11", "spawn-mochas": "^1.1.0", "steal-stache": "^4.0.0", From c4d9e6ac2a9ca3eaa17e9482a6ca9214cbfb14df Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 16 Aug 2018 13:33:19 -0400 Subject: [PATCH 3/3] Correct new linting errors --- test/async_test.js | 3 ++- test/auth-cookie-failed-domain_test.js | 3 ++- test/auth-cookie_test.js | 3 ++- test/incremental_prog_test.js | 3 ++- test/incremental_test.js | 3 ++- test/leak_test.js | 3 ++- test/xhr_test.js | 3 ++- 7 files changed, 14 insertions(+), 7 deletions(-) diff --git a/test/async_test.js b/test/async_test.js index 91dccba..001b2c1 100644 --- a/test/async_test.js +++ b/test/async_test.js @@ -14,9 +14,10 @@ describe("async rendering", function(){ }); helpers.createServer(8070, function(req, res){ + var data; switch(req.url) { case "/bar": - var data = [ { "a": "a" }, { "b": "b" } ]; + data = [ { "a": "a" }, { "b": "b" } ]; break; default: throw new Error("No route for " + req.url); diff --git a/test/auth-cookie-failed-domain_test.js b/test/auth-cookie-failed-domain_test.js index db0be16..c0f4dbd 100644 --- a/test/auth-cookie-failed-domain_test.js +++ b/test/auth-cookie-failed-domain_test.js @@ -24,10 +24,11 @@ describe("auth cookies - failed domain", function() { }); helpers.createServer(8070, function(req, res){ + var data; authHeader = req.headers.authorization; switch(req.url) { case "/api/list": - var data = [1,2,3,4,5]; + data = [1,2,3,4,5]; break; default: throw new Error("No route for " + req.url); diff --git a/test/auth-cookie_test.js b/test/auth-cookie_test.js index a78a867..f93f0ff 100644 --- a/test/auth-cookie_test.js +++ b/test/auth-cookie_test.js @@ -25,10 +25,11 @@ describe("auth cookies", function() { }); helpers.createServer(8070, function(req, res){ + var data; authHeader = req.headers.authorization; switch(req.url) { case "/api/list": - var data = [1,2,3,4,5]; + data = [1,2,3,4,5]; break; default: throw new Error("No route for " + req.url); diff --git a/test/incremental_prog_test.js b/test/incremental_prog_test.js index 5424b8b..a82a6cd 100644 --- a/test/incremental_prog_test.js +++ b/test/incremental_prog_test.js @@ -9,9 +9,10 @@ describe("Incremental rendering", function(){ before(function(done){ helpers.createServer(8070, function(req, res){ + var data; switch(req.url) { case "/bar": - var data = [ { "a": "a" }, { "b": "b" } ]; + data = [ { "a": "a" }, { "b": "b" } ]; break; default: throw new Error("No route for " + req.url); diff --git a/test/incremental_test.js b/test/incremental_test.js index 8e7b5f9..4209aea 100644 --- a/test/incremental_test.js +++ b/test/incremental_test.js @@ -12,9 +12,10 @@ describe("Incremental rendering", function(){ before(function(done){ helpers.createServer(8070, function(req, res){ + var data; switch(req.url) { case "/bar": - var data = [ { "a": "a" }, { "b": "b" } ]; + data = [ { "a": "a" }, { "b": "b" } ]; break; default: throw new Error("No route for " + req.url); diff --git a/test/leak_test.js b/test/leak_test.js index 8097dce..b8cdb6a 100644 --- a/test/leak_test.js +++ b/test/leak_test.js @@ -10,9 +10,10 @@ describe("Memory leaks", function(){ before(function(done){ helpers.createServer(8070, function(req, res){ + var data; switch(req.url) { case "/bar": - var data = [ { "a": "a" }, { "b": "b" } ]; + data = [ { "a": "a" }, { "b": "b" } ]; break; default: throw new Error("No route for " + req.url); diff --git a/test/xhr_test.js b/test/xhr_test.js index 1d9c965..f50d456 100644 --- a/test/xhr_test.js +++ b/test/xhr_test.js @@ -16,9 +16,10 @@ describe("xhr async rendering", function() { }); helpers.createServer(8070, function(req, res){ + var data; switch(req.url) { case "/api/list": - var data = [1,2,3,4,5]; + data = [1,2,3,4,5]; break; default: throw new Error("No route for " + req.url);