FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

domain: error handler runs outside of its domain · nodejs/node@43a5170 · GitHub

/ node Public

Commit 43a5170

Browse files
Julien Gilli
authored and
Trott
committed
domain: error handler runs outside of its domain
Before this change, domains' error handlers would run with the corresponding domain as the active domain. This creates the possibility for domains' error handlers to call themselves recursively if an event emitter created in the error handler emits an error, or if the error handler throws an error. This change sets the active domain to be the domain's parent (or null if the domain for which the error handler is called has no parent) to prevent that from happening. Fixes: #26086 PR-URL: #26211 Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent d74dc17 commit 43a5170

5 files changed

Lines changed: 268 additions & 19 deletions

‎lib/domain.js‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ function topLevelDomainCallback(cb, ...args) {
132132

133133
// It's possible to enter one domain while already inside
134134
// another one. The stack is each entered domain.
135-
const stack = [];
135+
let stack = [];
136136
exports._stack = stack;
137137
internalBinding('domain').enable(topLevelDomainCallback);
138138

@@ -211,6 +211,13 @@ Domain.prototype._errorHandler = function(er) {
211211
});
212212
er.domainThrown = true;
213213
}
214+
// Pop all adjacent duplicates of the currently active domain from the stack.
215+
// This is done to prevent a domain's error handler to run within the context
216+
// of itself, and re-entering itself recursively handler as a result of an
217+
// exception thrown in its context.
218+
while (exports.active === this) {
219+
this.exit();
220+
}
214221

