Skip to content

Commit

Permalink
events: change EventTarget handler exception behavior
Browse files Browse the repository at this point in the history
Change the behavior of EventTarget, instead of emitting
'error' on handler exception, emit 'uncaughtException'.

Fixes: nodejs#36770
  • Loading branch information
Nitzan Uziely committed Feb 5, 2021
1 parent fe43bd8 commit 959cf80
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 31 deletions.
9 changes: 6 additions & 3 deletions doc/api/events.md
Original file line number Diff line number Diff line change
Expand Up @@ -1137,6 +1137,9 @@ setMaxListeners(5, target, emitter);
<!-- YAML
added: v14.5.0
changes:
- version: REPLACEME
pr-url: xxx
description: changed EventTarget error handling.
- version: v15.4.0
pr-url: https://github.com/nodejs/node/pull/35949
description: No longer experimental.
Expand Down Expand Up @@ -1247,9 +1250,9 @@ target.addEventListener('foo', handler4, { once: true });
### `EventTarget` error handling

When a registered event listener throws (or returns a Promise that rejects),
by default the error is forwarded to the `process.on('error')` event
on `process.nextTick()`. Throwing within an event listener will *not* stop
the other registered handlers from being invoked.
By default the error is forwarded to the `process.on('uncaughtException')`
event on `process.nextTick()`. Throwing within an event listener will *not*
stop the other registered handlers from being invoked.

The `EventTarget` does not implement any special default handling for
`'error'` type events.
Expand Down
32 changes: 15 additions & 17 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,23 +248,23 @@ class EventTarget {

[kNewListener](size, type, listener, once, capture, passive) {
if (this[kMaxEventTargetListeners] > 0 &&
size > this[kMaxEventTargetListeners] &&
!this[kMaxEventTargetListenersWarned]) {
size > this[kMaxEventTargetListeners] &&
!this[kMaxEventTargetListenersWarned]) {
this[kMaxEventTargetListenersWarned] = true;
// No error code for this since it is a Warning
// eslint-disable-next-line no-restricted-syntax
const w = new Error('Possible EventTarget memory leak detected. ' +
`${size} ${type} listeners ` +
`added to ${inspect(this, { depth: -1 })}. Use ` +
'events.setMaxListeners() to increase limit');
`${size} ${type} listeners ` +
`added to ${inspect(this, { depth: -1 })}. Use ` +
'events.setMaxListeners() to increase limit');
w.name = 'MaxListenersExceededWarning';
w.target = this;
w.type = type;
w.count = size;
process.emitWarning(w);
}
}
[kRemoveListener](size, type, listener, capture) {}
[kRemoveListener](size, type, listener, capture) { }

addEventListener(type, listener, options = {}) {
if (arguments.length < 2)
Expand All @@ -285,7 +285,7 @@ class EventTarget {
// No error code for this since it is a Warning
// eslint-disable-next-line no-restricted-syntax
const w = new Error(`addEventListener called with ${listener}` +
' which has no effect.');
' which has no effect.');
w.name = 'AddEventListenerArgumentTypeWarning';
w.target = this;
w.type = type;
Expand Down Expand Up @@ -394,7 +394,7 @@ class EventTarget {
let next;

while (handler !== undefined &&
(handler.passive || event?.[kStop] !== true)) {
(handler.passive || event?.[kStop] !== true)) {
// Cache the next item in case this iteration removes the current one
next = handler.next;

Expand All @@ -420,9 +420,9 @@ class EventTarget {
}
const result = FunctionPrototypeCall(handler.callback, this, arg);
if (result !== undefined && result !== null)
addCatch(this, result, createEvent());
addCatch(result);
} catch (err) {
emitUnhandledRejectionOrErr(this, err, createEvent());
emitUncaughtException(err);
}

handler = next;
Expand Down Expand Up @@ -550,7 +550,7 @@ ObjectDefineProperties(NodeEventTarget.prototype, {

function shouldAddListener(listener) {
if (typeof listener === 'function' ||
typeof listener?.handleEvent === 'function') {
typeof listener?.handleEvent === 'function') {
return true;
}

Expand Down Expand Up @@ -582,19 +582,17 @@ function isEventTarget(obj) {
return obj?.constructor?.[kIsEventTarget];
}

function addCatch(that, promise, event) {
function addCatch(promise) {
const then = promise.then;
if (typeof then === 'function') {
FunctionPrototypeCall(then, promise, undefined, function(err) {
// The callback is called with nextTick to avoid a follow-up
// rejection from this promise.
process.nextTick(emitUnhandledRejectionOrErr, that, err, event);
emitUncaughtException(err);
});
}
}

function emitUnhandledRejectionOrErr(that, err, event) {
process.emit('error', err, event);
function emitUncaughtException(err) {
process.nextTick(() => { throw err; });
}

function makeEventHandler(handler) {
Expand Down
25 changes: 14 additions & 11 deletions test/parallel/test-eventtarget.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ let asyncTest = Promise.resolve();
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The "options" argument must be of type object.' +
common.invalidArgTypeHelper(i)
common.invalidArgTypeHelper(i)
})
));
}
Expand Down Expand Up @@ -176,20 +176,23 @@ let asyncTest = Promise.resolve();
}

{
const uncaughtException = common.mustCall((err, event) => {
const uncaughtException = common.mustCall((err, origin) => {
strictEqual(err.message, 'boom');
strictEqual(event.type, 'foo');
strictEqual(origin, 'uncaughtException');
}, 4);

// Whether or not the handler function is async or not, errors
// are routed to uncaughtException
process.on('error', uncaughtException);
// Make sure that we no longer call 'error' on error.
process.on('error', common.mustNotCall());
// Don't call rejection even for async handlers.
process.on('unhandledRejection', common.mustNotCall());
process.on('uncaughtException', uncaughtException);

const eventTarget = new EventTarget();

const ev1 = async () => { throw new Error('boom'); };
const ev2 = () => { throw new Error('boom'); };
const ev3 = { handleEvent() { throw new Error('boom'); } };
const ev1 = () => { throw new Error('boom'); };
const ev2 = { handleEvent() { throw new Error('boom'); } };

const ev3 = async () => { throw new Error('boom'); };
const ev4 = { async handleEvent() { throw new Error('boom'); } };

// Errors in a handler won't stop calling the others.
Expand Down Expand Up @@ -235,15 +238,15 @@ let asyncTest = Promise.resolve();
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The "event" argument must be an instance of Event.' +
common.invalidArgTypeHelper(i)
common.invalidArgTypeHelper(i)
});
});

const err = (arg) => ({
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
message: 'The "listener" argument must be an instance of EventListener.' +
common.invalidArgTypeHelper(arg)
common.invalidArgTypeHelper(arg)
});

[
Expand Down

0 comments on commit 959cf80

Please sign in to comment.