diff options
| -rw-r--r-- | lib/domain.js | 60 | ||||
| -rw-r--r-- | test/parallel/test-domain-emit-error-handler-stack.js | 161 | ||||
| -rw-r--r-- | test/parallel/test-domain-thrown-error-handler-stack.js | 46 | ||||
| -rw-r--r-- | test/parallel/test-domain-top-level-error-handler-clears-stack.js | 13 | ||||
| -rw-r--r-- | test/parallel/test-timers-reset-process-domain-on-throw.js | 7 |
5 files changed, 268 insertions, 19 deletions
diff --git a/lib/domain.js b/lib/domain.js index eab0b352bc..b2ffed2741 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -132,7 +132,7 @@ function topLevelDomainCallback(cb, ...args) { // It's possible to enter one domain while already inside // another one. The stack is each entered domain. -const stack = []; +let stack = []; exports._stack = stack; internalBinding('domain').enable(topLevelDomainCallback); @@ -211,6 +211,13 @@ Domain.prototype._errorHandler = function(er) { }); er.domainThrown = true; } + // Pop all adjacent duplicates of the currently active domain from the stack. + // This is done to prevent a domain's error handler to run within the context + // of itself, and re-entering itself recursively handler as a result of an + // exception thrown in its context. + while (exports.active === this) { + this.exit(); + } // The top-level domain-handler is handled separately. // @@ -221,15 +228,15 @@ Domain.prototype._errorHandler = function(er) { // process abort. Using try/catch here would always make V8 think // that these exceptions are caught, and thus would prevent it from // aborting in these cases. - if (stack.length === 1) { + if (stack.length === 0) { // If there's no error handler, do not emit an 'error' event // as this would throw an error, make the process exit, and thus // prevent the process 'uncaughtException' event from being emitted // if a listener is set. if (EventEmitter.listenerCount(this, 'error') > 0) { - // Clear the uncaughtExceptionCaptureCallback so that we know that, even - // if technically the top-level domain is still active, it would - // be ok to abort on an uncaught exception at this point + // Clear the uncaughtExceptionCaptureCallback so that we know that, since + // the top-level domain is not active anymore, it would be ok to abort on + // an uncaught exception at this point setUncaughtExceptionCaptureCallback(null); try { caught = this.emit('error', er); @@ -253,10 +260,6 @@ Domain.prototype._errorHandler = function(er) { // The domain error handler threw! oh no! // See if another domain can catch THIS error, // or else crash on the original one. - // If the user already exited it, then don't double-exit. - if (this === exports.active) { - stack.pop(); - } updateExceptionCapture(); if (stack.length) { exports.active = process.domain = stack[stack.length - 1]; @@ -486,7 +489,46 @@ EventEmitter.prototype.emit = function(...args) { er.domainThrown = false; } + // Remove the current domain (and its duplicates) from the domains stack and + // set the active domain to its parent (if any) so that the domain's error + // handler doesn't run in its own context. This prevents any event emitter + // created or any exception thrown in that error handler from recursively + // executing that error handler. + const origDomainsStack = stack.slice(); + const origActiveDomain = process.domain; + + // Travel the domains stack from top to bottom to find the first domain + // instance that is not a duplicate of the current active domain. + let idx = stack.length - 1; + while (idx > -1 && process.domain === stack[idx]) { + --idx; + } + + // Change the stack to not contain the current active domain, and only the + // domains above it on the stack. + if (idx < 0) { + stack.length = 0; + } else { + stack.splice(idx + 1); + } + + // Change the current active domain + if (stack.length > 0) { + exports.active = process.domain = stack[stack.length - 1]; + } else { + exports.active = process.domain = null; + } + + updateExceptionCapture(); + domain.emit('error', er); + + // Now that the domain's error handler has completed, restore the domains + // stack and the active domain to their original values. + exports._stack = stack = origDomainsStack; + exports.active = process.domain = origActiveDomain; + updateExceptionCapture(); + return false; } diff --git a/test/parallel/test-domain-emit-error-handler-stack.js b/test/parallel/test-domain-emit-error-handler-stack.js new file mode 100644 index 0000000000..d42e470979 --- /dev/null +++ b/test/parallel/test-domain-emit-error-handler-stack.js @@ -0,0 +1,161 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const domain = require('domain'); +const EventEmitter = require('events'); + +/* + * Make sure that the domains stack and the active domain is setup properly when + * a domain's error handler is called due to an error event being emitted. + * More specifically, we want to test that: + * - the active domain in the domain's error handler is *not* that domain, *but* + * the active domain is a any direct parent domain at the time the error was + * emitted. + * - the domains stack in the domain's error handler does *not* include that + * domain, *but* it includes all parents of that domain when the error was + * emitted. + */ +const d1 = domain.create(); +const d2 = domain.create(); +const d3 = domain.create(); + +function checkExpectedDomains(err) { + // First, check that domains stack and active domain is as expected when the + // event handler is called synchronously via ee.emit('error'). + if (domain._stack.length !== err.expectedStackLength) { + console.error('expected domains stack length of %d, but instead is %d', + err.expectedStackLength, domain._stack.length); + process.exit(1); + } + + if (process.domain !== err.expectedActiveDomain) { + console.error('expected active domain to be %j, but instead is %j', + err.expectedActiveDomain, process.domain); + process.exit(1); + } + + // Then make sure that the domains stack and active domain is setup as + // expected when executing a callback scheduled via nextTick from the error + // handler. + process.nextTick(() => { + const expectedStackLengthInNextTickCb = + err.expectedStackLength > 0 ? 1 : 0; + if (domain._stack.length !== expectedStackLengthInNextTickCb) { + console.error('expected stack length in nextTick cb to be %d, ' + + 'but instead is %d', expectedStackLengthInNextTickCb, + domain._stack.length); + process.exit(1); + } + + const expectedActiveDomainInNextTickCb = + expectedStackLengthInNextTickCb === 0 ? undefined : + err.expectedActiveDomain; + if (process.domain !== expectedActiveDomainInNextTickCb) { + console.error('expected active domain in nextTick cb to be %j, ' + + 'but instead is %j', expectedActiveDomainInNextTickCb, + process.domain); + process.exit(1); + } + }); +} + +d1.on('error', common.mustCall((err) => { + checkExpectedDomains(err); +}, 2)); + +d2.on('error', common.mustCall((err) => { + checkExpectedDomains(err); +}, 2)); + +d3.on('error', common.mustCall((err) => { + checkExpectedDomains(err); +}, 1)); + +d1.run(() => { + const ee = new EventEmitter(); + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 1); + + const err = new Error('oops'); + err.expectedStackLength = 0; + err.expectedActiveDomain = null; + ee.emit('error', err); + + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 1); +}); + +d1.run(() => { + d1.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 2); + + const err = new Error('oops'); + err.expectedStackLength = 0; + err.expectedActiveDomain = null; + ee.emit('error', err); + + assert.strictEqual(process.domain, d1); + assert.strictEqual(domain._stack.length, 2); + }); +}); + +d1.run(() => { + d2.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 2); + + const err = new Error('oops'); + err.expectedStackLength = 1; + err.expectedActiveDomain = d1; + ee.emit('error', err); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 2); + }); +}); + +d1.run(() => { + d2.run(() => { + d2.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 3); + + const err = new Error('oops'); + err.expectedStackLength = 1; + err.expectedActiveDomain = d1; + ee.emit('error', err); + + assert.strictEqual(process.domain, d2); + assert.strictEqual(domain._stack.length, 3); + }); + }); +}); + +d3.run(() => { + d1.run(() => { + d3.run(() => { + d3.run(() => { + const ee = new EventEmitter(); + + assert.strictEqual(process.domain, d3); + assert.strictEqual(domain._stack.length, 4); + + const err = new Error('oops'); + err.expectedStackLength = 2; + err.expectedActiveDomain = d1; + ee.emit('error', err); + + assert.strictEqual(process.domain, d3); + assert.strictEqual(domain._stack.length, 4); + }); + }); + }); +}); diff --git a/test/parallel/test-domain-thrown-error-handler-stack.js b/test/parallel/test-domain-thrown-error-handler-stack.js new file mode 100644 index 0000000000..564fd8a5d7 --- /dev/null +++ b/test/parallel/test-domain-thrown-error-handler-stack.js @@ -0,0 +1,46 @@ +'use strict'; + +const common = require('../common'); +const domain = require('domain'); + +/* + * Make sure that when an erorr is thrown from a nested domain, its error + * handler runs outside of that domain, but within the context of any parent + * domain. + */ + +const d = domain.create(); +const d2 = domain.create(); + +d2.on('error', common.mustCall((err) => { + if (domain._stack.length !== 1) { + console.error('domains stack length should be 1 but is %d', + domain._stack.length); + process.exit(1); + } + + if (process.domain !== d) { + console.error('active domain should be %j but is %j', d, process.domain); + process.exit(1); + } + + process.nextTick(() => { + if (domain._stack.length !== 1) { + console.error('domains stack length should be 1 but is %d', + domain._stack.length); + process.exit(1); + } + + if (process.domain !== d) { + console.error('active domain should be %j but is %j', d, + process.domain); + process.exit(1); + } + }); +})); + +d.run(() => { + d2.run(() => { + throw new Error('oops'); + }); +}); diff --git a/test/parallel/test-domain-top-level-error-handler-clears-stack.js b/test/parallel/test-domain-top-level-error-handler-clears-stack.js index 05d5fca467..9dcf038abf 100644 --- a/test/parallel/test-domain-top-level-error-handler-clears-stack.js +++ b/test/parallel/test-domain-top-level-error-handler-clears-stack.js @@ -10,19 +10,18 @@ const domain = require('domain'); const d = domain.create(); d.on('error', common.mustCall(() => { + // Scheduling a callback with process.nextTick _could_ enter a _new_ domain, + // but domain's error handlers are called outside of their domain's context. + // So there should _no_ domain on the domains stack if the domains stack was + // cleared properly when the domain error handler was called. process.nextTick(() => { - // Scheduling a callback with process.nextTick will enter a _new_ domain, - // and the callback will be called after the domain that handled the error - // was exited. So there should be only one domain on the domains stack if - // the domains stack was cleared properly when the domain error handler - // returned. - if (domain._stack.length !== 1) { + if (domain._stack.length !== 0) { // Do not use assert to perform this test: this callback runs in a // different callstack as the original process._fatalException that // handled the original error, thus throwing here would trigger another // call to process._fatalException, and so on recursively and // indefinitely. - console.error('domains stack length should be 1, but instead is:', + console.error('domains stack length should be 0, but instead is:', domain._stack.length); process.exit(1); } 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 55fd43feb8..a0de01f092 100644 --- a/test/parallel/test-timers-reset-process-domain-on-throw.js +++ b/test/parallel/test-timers-reset-process-domain-on-throw.js @@ -26,9 +26,10 @@ function err() { } function handleDomainError(e) { - // In the domain's error handler, the current active domain should be the - // domain within which the error was thrown. - assert.strictEqual(process.domain, d); + assert.strictEqual(e.domain, d); + // Domains' error handlers are called outside of their domain's context, so + // we're not expecting any active domain here. + assert.strictEqual(process.domain, undefined); } } |