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

discuss: remove domain module #45824

Open
bnoordhuis opened this issue Dec 12, 2022 · 9 comments · May be fixed by #45839
Open

discuss: remove domain module #45824

bnoordhuis opened this issue Dec 12, 2022 · 9 comments · May be fixed by #45839
Labels
discuss Issues opened for discussions and feedbacks. domain Issues and PRs related to the domain subsystem.

Comments

@bnoordhuis
Copy link
Member

  • require('domain') was deprecated 7.5 years ago

  • modern code uses AbortController

  • bug tracker activity suggests it's not in active use (or it works so well people never hit bugs0 - unlikely!)

  • the supporting infrastructure for domain is a kind of technical debt that also has performance implications1; it's not as bad as it used to be but it's still there

Proposal:

  • remove it for good in the next major release

  • leave a stub lib/domain.js that prints a warning, a la the sys module.


0 the prime reason for deprecation was because it was so buggy due to part poor design, part poor implementation

1 at one time it was so bad that if even one module imported domain, whole-program performance tanked

@bnoordhuis bnoordhuis added domain Issues and PRs related to the domain subsystem. discuss Issues opened for discussions and feedbacks. labels Dec 12, 2022
@MoLow
Copy link
Member

MoLow commented Dec 12, 2022

a good idea would be to test how this will effect citgm

@marco-ippolito
Copy link
Member

marco-ippolito commented Dec 12, 2022

this is the previous discussion opened more than 5 years ago: #10843
Other discussion about removal: #17143

Domains were deprecated in node v4, after all. As of v13, still no answer as to what to use instead.

It seems that it is something that has been around for quite some time now, I think is time to remove domain.

@bnoordhuis
Copy link
Member Author

They've been deprecated even longer, the deprecation notice was already there in the first io.js release! (ref. 6a29356)

I'll open a trial PR and see how it fares.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Dec 13, 2022
@bnoordhuis bnoordhuis linked a pull request Dec 13, 2022 that will close this issue
@bnoordhuis
Copy link
Member Author

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/ - honestly, there are so many random test failures (also in other, unrelated citgm runs) that it's hard to say whether it's indicative of anything.

FWIW, I didn't find any mentions of the domain module while randomly clicking through some of the failing test suites.

@bnoordhuis
Copy link
Member Author

Ah, thanks. Okay, so that means a runtime deprecation first before the hammer comes down.

@ORESoftware
Copy link
Contributor

ORESoftware commented Oct 30, 2023

I for one, vote to keep Domains - I use them for multiple use cases - is there a clear replacement for the functionality? Until there is a clear replacement, I vote to keep in core. Make something better, before just removing it. Test libraries like tap and others use domains, and I use domains in production.

In other words: The domain API is simple to use, intuitive, and works, however it is a bit slow in performance. I would like AsyncLocalStorage (or whatever replaces Domain) to be beyond "experimental" before removing Domains when there is no replacement for the Domain functionality (which is critical for some people who use in production).

For example middleware similar to this can improve production servers:

app.use((req,res,next) => {
    
    const d = Domain.create();
    
    d.once('error', e => {
       res.status(500).send(e.message);
    });
    
    d.run(next);

});

if there is a new api that can what the above does, and improve performance, I am all for it. 👍
However AsyncLocalStorage is still experimental?

For example:
https://chat.openai.com/share/2736160d-2f1c-4663-b7e7-758d6a922a2c

@lroal
Copy link

lroal commented Apr 24, 2024

Keep Domain. The AsyncLocalStorage and async_hooks just aren't reliable.
However, I see there is a TC39 proposal in stage 2 for AsyncLocalStorage

@Flarna
Copy link
Member

Flarna commented Apr 24, 2024

The AsyncLocalStorage and async_hooks just aren't reliable.

FWIW domain uses async hooks internally so I doubt it's more reliable.

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

Successfully merging a pull request may close this issue.

7 participants