Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define a central error reporting hook #43136

Closed
dead-claudia opened this issue May 18, 2022 · 2 comments
Closed

Define a central error reporting hook #43136

dead-claudia opened this issue May 18, 2022 · 2 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@dead-claudia
Copy link

What is the problem this feature will solve?

I want to be able to just add one hook and capture all warnings and non-fatal errors within a system, and deal with them exactly in the way I know is best for my app, without the noise and without significant code changes when new language and platform features are added.

What is the feature you are proposing to solve the problem?

A process.addErrorMonitor(monitor) method, accepting a monitor(error, origin) function and returning a function to remove it.

  • error - The error in question
  • origin - A string representing where it came from:
    • "sync" - Error was thrown and propagated out of the event loop
    • "async" - Error came from either an unhandled "error" event or a rejected promise that was collected before the error was handled
    • "swallow" - Error came from Promise.all or Promise.race
  • If the return value is falsy, the process dies with the error (and other error monitors skipped), and if it's truthy, it's kept alive without any logging at all.
  • This hook is per-thread and is invoked after the current microtask completes, but before any subsequent tasks are started. It's invoked with each error individually until it returns a value such that the process should die. This makes it particularly convenient when paired with a watchdog parent thread, as sending messages is synchronous relative to the sending thread.
  • Later monitors would be invoked before earlier monitors, allowing them to be reasoned with as proper limited error interception boundaries.

Note: this is not a request for zones or other scoped error interception at a level finer than a single thread. I don't want to complicate the feature any more than it needs to be.

There is one open question: should origin === "swallow" also be fired for finally and/or Promise.prototype.finally as well? I vote no, at least not initially:

  • Without it, one could in theory just reimplement Promise.all and Promise.race to polyfill this fully - V8 providing a native hook is just a point of added convenience here.
  • Doing it with just those two methods would make it a lot harder for V8 to screw up, making the premise behind Deprecate multipleResolves? #41554 less of a concern.
  • There's more ways to swallow exceptions than is afforded via finally (both from let pa = fetchA(), pb = fetchB(); await pa; await pb to catch without a binding), so it's always a best effort anyways. The line between swallowed and truly unhandled is extremely blurry, and the separated await form as mentioned in this point would (unhelpfully) trigger "async" per this propsoal, not "swallow" anyways if pa rejects.

What alternatives have you considered?

  1. Just using all the various existing hooks, including multipleResolves. That last one is of course not sustainable when it's being deprecated due to an inconsistent V8 implementation in Deprecate multipleResolves? #41554. And the rest, while doable, is very awkward to implement in practice.
  2. Introducing a limited variant of Deprecate multipleResolves? #41554 that only returns rejections.
  3. Just wiring up everything to propagate to that central error monitoring point, and replacing Promise.all and Promise.race with variants that report errors instead. This is of course risks significant performance burden, so it's not preferable to the other options. Also, if suppression via finally is tracked, this also would require a lot of source boilerplate to implement as well.
@dead-claudia dead-claudia added the feature request Issues that request new features to be added to Node.js. label May 18, 2022
@bnoordhuis
Copy link
Member

Best way forward is probably to open a pull request implementing what you suggest but please understand it might get rejected. Trying to supersede existing mechanisms with a new one is a bit like xkcd 927.

without significant code changes when new language and platform features are added

That's unknowable without a working crystal ball. I don't know if you were planning to add that claim to the docs but I'd refrain from doing so.

@dead-claudia
Copy link
Author

Best way forward is probably to open a pull request implementing what you suggest but please understand it might get rejected. Trying to supersede existing mechanisms with a new one is a bit like xkcd 927.

I'm aware. Besides, I was kinda creating it as kind of a "what if", not that it'd specifically make it in in this precise shape.

That's unknowable without a working crystal ball. I don't know if you were planning to add that claim to the docs but I'd refrain from doing so.

Obviously, I wasn't planning to add that. 😆


In any case, closing in favor of #43165, which is a lot less of a pipe dream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

2 participants