Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure module and test callbacks are released for GC. #1279

Merged
merged 3 commits into from May 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions build/tasks/test-on-node.js
Expand Up @@ -36,6 +36,9 @@ module.exports = function( grunt ) {
grunt.log.writeln( "-----" );
grunt.log.ok( constructMessage( totals ) );

// Refresh the QUnit global to be used in other tests
global.QUnit = requireFresh( "../../dist/qunit" );

done( !error );
} );
} );
Expand Down
10 changes: 10 additions & 0 deletions src/test.js
Expand Up @@ -254,6 +254,10 @@ Test.prototype = {
finish: function() {
config.current = this;

// Release the test callback to ensure that anything referenced has been
// released to be garbage collected.
this.callback = undefined;

if ( this.steps.length ) {
const stepsList = this.steps.join( ", " );
this.pushFailure( "Expected assert.verifySteps() to be called before end of test " +
Expand Down Expand Up @@ -342,6 +346,11 @@ Test.prototype = {
config.current = undefined;

function logSuiteEnd( module ) {

// Reset `module.hooks` to ensure that anything referenced in these hooks
// has been released to be garbage collected.
module.hooks = {};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something in the suiteEnd probably was using hooks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been a doozy digging into. I'm still somewhat uncertain, but it seems that the test-on-node grunt task is leaking tests (not properly waiting for the QUnit.done that is setup in tests/log.js which does a setTimeout inside the done callback).

I've pushed a small cleanup that prevents the failure, but there is definitely some work needed in this area to clean up the leakage between tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that as of this commit, when QUnit is tries to report the "No tests run" error, it is unable to do so now, because:

  			if (module.hooks[handler].length) {
  			                          ^

TypeError: Cannot read property 'length' of undefined
    at processHooks

I ran into this as part of reproducing #1405 locally.


emit( "suiteEnd", module.suiteReport.end( true ) );
runLoggingCallbacks( "moduleDone", {
name: module.name,
Expand All @@ -351,6 +360,7 @@ Test.prototype = {
total: module.stats.all,
runtime: now() - module.stats.started
} );

}
},

Expand Down
10 changes: 10 additions & 0 deletions test/cli/fixtures/expected/tap-outputs.js
Expand Up @@ -169,5 +169,15 @@ ok 1 Single > has a test
# pass 1
# skip 0
# todo 0
# fail 0`,

"node --expose-gc --allow-natives-syntax ../../../bin/qunit memory-leak/*.js":
`TAP version 13
ok 1 some nested module > can call method on foo
ok 2 later thing > has released all foos
1..2
# pass 2
# skip 0
# todo 0
# fail 0`
};
60 changes: 60 additions & 0 deletions test/cli/fixtures/memory-leak/index.js
@@ -0,0 +1,60 @@
/* globals gc */

const foos = new WeakSet();
class Foo {
constructor() {
foos.add( this );
}

sayHello() {
return "Hi!";
}

destroy() { }
}

QUnit.module( "some nested module", function( hooks ) {
let foo;

hooks.beforeEach( function() {
foo = new Foo();
} );

hooks.afterEach( function() {
foo.destroy();
} );

QUnit.test( "can call method on foo", function( assert ) {
assert.equal( foo.sayHello(), "Hi!" );
} );

} );

QUnit.module( "later thing", function() {
QUnit.test( "has released all foos", function( assert ) {

// Without async here, the test runner is recursive, and therefore the `foo`
// instance is still retained.
return Promise.resolve()
.then( () => {

// Requires `--expose-gc` flag to function properly.
gc();

// Using `new Function` here to avoid a syntax error.
const retainedFoos = new Function(
"foos",
"return %GetWeakSetValues(foos, 0)"
)( foos );

assert.equal( retainedFoos.length, 0 );
} )
.catch( error => {
if ( error instanceof SyntaxError ) {
console.log( "Must launch Node 9 with `--expose-gc --allow-natives-syntax`" );
} else {
throw error;
}
} );
} );
} );
12 changes: 12 additions & 0 deletions test/cli/main.js
Expand Up @@ -4,6 +4,7 @@ const co = require( "co" );

const expectedOutput = require( "./fixtures/expected/tap-outputs" );
const execute = require( "./helpers/execute" );
const semver = require( "semver" );

QUnit.module( "CLI Main", function() {
QUnit.test( "defaults to running tests in 'test' directory", co.wrap( function* ( assert ) {
Expand Down Expand Up @@ -123,6 +124,17 @@ QUnit.module( "CLI Main", function() {
}
} ) );

if ( semver.gte( process.versions.node, "9.0.0" ) ) {
QUnit.test( "callbacks and hooks from modules are properly released for garbage collection", co.wrap( function* ( assert ) {
const command = "node --expose-gc --allow-natives-syntax ../../../bin/qunit memory-leak/*.js";
const execution = yield execute( command );

assert.equal( execution.code, 0 );
assert.equal( execution.stderr, "" );
assert.equal( execution.stdout, expectedOutput[ command ] );
} ) );
}

QUnit.module( "filter", function() {
QUnit.test( "can properly filter tests", co.wrap( function* ( assert ) {
const command = "qunit --filter 'single' test single.js 'glob/**/*-test.js'";
Expand Down