fix(client): Rethrow async event emitter errors ASAP #1312
Conversation
| @@ -146,7 +146,18 @@ export class AsyncEventEmitter<Events extends EventMap> { | |||
| // deep copy because once listeners mutate the `this.listeners` array as they remove themselves | |||
| // which breaks the `map` which iterates over that same array while the contents may shift | |||
| const ls = [...listeners] | |||
| const listenerProms = ls.map((listener) => listener(...args)) | |||
| const listenerProms = ls.map(async (listener) => { | |||
There was a problem hiding this comment.
there's a bit of boilerplate here with the async function, await, and then surrounding it in try catch.
Would simplify it to:
ls.map((listener) =>
listener(...args)
.catch((err) =>
queueMicrotask(() => throw err)
)
)There was a problem hiding this comment.
Minor issue is that the listeners can be both sync and async (return type void | Promise<void>) so .catch can only conditionally be chained, which means that errors from synchronous listeners will be thrown immediately in the map and the ones from asynchronous listeners will be thrown after the queue is processed.
Not necessarily the worst case but with the try ... catch and await they are all handled the same way and all errors are bubbled up in the same way (although synchronous handlers will now be delayed a little bit because of await giving up control, but it's fine since it's an async event emitter)
Basically without the try ... catch we introduce the possibility of the queue not being fully processed and it getting interrupted by an error from a synchronous listener
There was a problem hiding this comment.
I see, let's keep it how it is 👍
Minor correction to the fix implemented in #1247
Reintroducing the
queueMicrotaskthrowing approach to ensure errors are definitely not suppressed (e.g. they need to be thrown independently of all listener promisessettling/fulfilling) and ideally if more than one of them occur, all of them are exposed to the global error handler. See discussion here #1247 (comment)The
queueMicrotaskAPI seems to be widely supported by browsers and runtimes so I don't think there's a need to worry. The alternative is also to usesetTimeoutwhich is also widely supported, but that might cause the errors to be thrown significantly later than when they were emitted.