From dfcbd5aaf52bdaad9f366b69965572282b6c59db Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Fri, 11 Dec 2015 17:43:04 -0800 Subject: [PATCH 1/2] test: add test-domain-exit-dispose-again back 1c8584997346d549dd5ff4bb787f48f52440a9cb "fixed" test-domain-exit-dispose-again by changing its logic to test that process.domain was cleared properly in case an error was thrown from a timer's callback. However, it became clear when reviewing a recent change that refactors lib/timers.js that it was not quite the intention of the original test. Thus, this change adds the original implementation of test-domain-exit-dispose-again back, with comments that make its implementation easier to understand. It also preserve the changes made by 1c8584997346d549dd5ff4bb787f48f52440a9cb, but it moves them to a new test file named test-timers-reset-process-domain-on-throw.js. --- .../test-domain-exit-dispose-again.js | 99 +++++++++++++------ ...st-timers-reset-process-domain-on-throw.js | 45 +++++++++ 2 files changed, 113 insertions(+), 31 deletions(-) create mode 100644 test/parallel/test-timers-reset-process-domain-on-throw.js diff --git a/test/parallel/test-domain-exit-dispose-again.js b/test/parallel/test-domain-exit-dispose-again.js index 6fe9f25fb0d887..3cae101fd69c39 100644 --- a/test/parallel/test-domain-exit-dispose-again.js +++ b/test/parallel/test-domain-exit-dispose-again.js @@ -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'); +}); diff --git a/test/parallel/test-timers-reset-process-domain-on-throw.js b/test/parallel/test-timers-reset-process-domain-on-throw.js new file mode 100644 index 00000000000000..e6a7215d368cb6 --- /dev/null +++ b/test/parallel/test-timers-reset-process-domain-on-throw.js @@ -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:', + process.domain); + process.exit(1); + } +} From 7bf1f08dfcd4629e3d62d6ede4db57aab43515c5 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Mon, 21 Dec 2015 14:58:43 -0800 Subject: [PATCH 2/2] Changes according to code review --- .../parallel/test-timers-reset-process-domain-on-throw.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-timers-reset-process-domain-on-throw.js b/test/parallel/test-timers-reset-process-domain-on-throw.js index e6a7215d368cb6..f72530b5423886 100644 --- a/test/parallel/test-timers-reset-process-domain-on-throw.js +++ b/test/parallel/test-timers-reset-process-domain-on-throw.js @@ -35,11 +35,11 @@ function err() { 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); + console.log('process.domain should be null in this timer callback, but ' + + 'instead is:', process.domain); + // 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. process.exit(1); } }