Fix for memory leak caused by referenced to deferred test functions #2037

bd82 · 2016-01-03T14:33:47Z

(it/before[Each]/after[Each]).

related to #1991

danielstjules · 2016-01-03T18:19:12Z

test/integration/memory.leak.js

+      // in node.js versions 0.11 and 0.12 it seems that large array allocation is very inefficient
+      // thus causing the respective Travis-CI jobs to run much longer and causing this test
+      // to sometimes fail due to timeout.
+      this.timeout(1000);


this.timeout won't actually timeout the test after 1 second, since MemoryLeak's for loop is a blocking operation, and mocha just uses setTimeout:

mocha/lib/runnable.js

Lines 211 to 217 in d811eb9

this.timer = setTimeout(function() {

if (!self._enableTimeouts) {

return;

}

self.callback(new Error('timeout of ' + ms + 'ms exceeded. Ensure the done() callback is being called in this test.'));

self.timedOut = true;

}, ms);

This example might help clarify:

$ cat example.js setTimeout(function() { console.log(new Date(), 'Timeout!'); }, 1000); console.log(new Date()); var array = []; for (var i = 0; i < 20000000; i++) { array.push({val: i}); } // $ time node example.js // Sun Jan 03 2016 13:31:21 GMT-0500 (EST) // Sun Jan 03 2016 13:31:27 GMT-0500 (EST) 'Timeout!' // // real 0m6.837s // user 0m6.444s // sys 0m0.425s

Thanks for the clarification.
The weaker condition of waiting at least 1000ms is good enough in this use case.
The timeout is there because on node.js versions 0.11 and 0.12 on Travis-CI
sometimes take slightly longer than 200ms. (the default timeout) and thus the change
failed there.

see

https://travis-ci.org/mochajs/mocha/builds/99955445

https://travis-ci.org/mochajs/mocha/builds/99954248

Ah, my mistake! Makes sense :)

danielstjules · 2016-01-03T18:24:18Z

Thanks for the PR! Going over it now!

Could you move test/integration/memory.leak.js to test/integration/fixtures/memory.leak.js? Integration test fixtures are ran by separate tests and have their output verified. Running the file directly using make test-integration results in quite a bit of noise. If ran as a fixture, you can simply check for a 0 exit code, and the lack of "Allocation failed" in the output :)

danielstjules · 2016-01-03T18:26:21Z

lib/runner.js

+ * Cleans up the references to all the deferred functions (before/after/beforeEach/afterEach)
+ * of a Suite. These must be deleted otherwise it may create a memory leak as those functions may
+ * reference variables from closures, thus those variables can never be garbage collected as long
+ * as the deferred functions exist.


The file appears to mostly follow a 80 char ruler - could you update this doc comment?

danielstjules · 2016-01-03T18:59:09Z

Actually, rather than an integration test (since it takes 40s+ to run on my laptop) could you add a unit test that verified runnable fns were deleted from a suite? It could be added to test/runner.js I'm imagining something like

it('deletes runnable fn when no longer needed', function(done) {
  var test = new Test('test', function() {});
  suite.addTest(test);
  suite.before(function() {});
  suite.beforeEach(function() {});
  suite.afterEach(function() {});
  suite.after(function() {});

  var runner = new Runner(suite);
  runner.runTests(function(err) {
    // perform assertions here
    done();
  });
});

bd82 · 2016-01-03T20:31:33Z

Actually, rather than an integration test (since it takes 40s+ to run on my laptop) could you add a unit test that verified runnable fns were deleted from a suite? It could be added to test/runner.js I'm imagining something like

From which century is that laptop? :) anyhow I see your point as the Travis CI containers are also
fairly weak.

The problem is that verification of simple property deletion is a much weaker check as it will not detect
new memory leaks created by changes that copy those references somewhere else.

I think I can do something simple that scans the object graph starting from the Suite and detects
properties that meet the conditions:

are of Function type
toString() on them contains the the name of the variable ("closureVar")

That check will be more dynamic (no fixed property names) and better detect regressions in the future.

danielstjules · 2016-01-03T21:13:13Z

From which century is that laptop? :) anyhow I see your point as the Travis CI containers are also
fairly weak.

haha that's fair, the 40s is with node v0.12. With Node v4, I'm seeing 8s.

The problem is that verification of simple property deletion is a much weaker check as it will not detect new memory leaks created by changes that copy those references somewhere else.

A regression that introduced a smaller memory leak might still go undetected. But your integration test does a good job of catching most cases - e.g. beforeEach fn's magically not being deleted. Others, I'm less worried about. Some reporters (markdown, doc, html) perform simple operations on test.fn. One could think of contrived examples in which they're copied by accident, causing a memory leak unique to a given reporter. Though I'm sure neither of us would advocate running the integration test for each reporter to achieve that kind of branch coverage.

You make a good point, and I'm happy with the existing integration test if it could be moved under fixtures, as mentioned here: #2037 (comment) A single spec could then be added to test/integration/regression.js I'd just rather not spam the test suite output with 300 specs if it can be avoided. :)

bd82 · 2016-01-03T22:17:25Z

Hi @danielstjules.

I've replaced the test that reproduced the memory leak, but was slow and created a lot of noise in the test report.

The new one is:

very fast
will detect even 'minor' memory leaks without needing to crash V8.

I've kept it in integration tests as:

The report is small and meaningful (prints path to undeleted 'runnable fn').
It was written as a user Test, without programmatically creating the suite / runner (directly).

