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

test: add test-domain-exit-dispose-again back #4256

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 68 additions & 31 deletions test/parallel/test-domain-exit-dispose-again.js
Original file line number Diff line number Diff line change
@@ -1,39 +1,76 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const domain = require('domain');

// Use the same timeout value so that both timers' callbacks are called during
// the same invocation of the underlying native timer's callback (listOnTimeout
// in lib/timers.js).
setTimeout(err, 50);
setTimeout(common.mustCall(secondTimer), 50);
// This test makes sure that when a domain is disposed, timers that are
// attached to that domain are not fired, but timers that are _not_ attached
// to that domain, including those whose callbacks are called from within
// the same invocation of listOnTimeout, _are_ called.

function err() {
var common = require('../common');
var assert = require('assert');
var domain = require('domain');
var disposalFailed = false;

// Repeatedly schedule a timer with a delay different than the timers attached
// to a domain that will eventually be disposed to make sure that they are
// called, regardless of what happens with those timers attached to domains
// that will eventually be disposed.
var a = 0;
log();
function log() {
console.log(a++, process.domain);
if (a < 10) setTimeout(log, 20);
}

var secondTimerRan = false;

// Use the same timeout duration for both "firstTimer" and "secondTimer"
// callbacks so that they are called during the same invocation of the
// underlying native timer's callback (listOnTimeout in lib/timers.js).
const TIMEOUT_DURATION = 50;

setTimeout(function firstTimer() {
const d = domain.create();
d.on('error', handleDomainError);
d.run(err2);

function err2() {
// this function doesn't exist, and throws an error as a result.
d.on('error', function handleError(err) {
// Dispose the domain on purpose, so that we can test that nestedTimer
// is not called since it's associated to this domain and a timer whose
// domain is diposed should not run.
d.dispose();
console.error(err);
console.error('in domain error handler',
process.domain, process.domain === d);
});

d.run(function() {
// Create another nested timer that is by definition associated to the
// domain "d". Because an error is thrown before the timer's callback
// is called, and because the domain's error handler disposes the domain,
// this timer's callback should never run.
setTimeout(function nestedTimer() {
console.error('Nested timer should not run, because it is attached to ' +
'a domain that should be disposed.');
disposalFailed = true;
process.exit(1);
});

// Make V8 throw an unreferenced error. As a result, the domain's error
// handler is called, which disposes the domain "d" and should prevent the
// nested timer that is attached to it from running.
err3();
}
});
}, TIMEOUT_DURATION);

function handleDomainError(e) {
// In the domain's error handler, the current active domain should be the
// domain within which the error was thrown.
assert.equal(process.domain, d);
}
}
// This timer expires in the same invocation of listOnTimeout than firstTimer,
// but because it's not attached to any domain, it must run regardless of
// domain "d" being disposed.
setTimeout(function secondTimer() {
console.log('In second timer');
secondTimerRan = true;
}, TIMEOUT_DURATION);

function secondTimer() {
// secondTimer was scheduled before any domain had been created, so its
// callback should not have any active domain set when it runs.
// Do not use assert here, as it throws errors and if a domain with an error
// handler is active, then asserting wouldn't make the test fail.
if (process.domain !== null) {
console.log('process.domain should be null, but instead is:',
process.domain);
process.exit(1);
}
}
process.on('exit', function() {
assert.equal(a, 10);
assert.equal(disposalFailed, false);
assert(secondTimerRan);
console.log('ok');
});
45 changes: 45 additions & 0 deletions test/parallel/test-timers-reset-process-domain-on-throw.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

// This test makes sure that when throwing from within a timer's callback,
// its active domain at the time of the throw is not the process' active domain
// for the next timers that need to be processed on the same turn of the event
// loop.

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

// Use the same timeout value so that both timers' callbacks are called during
// the same invocation of the underlying native timer's callback (listOnTimeout
// in lib/timers.js).
setTimeout(err, 50);
setTimeout(common.mustCall(secondTimer), 50);

function err() {
const d = domain.create();
d.on('error', handleDomainError);
d.run(err2);

function err2() {
// this function doesn't exist, and throws an error as a result.
err3();
}

function handleDomainError(e) {
// In the domain's error handler, the current active domain should be the
// domain within which the error was thrown.
assert.equal(process.domain, d);
}
}

function secondTimer() {
// secondTimer was scheduled before any domain had been created, so its
// callback should not have any active domain set when it runs.
// Do not use assert here, as it throws errors and if a domain with an error
// handler is active, then asserting wouldn't make the test fail.
if (process.domain !== null) {
console.log('process.domain should be null, but instead is:',
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it would be more clear as 'process.domain should be null on timer'?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by "on timer"? process.domain is a process-wide variable, so specifying "on timer" doesn't necessarily make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

"inside timer callback"?

Copy link
Author

Choose a reason for hiding this comment

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

It should be null in that specific timer's callback, not in every timer's callbacks. I can rephrase this as process.domain should be null in this timer callabck, but instead id: if that helps.

process.domain);
process.exit(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we avoid using process.exit() in tests because it is rather immediate, is there a way this could be switched to Throw?

Copy link
Author

Choose a reason for hiding this comment

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

The problem is explained by the comment above:

// Do not use assert here, as it throws errors and if a domain with an error
// handler is active, then asserting wouldn't make the test fail.

maybe this comment should be moved right above the call to process.exit(1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, probably. And of course, domains, oopsie.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll move it closer to the call to process.exit.

}
}