215222
// The top-level domain-handler is handled separately.
216223
//
@@ -221,15 +228,15 @@ Domain.prototype._errorHandler = function(er) {
221228
// process abort. Using try/catch here would always make V8 think
222229
// that these exceptions are caught, and thus would prevent it from
223230
// aborting in these cases.
224-
if (stack.length === 1) {
231+
if (stack.length === 0) {
225232
// If there's no error handler, do not emit an 'error' event
226233
// as this would throw an error, make the process exit, and thus
227234
// prevent the process 'uncaughtException' event from being emitted
228235
// if a listener is set.
229236
if (EventEmitter.listenerCount(this, 'error') > 0) {
230-
// Clear the uncaughtExceptionCaptureCallback so that we know that, even
231-
// if technically the top-level domain is still active, it would
232-
// be ok to abort on an uncaught exception at this point
237+
// Clear the uncaughtExceptionCaptureCallback so that we know that, since
238+
// the top-level domain is not active anymore, it would be ok to abort on
239+
// an uncaught exception at this point
233240
setUncaughtExceptionCaptureCallback(null);
234241
try {
235242
caught = this.emit('error', er);
@@ -253,10 +260,6 @@ Domain.prototype._errorHandler = function(er) {
253260
// The domain error handler threw! oh no!
254261
// See if another domain can catch THIS error,
255262
// or else crash on the original one.
256-
// If the user already exited it, then don't double-exit.
257-
if (this === exports.active) {
258-
stack.pop();
259-
}
260263
updateExceptionCapture();
261264
if (stack.length) {
262265
exports.active = process.domain = stack[stack.length - 1];
@@ -486,7 +489,46 @@ EventEmitter.prototype.emit = function(...args) {
486489
er.domainThrown = false;
487490
}
488491

492+
// Remove the current domain (and its duplicates) from the domains stack and
493+
// set the active domain to its parent (if any) so that the domain's error
494+
// handler doesn't run in its own context. This prevents any event emitter
495+
// created or any exception thrown in that error handler from recursively
496+
// executing that error handler.
497+
const origDomainsStack = stack.slice();
498+
const origActiveDomain = process.domain;
499+
500+
// Travel the domains stack from top to bottom to find the first domain
501+
// instance that is not a duplicate of the current active domain.
502+
let idx = stack.length - 1;
503+
while (idx > -1 && process.domain === stack[idx]) {
504+
--idx;
505+
}
506+
507+
// Change the stack to not contain the current active domain, and only the
508+
// domains above it on the stack.
509+
if (idx < 0) {
510+
stack.length = 0;
511+
} else {
512+
stack.splice(idx + 1);
513+
}
514+
515+
// Change the current active domain
516+
if (stack.length > 0) {
517+
exports.active = process.domain = stack[stack.length - 1];
518+
} else {
519+
exports.active = process.domain = null;
520+
}
521+
522+
updateExceptionCapture();
523+
489524
domain.emit('error', er);
525+
526+
// Now that the domain's error handler has completed, restore the domains
527+
// stack and the active domain to their original values.
528+
exports._stack = stack = origDomainsStack;
529+
exports.active = process.domain = origActiveDomain;
530+
updateExceptionCapture();
531+
490532
return false;
491533
}
492534

‎test/parallel/test-domain-emit-error-handler-stack.js‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const domain = require('domain');
6+
const EventEmitter = require('events');
7+
8+
/*
9+
* Make sure that the domains stack and the active domain is setup properly when
10+
* a domain's error handler is called due to an error event being emitted.
11+
* More specifically, we want to test that:
12+
* - the active domain in the domain's error handler is *not* that domain, *but*
13+
* the active domain is a any direct parent domain at the time the error was
14+
* emitted.
15+
* - the domains stack in the domain's error handler does *not* include that
16+
* domain, *but* it includes all parents of that domain when the error was
17+
* emitted.
18+
*/
19+
const d1 = domain.create();
20+
const d2 = domain.create();
21+
const d3 = domain.create();
22+
23+
function checkExpectedDomains(err) {
24+
// First, check that domains stack and active domain is as expected when the
25+
// event handler is called synchronously via ee.emit('error').
26+
if (domain._stack.length !== err.expectedStackLength) {
27+
console.error('expected domains stack length of %d, but instead is %d',
28+
err.expectedStackLength, domain._stack.length);
29+
process.exit(1);
30+
}
31+
32+
if (process.domain !== err.expectedActiveDomain) {
33+
console.error('expected active domain to be %j, but instead is %j',
34+
err.expectedActiveDomain, process.domain);
35+
process.exit(1);
36+
}
37+
38+
// Then make sure that the domains stack and active domain is setup as
39+
// expected when executing a callback scheduled via nextTick from the error
40+
// handler.
41+
process.nextTick(() => {
42+
const expectedStackLengthInNextTickCb =
43+
err.expectedStackLength > 0 ? 1 : 0;
44+
if (domain._stack.length !== expectedStackLengthInNextTickCb) {
45+
console.error('expected stack length in nextTick cb to be %d, ' +
46+
'but instead is %d', expectedStackLengthInNextTickCb,
47+
domain._stack.length);
48+
process.exit(1);
49+
}
50+
51+
const expectedActiveDomainInNextTickCb =
52+
expectedStackLengthInNextTickCb === 0 ? undefined :
53+
err.expectedActiveDomain;
54+
if (process.domain !== expectedActiveDomainInNextTickCb) {
55+
console.error('expected active domain in nextTick cb to be %j, ' +
56+
'but instead is %j', expectedActiveDomainInNextTickCb,
57+
process.domain);
58+
process.exit(1);
59+
}
60+
});
61+
}
62+
63+
d1.on('error', common.mustCall((err) => {
64+
checkExpectedDomains(err);
65+
}, 2));
66+
67+
d2.on('error', common.mustCall((err) => {
68+
checkExpectedDomains(err);
69+
}, 2));
70+
71+
d3.on('error', common.mustCall((err) => {
72+
checkExpectedDomains(err);
73+
}, 1));
74+
75+
d1.run(() => {
76+
const ee = new EventEmitter();
77+
assert.strictEqual(process.domain, d1);
78+
assert.strictEqual(domain._stack.length, 1);
79+
80+
const err = new Error('oops');
81+
err.expectedStackLength = 0;
82+
err.expectedActiveDomain = null;
83+
ee.emit('error', err);
84+
85+
assert.strictEqual(process.domain, d1);
86+
assert.strictEqual(domain._stack.length, 1);
87+
});
88+
89+
d1.run(() => {
90+
d1.run(() => {
91+
const ee = new EventEmitter();
92+
93+
assert.strictEqual(process.domain, d1);
94+
assert.strictEqual(domain._stack.length, 2);
95+
96+
const err = new Error('oops');
97+
err.expectedStackLength = 0;
98+
err.expectedActiveDomain = null;
99+
ee.emit('error', err);
100+
101+
assert.strictEqual(process.domain, d1);
102+
assert.strictEqual(domain._stack.length, 2);
103+
});
104+
});
105+
106+
d1.run(() => {
107+
d2.run(() => {
108+
const ee = new EventEmitter();
109+
110+
assert.strictEqual(process.domain, d2);
111+
assert.strictEqual(domain._stack.length, 2);
112+
113+
const err = new Error('oops');
114+
err.expectedStackLength = 1;
115+
err.expectedActiveDomain = d1;
116+
ee.emit('error', err);
117+
118+
assert.strictEqual(process.domain, d2);
119+
assert.strictEqual(domain._stack.length, 2);
120+
});
121+
});
122+
123+
d1.run(() => {
124+
d2.run(() => {
125+
d2.run(() => {
126+
const ee = new EventEmitter();
127+
128+
assert.strictEqual(process.domain, d2);
129+
assert.strictEqual(domain._stack.length, 3);
130+
131+
const err = new Error('oops');
132+
err.expectedStackLength = 1;
133+
err.expectedActiveDomain = d1;
134+
ee.emit('error', err);
135+
136+
assert.strictEqual(process.domain, d2);
137+
assert.strictEqual(domain._stack.length, 3);
138+
});
139+
});
140+
});
141+
142+
d3.run(() => {
143+
d1.run(() => {
144+
d3.run(() => {
145+
d3.run(() => {
146+
const ee = new EventEmitter();
147+
148+
assert.strictEqual(process.domain, d3);
149+
assert.strictEqual(domain._stack.length, 4);
150+
151+
const err = new Error('oops');
152+
err.expectedStackLength = 2;
153+
err.expectedActiveDomain = d1;
154+
ee.emit('error', err);
155+
156+
assert.strictEqual(process.domain, d3);
157+
assert.strictEqual(domain._stack.length, 4);
158+
});
159+
});
160+
});
161+
});

‎test/parallel/test-domain-thrown-error-handler-stack.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const domain = require('domain');
5+
6+
/*
7+
* Make sure that when an erorr is thrown from a nested domain, its error
8+
* handler runs outside of that domain, but within the context of any parent
9+
* domain.
10+
*/
11+
12+
const d = domain.create();
13+
const d2 = domain.create();
14+
15+
d2.on('error', common.mustCall((err) => {
16+
if (domain._stack.length !== 1) {
17+
console.error('domains stack length should be 1 but is %d',
18+
domain._stack.length);
19+
process.exit(1);
20+
}
21+
22+
if (process.domain !== d) {
23+
console.error('active domain should be %j but is %j', d, process.domain);
24+
process.exit(1);
25+
}
26+
27+
process.nextTick(() => {
28+
if (domain._stack.length !== 1) {
29+
console.error('domains stack length should be 1 but is %d',
30+
domain._stack.length);
31+
process.exit(1);
32+
}
33+
34+
if (process.domain !== d) {
35+
console.error('active domain should be %j but is %j', d,
36+
process.domain);
37+
process.exit(1);
38+
}
39+
});
40+
}));
41+
42+
d.run(() => {
43+
d2.run(() => {
44+
throw new Error('oops');
45+
});
46+
});

‎test/parallel/test-domain-top-level-error-handler-clears-stack.js‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,18 @@ const domain = require('domain');
1010
const d = domain.create();
1111

1212
d.on('error', common.mustCall(() => {
13+
// Scheduling a callback with process.nextTick _could_ enter a _new_ domain,
14+
// but domain's error handlers are called outside of their domain's context.
15+
// So there should _no_ domain on the domains stack if the domains stack was
16+
// cleared properly when the domain error handler was called.
1317
process.nextTick(() => {
14-
// Scheduling a callback with process.nextTick will enter a _new_ domain,
15-
// and the callback will be called after the domain that handled the error
16-
// was exited. So there should be only one domain on the domains stack if
17-
// the domains stack was cleared properly when the domain error handler
18-
// returned.
19-
if (domain._stack.length !== 1) {
18+
if (domain._stack.length !== 0) {
2019
// Do not use assert to perform this test: this callback runs in a
2120
// different callstack as the original process._fatalException that
2221
// handled the original error, thus throwing here would trigger another
2322
// call to process._fatalException, and so on recursively and
2423
// indefinitely.
25-
console.error('domains stack length should be 1, but instead is:',
24+
console.error('domains stack length should be 0, but instead is:',
2625
domain._stack.length);
2726
process.exit(1);
2827
}

‎test/parallel/test-timers-reset-process-domain-on-throw.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ function err() {
2626
}
2727

2828
function handleDomainError(e) {
29-
// In the domain's error handler, the current active domain should be the
30-
// domain within which the error was thrown.
31-
assert.strictEqual(process.domain, d);
29+
assert.strictEqual(e.domain, d);
30+
// Domains' error handlers are called outside of their domain's context, so
31+
// we're not expecting any active domain here.
32+
assert.strictEqual(process.domain, undefined);
3233
}
3334
}
3435

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL