From be39f304472b97ea02525778afc74eab4f0284b4 Mon Sep 17 00:00:00 2001 From: Julien Gilli Date: Mon, 14 Dec 2015 12:11:12 -0800 Subject: [PATCH] test: add test-domain-exit-dispose-again back d1ba82af1c2528c71e1b6b6a57844a7519b66ccb "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 preserves the changes made by d1ba82af1c2528c71e1b6b6a57844a7519b66ccb, but it moves them to a new test file named test-timers-reset-process-domain-on-throw.js. PR: #4278 PR-URL: https://github.com/nodejs/node/pull/4278 Reviewed-By: James M Snell --- test/simple/test-domain-exit-dispose-again.js | 91 +++++++++++++------ ...st-timers-reset-process-domain-on-throw.js | 59 ++++++++++++ 2 files changed, 121 insertions(+), 29 deletions(-) create mode 100644 test/simple/test-timers-reset-process-domain-on-throw.js diff --git a/test/simple/test-domain-exit-dispose-again.js b/test/simple/test-domain-exit-dispose-again.js index fb58a673726bc0..63e021e698deb9 100644 --- a/test/simple/test-domain-exit-dispose-again.js +++ b/test/simple/test-domain-exit-dispose-again.js @@ -19,41 +19,74 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -var common = require('../common'); +// 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. + +var common = require('../common.js'); 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); +} -// 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); +var secondTimerRan = false; -function err() { +// 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). +var TIMEOUT_DURATION = 50; + +setTimeout(function firstTimer() { var d = domain.create(); - d.on('error', handleDomainError); - d.run(err2); + 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 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); + }); - function err2() { - // this function doesn't exist, and throws an error as a result. + // 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/simple/test-timers-reset-process-domain-on-throw.js b/test/simple/test-timers-reset-process-domain-on-throw.js new file mode 100644 index 00000000000000..fb58a673726bc0 --- /dev/null +++ b/test/simple/test-timers-reset-process-domain-on-throw.js @@ -0,0 +1,59 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); +var 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() { + var 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); + } +}