EventTarget uncaught exception handling differs from spec/browser

[wnayes]

Version

v19.7.0

Platform

Linux 6.1.12-arch1-1 #1 SMP PREEMPT_DYNAMIC Tue, 14 Feb 2023 22:08:08 +0000 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

This is very similar to #38122. Re-opening essentially, to consider further refining the behavior.

Here is the example from that issue:

(function() {
  const t = new EventTarget();
  t.addEventListener('foo', evt => { throw new Error("Error thrown by first listener"); });
  t.addEventListener('foo', evt => console.log("Second listener received event:", evt));
  t.dispatchEvent(new Event('foo'));
  console.log("After dispatch");
})();

This can be ran in both Node and browsers.

How often does it reproduce? Is there a required condition?

Consistent behavior.

What is the expected behavior?

Browsers will trigger the unhandled exception handling (window onerror) immediately, followed by the two console logs.

1. Uncaught Error: Error thrown by first listener
2. log: Second listener received event: foo { … }
3. log: After dispatch

What do you see instead?

Node is delaying the unhandled exception handling until the "next tick."

1. log: Second listener received event: foo { … }
2. log: After dispatch
3. Uncaught Error: Error thrown by first listener

Additional information

The behavior is due to use of emitUncaughtException during dispatch in event_target.js, which throws the error on the next tick.

The impacts seem to be that:

• subsequent code (between when the error is thrown and the next tick) may not realize there was an unhandled exception in one of the handlers. In browsers, you can immediately observe an error via window.onerror.
• If you are debugging with "Uncaught Exceptions" in VS Code, for example, the debugger never pauses, yet the process terminates with an "Uncaught Error." This seems unexpected. This is partly due to a try/catch block in event_target.js wrapping the dispatch logic.

It seems like a general direction towards closer behavior with browsers would be (1) throw immediately, not on next tick and (2) don't wrap dispatching with a try/catch. But if those things were all that was done, I'm not sure how execution would resume (to process subsequent handlers) after a handled exception that doesn't terminate.

[bnoordhuis]

Are there WPT tests that cover this behavior? If yes, it's a matter of enabling them in test/wpt and fixing any failures. If no, then first order of business is adding them upstream (to dom/events, probably.)

[Linkgoron]

Indeed, the request here is to "somehow" both synchronously call the uncaught exception path as continuing the execution of the listeners. Note that a major difference between browsers and Node is that if an uncaught exception handler does not exist, we'll never call the other handlers - which might not be what we want.

Regarding the implementation, I think that this would basically mean changing the current implementation in event_target to call process._fatalException(err) instead of the process.nextTick(()=> {throw err;}).

[bnoordhuis]

@wnayes do you plan to work on this?

[benjamingr]

EventTarget uncaught exception handling differs from spec

First - let me clear up a misconception. No it doesn't as far as I know. The error in this case in spec speak is "report the exception", it is quite intentionally implementation defined . We can (for example) suppress that error and ignore it and still implement the spec correctly.

Error handling is fundamentally different between browsers and server runtimes so it makes sense the spec allows this.

Let's focus on how to be useful and less surprising to our users. Spec compliance isn't the issue here the fact you feel like you ran into a footgun is.

---

I see several viable paths forward:

• Do nothing, the current behavior is mostly fine IMO?
• Emit uncaught exception synchronously, which means subsequent listeners won't run and the error emits synchronously unless the user opts out of the error model (and safety guarantees) of Node.js with an uncaughtException listener.
• Emit the error event (again) synchronously and the uncaughtException event after a microtick.
• Something else?

[wnayes]

I had been trying to re-implement this tricky thing from React. It uses dispatchEvent + window.onerror as an alternative to try/catch, to be able to "catch" errors from user-given callbacks in a way that doesn't involve a true catch block (and consequential debugger behaviors around caught exceptions).

I opened this issue because I was surprised with the extent that EventTarget in Node differed from browser, though the use case I was focusing on is certainly obscure.

I have no concrete plans to look into the EventTarget changes myself, though I am still a bit curious if there is some way to achieve the callback/debugger behavior mentioned. More recently, I looked into EventEmitter and Domains, and these all behaved similar to EventTarget as far as callback dispatch goes, and I was stumped at that point.

[benjamingr]

I opened this issue because I was surprised with the extent that EventTarget in Node differed from browser, though the use case I was focusing on is certainly obscure.

That's fair, and in the past we did fire an error event on process but we removed it since people were not using it and the spec didn't require it. What I'm saying is "When we changed the behavior I noticed this and was waiting for someone to bring that up so we can discuss it".

I have no concrete plans to look into the EventTarget changes myself, though I am still a bit curious if there is some way to achieve the callback/debugger behavior mentioned. More recently, I looked into EventEmitter and Domains, and these all behaved similar to EventTarget as far as callback dispatch goes, and I was stumped at that point.

Sure, can you elaborate on where you're stuck in particular? You can add a uncaughtException handler, rethrow the error if it's not the one you're looking for and intercept it there can't you?

[wnayes]

The goal is to have an API that behaves like this:

function invokeGuardedCallback(callback, onError) {
    try {
        callback();
    } catch (e) {
        onError(e);
    }
}

Except we don't want to use that catch block implementation, because it prevents the debugger from pausing on exceptions within callback. (Unless you pause on all caught exceptions.)

In browsers, this can be implemented with this trick (some details omitted below):

function invokeGuardedCallback(callback, onError) {
    let error;
    globalThis.addEventListener("error", (e) => {
        error = e;
    });
    const target = new EventTarget();
    target.addEventListener("fakeevent", () => { callback(); });
    target.dispatchEvent(new Event("fakeevent"));
    if (error) {
        onError(error);
    }
}

With the current behavior in Node, the above approach doesn't functionally work, because the error event (uncaughtException in this case) fires on a new call stack.

If it were changed to fire synchronously during dispatch, the code would start to function correctly. But since dispatchEvent in Node itself is implemented with a catch block, the debugger behavior always treats all exceptions as caught. That's an issue for this specific use case, but also a general debugging nuisance for any code called through dispatchEvent in Node.

To fix the caught exceptions debugger concern, it seems like the Node internal code needs its own sort of invokeGuardedCallback for calling given callbacks. (And at that point, well, it would be nice it was a public API 😄).

[bnoordhuis]

@benjamingr is ^-- actionable? If not, then I'll close this out as working-as-expected, I suppose?

[benjamingr]

It's actionable (to let users add code where the debugger doesn't break and doesn't interfere with error handling) but should likely be a separate issue

[bnoordhuis]

Okay then, I'll go ahead and close this issue and @wnayes can open a new one for the debugger thingy. But please reopen if I misunderstood you.