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

Should domains catch unhandledRejection events #20474

Closed
vdeturckheim opened this issue May 2, 2018 · 8 comments
Closed

Should domains catch unhandledRejection events #20474

vdeturckheim opened this issue May 2, 2018 · 8 comments
Assignees
Labels
domain Issues and PRs related to the domain subsystem. promises Issues and PRs related to ECMAScript promises.

Comments

@vdeturckheim
Copy link
Member

  • Version: future
  • Platform: any
  • Subsystem: domain

I have been thinking about this for a while and now that domain API relies on Async Hooks, it would probably make sense by symmetry to catch unhandledRejection events in domains.

d.run(() => {
    throw new Error() // this will be caught
});

but

d.run(await () => {
    throw new Error() // this will not be caught
});

What is your take on this? I might be missing some previous discussions on that topic, sorry if it's the case 👼 .
cc @nodejs/domains @mmarchini

@vdeturckheim vdeturckheim added domain Issues and PRs related to the domain subsystem. promises Issues and PRs related to ECMAScript promises. labels May 2, 2018
@vdeturckheim vdeturckheim self-assigned this May 2, 2018
@mmarchini
Copy link
Contributor

cc @nodejs/diagnostics

@mcollina
Copy link
Member

mcollina commented May 2, 2018

I am -1 to do anything to domains. I think we should remove them as soon as async_hooks exits experimental. Ideally we could still maintain a userland implementation of them (they are implemented on top of async_hooks anyway).

@AyushG3112
Copy link
Contributor

-1. domain is doc-only deprecated anyways(https://nodejs.org/api/domain.html), and I believe we should avoid it.

@ofrobots
Copy link
Contributor

ofrobots commented May 2, 2018

I'm -1 on changes to domains at this point. They are a dangerous and problematic API. I think we need them around for a while so that people can migrate to async-hooks or other higher level alternatives.

@vdeturckheim
Copy link
Member Author

I get your points but I still feel we could go further here:

  • domain is pretty much already an "over async hooks alternative", externalization should not be hard at this point
  • if we externalize, we will add new features (at least I will try to do it) and maintain it, so if we do it later, why not doing it now?

At this point, domains are still used a lot (I know a handful of new projects which rely on them).

@AndreasMadsen
Copy link
Member

Ideally we could still maintain a userland implementation of them (they are implemented on top of async_hooks anyway).

We are almost there, but there is one feature we can't support and we need another semver major release (node.js 11) before we can remove support for that feature. See DEP0097.

This feature addition makes sense to me, the only reason why it shouldn't be added is that we wish to remove domains. If we declare make an official effort to move domain to a userland module in node.js 11 or 12, then I don't see an issue with this feature.

@apapirovski
Copy link
Member

Something that hasn't been discussed is the fact that we rely on the domain module extensively within the REPL. It'll take a lot of work to change that.

@vdeturckheim
Copy link
Member Author

Closing as this will probably not happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain Issues and PRs related to the domain subsystem. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

No branches or pull requests

7 participants