events: change EventTarget handler exception behavior

Nitzan Uziely · benjamingr · commit 83d6e63aeea9 · 2021-02-15T20:10:07.000+02:00
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>
diff --git a/doc/api/events.md b/doc/api/events.md
@@ -1137,6 +1137,9 @@ setMaxListeners(5, target, emitter);
 <!-- YAML
 added: v14.5.0
 changes:
+  - version: REPLACEME
+    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.
diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js
@@ -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;
@@ -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) {
diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js
@@ -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();
 
diff --git a/test/parallel/test-process-uncaught-exception-monitor.js b/test/parallel/test-process-uncaught-exception-monitor.js
@@ -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;

Original file line number	Diff line number	Diff line change
`@@ -459,9 +459,9 @@ class EventTarget {`
`459`	`459`	`result = FunctionPrototypeCall(callback, this, arg);`
`460`	`460`	`}`
`461`	`461`	`if (result !== undefined && result !== null)`
`462`		`- addCatch(this, result, createEvent());`
	`462`	`+ addCatch(result);`
`463`	`463`	`} catch (err) {`
`464`		`- emitUnhandledRejectionOrErr(this, err, createEvent());`
	`464`	`+ emitUncaughtException(err);`
`465`	`465`	`}`
`466`	`466`
`467`	`467`	`handler = next;`
`@@ -624,19 +624,19 @@ function isEventTarget(obj) {`
`624`	`624`	`return obj?.constructor?.[kIsEventTarget];`
`625`	`625`	`}`
`626`	`626`
`627`		`-function addCatch(that, promise, event) {`
	`627`	`+function addCatch(promise) {`
`628`	`628`	`const then = promise.then;`
`629`	`629`	`if (typeof then === 'function') {`
`630`	`630`	`FunctionPrototypeCall(then, promise, undefined, function(err) {`
`631`	`631`	`// The callback is called with nextTick to avoid a follow-up`
`632`	`632`	`// rejection from this promise.`
`633`		`- process.nextTick(emitUnhandledRejectionOrErr, that, err, event);`
	`633`	`+ emitUncaughtException(err);`
`634`	`634`	`});`
`635`	`635`	`}`
`636`	`636`	`}`
`637`	`637`
`638`		`-function emitUnhandledRejectionOrErr(that, err, event) {`
`639`		`- process.emit('error', err, event);`
	`638`	`+function emitUncaughtException(err) {`
	`639`	`+ process.nextTick(() => { throw err; });`
`640`	`640`	`}`
`641`	`641`
`642`	`642`	`function makeEventHandler(handler) {`