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: #36770

PR-URL: #37237
Fixes: #36770
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
Nitzan Uziely authored and benjamingr committed Feb 15, 2021
1 parent 9803729 commit 83d6e63
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 18 deletions.
3 changes: 3 additions & 0 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

This comment has been minimized.

Copy link
@omril1

omril1 Feb 22, 2021

@benjamingr I think this was missed during review.

This comment has been minimized.

Copy link
@richardlau

richardlau Feb 22, 2021

Member

No, it hasn't gone out in a release yet. This placeholder is replaced as part of the release process.

pr-url: https://github.com/nodejs/node/pull/37237
description: changed EventTarget error handling.
- version: v15.4.0
pr-url: https://github.com/nodejs/node/pull/35949
description: No longer experimental.
Expand Down
12 changes: 6 additions & 6 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,9 @@ class EventTarget {
result = FunctionPrototypeCall(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 @@ -624,19 +624,19 @@ 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
12 changes: 7 additions & 5 deletions test/parallel/test-eventtarget.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,16 @@ 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();

Expand Down
14 changes: 7 additions & 7 deletions test/parallel/test-process-uncaught-exception-monitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ process.on(
process.on('uncaughtException', common.mustCall((err, origin) => {
assert.strictEqual(origin, 'uncaughtException');
assert.strictEqual(err, theErr);
}));

process.nextTick(common.mustCall(() => {
// Test with uncaughtExceptionCaptureCallback installed
process.setUncaughtExceptionCaptureCallback(common.mustCall(
(err) => assert.strictEqual(err, theErr))
);
process.nextTick(common.mustCall(() => {
// Test with uncaughtExceptionCaptureCallback installed
process.setUncaughtExceptionCaptureCallback(common.mustCall(
(err) => assert.strictEqual(err, theErr))
);

throw theErr;
throw theErr;
}));
}));

throw theErr;

0 comments on commit 83d6e63

Please sign in to comment.