events: implement captureRejections for async handlers

[mcollina]

One of the biggest source of issues with 'unhandledRejection' is the use of EventEmitter in combination with an async function (see https://github.com/mcollina/make-promises-safe for details). Currently, there is no way to catch a rejection when it is emitted within an event handler, causing hard to track bugs and memory leaks. Our current best practice is to always wrap the content of an async function in a try/catch block and handle errors, but this is error prone.

This PR adds a new captureRejections option to EventEmitter as well as a way to override the global default. If an EventEmitter is created with captureRejections enabled, a .catch() handler is added every time a Promise is returned from an event handler: if a rejection happens, the 'error' event is triggered, avoiding the 'unhandledRejection'. An implementer can also listen to Symbol.for('nodejs.rejection') to automatically dispose of resources, and in that case 'error' is not emitted (Streams will leverage this in the future).

Example:

const ee1 = new EventEmitter({ captureRejections: true });
ee1.on('something', async (value) => {
  throw new Error('kaboom');
});

ee1.on('error', console.log);

const ee2 = new EventEmitter({ captureRejections: true });
ee2.on('something', async (value) => {
  throw new Error('kaboom');
});

ee2.on(Symbol.for('nodejs.rejection'), console.log);

EventEmitter.captureRejections = true;
const ee3 = new EventEmitter();
ee3.on('something', async (value) => {
  throw new Error('kaboom');
});

ee3.on('error', console.log);

This has also the side benefit of greatly improving our microbenchmarks, up to 88% in one case:

                                                     confidence improvement accuracy (*)   (**)  (***)
 events/ee-add-remove.js n=1000000                                   1.54 %       ±2.98% ±4.02% ±5.33%
 events/ee-emit.js listeners=1 argc=0 n=2000000             ***     34.13 %       ±0.69% ±0.92% ±1.20%
 events/ee-emit.js listeners=1 argc=10 n=2000000            ***     12.08 %       ±1.63% ±2.19% ±2.88%
 events/ee-emit.js listeners=1 argc=2 n=2000000             ***      9.37 %       ±1.36% ±1.83% ±2.43%
 events/ee-emit.js listeners=1 argc=4 n=2000000             ***      8.17 %       ±3.18% ±4.27% ±5.65%
 events/ee-emit.js listeners=10 argc=0 n=2000000            ***     88.93 %       ±0.71% ±0.96% ±1.26%
 events/ee-emit.js listeners=10 argc=10 n=2000000           ***     22.19 %       ±0.95% ±1.27% ±1.66%
 events/ee-emit.js listeners=10 argc=2 n=2000000            ***     26.07 %       ±0.54% ±0.72% ±0.94%
 events/ee-emit.js listeners=10 argc=4 n=2000000            ***     22.78 %       ±0.57% ±0.76% ±0.99%
 events/ee-emit.js listeners=5 argc=0 n=2000000             ***     64.96 %       ±3.82% ±5.14% ±6.82%
 events/ee-emit.js listeners=5 argc=10 n=2000000            ***     24.33 %       ±0.88% ±1.17% ±1.53%
 events/ee-emit.js listeners=5 argc=2 n=2000000             ***     24.50 %       ±1.04% ±1.38% ±1.81%
 events/ee-emit.js listeners=5 argc=4 n=2000000             ***     26.23 %       ±1.88% ±2.52% ±3.31%
 events/ee-listener-count-on-prototype.js n=50000000                 0.33 %       ±0.72% ±0.96% ±1.25%
 events/ee-listeners-many.js n=5000000                               0.98 %       ±1.60% ±2.12% ±2.76%
 events/ee-listeners.js n=5000000                                    0.04 %       ±1.10% ±1.46% ±1.91%
 events/ee-once.js n=20000000                               ***      6.17 %       ±2.24% ±3.02% ±3.99%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 17 comparisons, you can thus
expect the following amount of false-positive results:
  0.85 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.17 false positives, when considering a   1% risk acceptance (**, ***),
  0.02 false positives, when considering a 0.1% risk acceptance (***)

I have not done a deep investigation on why, but I think it's due to the retrieval of Reflect.apply from the Reflect object. Even if this change gets rejected, I think we should investigate how to leverage the perf improvements alone.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[devsnek]

You can just use Reflect.apply, since Reflect here is coming from primordials

[mcollina]

That's what's giving us the speed bump.

[himself65]

this property only able on async function, why don't rename it to asyncCatch? 🤔

[devsnek]

you could have a normal function that returns a promise

[benjamingr]

@devsnek arguably a normal function that returns a promise is an async function :P

[benjamingr]

This looks great it will take me a few days to think this over and review 😊

[benjamingr]

First batch before taking a look at the code.

[benjamingr]

I would prefer an explicit name like captureRejections or something that indicates that rejections are converted to errors.

[benjamingr]

Please add a test on how this works in subclasses of EventEmitter? This is scary for libraries to do otherwise I think.

[benjamingr]

Suggested change

	setting change this behavior, installing a `.catch()` handler on the
	setting change this behavior, installing an error handler on the

[benjamingr]

Honestly this would require someone to return a promise from an event emitter method and add a catch method to it later which means it's not an async function.

I am entirely fine with not supporting this case or emitting an event here just because the added complexity likely isn't worth it.

[benjamingr]

I think that if we keep this symbol it's likely better to keep it on events rather than the global symbol registry like util.promisify.custom unless there is a specific need for it to behave well across realms.

[jasnell]

Keeping it on the global symbol registry allows it to be used browser side as well, which is going to be important for things like readable-stream and the various browser ports of EventEmitter. That said, exposing the symbol as a convenience property makes sense... e.g.

const { EventEmitter, rejection } = require('events')

ee2.on(rejection, console.log)

// would be equivalent to

ee2.on(Symbol.for('nodejs.rejection'), console.log)

// either would work

[mcollina]

I'm a bit unsure about exporting it as a symbol, mainly because we have a global switch at events.captureRejections, and the Symbol at events.rejection. I think this can get confusing very quickly.

[benjamingr]

@devsnek arguably a normal function that returns a promise is an async function :P

[benjamingr]

Random thought: we can check if something is an AsyncFunction passed to the event emitter, if it is we know that there is no catch handler attached to it because we have invoked the function ourselves.

In this case (which is by far the most common one) unlike a regular unhandledRejection I think we should emit error by default in a future Node.js version and I'd like to discuss this and/or vote on this in the summit if possible :]

[mcollina]

Random thought: we can check if something is an AsyncFunction passed to the event emitter, if it is we know that there is no catch handler attached to it because we have invoked the function ourselves.

I thought a lot about that. However that would be hard for test frameworks, including our own in core common.mustCall(async function() {}) returns a function() {}. Moreover, that would not be safe against refactoring, as moving some code around can change where the promise come from. In other terms, I think we should not have a different behavior between functions that returns a promise and async functions, even thought the goal of this PR is to cover async functions.

---

In this case (which is by far the most common one) unlike a regular unhandledRejection I think we should emit error by default in a future Node.js version and I'd like to discuss this and/or vote on this in the summit if possible :]

I would like discuss to turn EventEmitter.autoCatch = true by default in 13. At the beginning I thought to make this a semver-major PR, but @MylesBorins made the point that making it semver-minor means it could be backported down to 10.

[benjamingr]

In other terms, I think we should not have a different behavior between functions that returns a promise and async functions, even thought the goal of this PR is to cover async functions

What about "functions we know return promises no one adds a catch to :)?

[mcollina]

What about "functions we know return promises no one adds a catch to :)?

I don't understand. You mean functions that purposefully would like to trigger an 'unhandledRejection'?