nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.88k stars 29.73k forks source link

discuss: remove domain module #45824

Open bnoordhuis opened 1 year ago

bnoordhuis commented 1 year ago

Proposal:


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

MoLow commented 1 year ago

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

marco-ippolito commented 1 year ago

this is the previous discussion opened more than 5 years ago: https://github.com/nodejs/node/issues/10843 Other discussion about removal: https://github.com/nodejs/node/issues/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 commented 1 year ago

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

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

bnoordhuis commented 1 year ago

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.

targos commented 1 year ago

There are a few (all I saw come either from mocha or tap): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/bluebird_v3_7_2/ https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/browserify_v17_0_0/ https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/gulp_v4_0_2/ https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/node_gyp_v9_3_0/ https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3060/nodes=fedora-last-latest-x64/testReport/junit/(root)/citgm/rimraf_v3_0_2/ ... and more with tap

bnoordhuis commented 1 year ago

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

ORESoftware commented 1 year ago

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 commented 7 months ago

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 commented 7 months ago

The AsyncLocalStorage and async_hooks just aren't reliable.

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