What do you think?

danielstjules · 2016-01-03T23:37:50Z

Looking at your latest commit, imagine the following diff:

diff --git a/lib/runnable.js b/lib/runnable.js
index 5e1ab76..70ec96d 100644
--- a/lib/runnable.js
+++ b/lib/runnable.js
@@ -42,9 +42,11 @@ module.exports = Runnable;
  * @param {string} title
  * @param {Function} fn
  */
+var fns = [];
 function Runnable(title, fn) {
   this.title = title;
   this.fn = fn;
+  fns.push(fn);
   this.async = fn && fn.length;
   this.sync = !this.async;
   this._timeout = 2000;

Running ./bin/mocha test/integration/memory.leak.js on your current branch would result in all tests passing - it would not detect the leak. Your previous integration test, however, correctly identifies the leak.

Would you mind reverting and using the previous integration test, along with my suggestion of placing it in fixtures? A single spec in test/integration/regression.js, something like:
it('#1991: does not leak memory by keeping reference to runnable fns') would clearly convey the intent, as opposed to:

* deletion of runnable fn to avoid memory leaks via closures
  * inner suite
    * access a variable via a closure

gustiando · 2016-01-04T15:37:50Z

@bd82 this is awesome! thanks a lot for making a PR for this. really excited for this! hopefully it gets pulled in soon :D

bd82 · 2016-01-04T16:12:19Z

@danielstjules:
I agree that it is better to actually reproduce the V8 out of memory crash.
especially because its harder to argue with 'out of memory' vs some strange test failure
after a new change. :)
And my alternative test indeed will not catch references outside the Suite.

I'll revert to the original test and put it in a fixture.

np @gumatias, it was an interesting bug giving a chance to learn some memory leak debugging in javascript. :)

danielstjules · 2016-01-04T17:39:45Z

@bd82 Thanks again! Appreciate all your work on this PR :)

(it/before[Each]/after[Each]). related to #1991.

bd82 · 2016-01-05T17:33:09Z

@danielstjules pleasure to help. :)
Also thanks to @shahar-h who helped debug it.

I've updated the PR to use a fixture with clear description/error messages.
And a large timeout as I don't know how slow it will run on a Travis-CI virtual container + node.js 0.12.
Hopefully this PR is now done :)

danielstjules · 2016-01-06T10:26:35Z

Looks good! Reporters that use test.fn still work, so that's perfect. :)

Thanks again!

Fix for memory leak caused by referenced to deferred test functions

danielstjules · 2016-01-26T07:28:38Z

Related issue here: #2066

bd82 mentioned this pull request Jan 3, 2016

Declarations do not get cleaned up unless you set them to null #1991

Closed

danielstjules reviewed Jan 3, 2016
View reviewed changes

Fix for memory leak caused by referenced to deferred test functions

33c825b

(it/before[Each]/after[Each]). related to #1991.

danielstjules added a commit that referenced this pull request Jan 6, 2016

Merge pull request #2037 from bd82/master

63ef07f

Fix for memory leak caused by referenced to deferred test functions

danielstjules merged commit 63ef07f into mochajs:master Jan 6, 2016

danielstjules mentioned this pull request Jan 26, 2016

HTML/DOC reporter regressions as a result of #2037 #2067

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for memory leak caused by referenced to deferred test functions #2037

Fix for memory leak caused by referenced to deferred test functions #2037

bd82 commented Jan 3, 2016

danielstjules Jan 3, 2016

danielstjules Jan 3, 2016

bd82 Jan 3, 2016

danielstjules Jan 3, 2016

danielstjules commented Jan 3, 2016

danielstjules Jan 3, 2016

bd82 Jan 3, 2016

danielstjules commented Jan 3, 2016

bd82 commented Jan 3, 2016

danielstjules commented Jan 3, 2016

bd82 commented Jan 3, 2016

danielstjules commented Jan 3, 2016

gustiando commented Jan 4, 2016

bd82 commented Jan 4, 2016

danielstjules commented Jan 4, 2016

bd82 commented Jan 5, 2016

danielstjules commented Jan 6, 2016

danielstjules commented Jan 26, 2016

	this.timer = setTimeout(function() {
	if (!self._enableTimeouts) {
	return;
	}
	self.callback(new Error('timeout of ' + ms + 'ms exceeded. Ensure the done() callback is being called in this test.'));
	self.timedOut = true;
	}, ms);

Fix for memory leak caused by referenced to deferred test functions #2037

Fix for memory leak caused by referenced to deferred test functions #2037

Conversation

bd82 commented Jan 3, 2016

danielstjules Jan 3, 2016

Choose a reason for hiding this comment

danielstjules Jan 3, 2016

Choose a reason for hiding this comment

bd82 Jan 3, 2016

Choose a reason for hiding this comment

danielstjules Jan 3, 2016

Choose a reason for hiding this comment

danielstjules commented Jan 3, 2016

danielstjules Jan 3, 2016

Choose a reason for hiding this comment

bd82 Jan 3, 2016

Choose a reason for hiding this comment

danielstjules commented Jan 3, 2016

bd82 commented Jan 3, 2016

danielstjules commented Jan 3, 2016

bd82 commented Jan 3, 2016

danielstjules commented Jan 3, 2016

gustiando commented Jan 4, 2016

bd82 commented Jan 4, 2016

danielstjules commented Jan 4, 2016

bd82 commented Jan 5, 2016

danielstjules commented Jan 6, 2016

danielstjules commented Jan 26, 2016