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

deprecate domains #66

Closed
jonathanong opened this issue Dec 4, 2014 · 91 comments
Closed

deprecate domains #66

jonathanong opened this issue Dec 4, 2014 · 91 comments
Labels
domain Issues and PRs related to the domain subsystem.

Comments

@jonathanong
Copy link
Contributor

what's the status on domains? can we deprecate it before v1?

@juliangruber
Copy link
Member

afaik no breaking changes will happen until v1

@jonathanong
Copy link
Contributor Author

just labeling something as deprecated is not a breaking change

@juliangruber
Copy link
Member

gotcha, 👍 from me, domains have only meant pain to me personally

@rvagg rvagg added the tc-agenda label Dec 4, 2014
@rvagg
Copy link
Member

rvagg commented Dec 4, 2014

I'm on board, I bet a bunch of TC members would be too, marking as tc-agenda

@indutny
Copy link
Member

indutny commented Dec 4, 2014

+1

@Marak
Copy link

Marak commented Dec 4, 2014

I never understood why domains were added to core.

It seemed wrong at the time, and still seems equally as wrong.

For backwards compatibility, will it be possible to put this functionality into an npm module as a custom add-on? Does this already exist?

@Fishrock123
Copy link
Contributor

+1 domains are confusing and I hear nothing but incompatibilities and edge cases

@othiym23
Copy link
Contributor

othiym23 commented Dec 4, 2014

+1 sad 🎺

@Marak

I never understood why domains were added to core.

There's a place for unified sync & async error handling for Node, and due to the way that state propagates through core, there at least need to be hooks exposed by core to facilitate this. uncaughtException is too gross-grained, not everybody uses (or likes) promises, try/catch + stream error listeners + callback convention is complicated to teach and learn and harder to master, etc. Probably the best effort at this so far is @piscisaureus and co's zones, but that solution has limitations that are going to be tough to resolve outside core.

I'm hopeful that @trevnorris's AsyncWrap can provide the foundation for a better / lower-impact way of building something useful along these lines in the future, and I consider domains a very useful failure that helped point to what AsyncWrap / AsyncListener should do.

@bnoordhuis
Copy link
Member

It would have to go through the TC but I don't think there is much opposition to deprecating domains. Who volunteers to follow up with a pull request?

@tellnes
Copy link
Contributor

tellnes commented Dec 4, 2014

@bnoordhuis A deprecation warning if you call domain.create()? Or when you require it since it has side effects (acording to code comments).

@bnoordhuis
Copy link
Member

@tellnes I'll leave that to your own judgment. Doing it at require() time would get the message out most effectively but it might end up being extremely annoying.

If you do do it at require() time, you should also update lib/repl.js and make it load the domain module lazily.

@tellnes
Copy link
Contributor

tellnes commented Dec 5, 2014

lib/repl.js needs to be changed anyway because it is calling domain.create().

@rlidwka
Copy link
Contributor

rlidwka commented Dec 5, 2014

Is there a suitable replacement that could be used? Repl is a valid use-case for example.

@tellnes
Copy link
Contributor

tellnes commented Dec 5, 2014

@rlidwka Yes, the new tracing module in core. Please see my implementation in #75.

@jodytate
Copy link

jodytate commented Dec 5, 2014

+1 what @Marak said and +1 @Fishrock123 said, so +2

@jmcbee
Copy link

jmcbee commented Dec 6, 2014

What is TC? Only TechCrunch is going in my mind.

@SomeoneWeird
Copy link
Member

See Technical Committee

@benjamingr
Copy link
Member

+1 domains have only brought me pain and suffering in my code and were not good enough in practice. I'd love to see them removed in v1 and deprecated today.

@ORESoftware
Copy link
Contributor

ORESoftware commented Aug 4, 2017

@SosZ please search and read the whole thread before, I patched promises to use domains myself, in a much simpler way

Your code is a little bit of a horror show. What about this instead?

const then = Promise.prototype.then;
Promise.prototype.then = function (fn1, fn2) {

  if (process.domain) {
    fn1 = fn1 && process.domain.bind(fn1);
    fn2 = fn2 && process.domain.bind(fn2);
  }

  return then.call(this, fn1, fn2);
};

Note that you do not need to patch the Promise constructor, because the executors run synchronously, but you can patch Promise.prototype.catch to work in the same way as the above patch on Promise.prototype.then. That being said, pretty certain Node version 8 has allowed domains to work with Promises, so the above patch may only be useful for Node.js versions < 8.

@ORESoftware
Copy link
Contributor

ORESoftware commented Aug 4, 2017

I just want to add my 2 cents to the long run discussion. I have a job. I want to keep my job. I really prefer to not have my server(s) crash, even if we use forever or supervisor to restart upon crashes.

Using the following allows us to avoid many crashes:

let domain = require('domain');

app.use(function(req, res, next){

  let d = domain.create(); // create a new domain for this request

  res.once('finish', function(){
    d.exit();
    d.removeAllListeners();
  });

  d.once('error', function(e){
    // log the issue right here
    if(!res.headersSent){
        res.json({
          error: e.stack || e
       });
    }
  });

  d.run(next);   // we invoke the next middleware
});

we use domains in production. Hopefully no memory leaks though 😂

@evanlucas
Copy link
Contributor

@ORESoftware I've moderated your previous comment. I found it to be inappropriate for this issue tracker. We intend on keeping this issue tracker professional and would appreciate if everyone who posts here does the same.

@nickserv
Copy link

nickserv commented Aug 8, 2017

Domains are still a hack, letting the process crash is often the better option especially with forever/supervisor.

@ORESoftware
Copy link
Contributor

ORESoftware commented Aug 8, 2017

@evanlucas censorship always appreciated, but I guess you're right, best to keep politics off of Github lol...@nickmmcurdy - you'd have to back up that statement - domains will fail in their intention if an error is raised for the wrong path of execution, and if it's easy to misuse them than that's also bad. But I don't think they are a hack perse, just perhaps imperfectly implemented.

@addaleax
Copy link
Member

addaleax commented Aug 8, 2017

@ORESoftware We have a Code of Conduct that asks you to be respectful of other people’s viewpoints and refrain from using language that “could reasonably be considered inappropriate in a professional setting”.

If you don’t feel comfortable with that, it might be easier to take a step back; Node’s project leadership is standing behind the choice to apply that CoC to our spaces.

@ORESoftware
Copy link
Contributor

Imma read the CoC right now to see if my microaggression actually violated it.

@ORESoftware
Copy link
Contributor

Yep, looks like I violated point 1

  • Using welcoming and inclusive language

whoops, my bad fam

targos added a commit to targos/node that referenced this issue Apr 17, 2021
Original commit message:

    [macro-assembler] Avoid using the isolate in CallRecordWriteStub

    CallRecordWriteStub is used in a background compile thread for
    JS-to-Wasm wrapper compilation, so it should avoid accessing the
    isolate.
    Call the builtin using CallBuiltin which does not require a Handle<Code>
    object and instead gets the call target directly from the embedded data.

    R=​[email protected]

    (cherry picked from commit 6b3994e8507b32dfb956329395dbe33a2a8fee14)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1146813
    Change-Id: I4ee59084e4184f2e9039208e4e6db43482cefde6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2593333
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Thibaud Michaud <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#71785}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2731535
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Reviewed-by: Jana Grill <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#66}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@d2283ba
targos added a commit to targos/node that referenced this issue Apr 17, 2021
Original commit message:

    [macro-assembler] Avoid using the isolate in CallRecordWriteStub

    CallRecordWriteStub is used in a background compile thread for
    JS-to-Wasm wrapper compilation, so it should avoid accessing the
    isolate.
    Call the builtin using CallBuiltin which does not require a Handle<Code>
    object and instead gets the call target directly from the embedded data.

    R=​[email protected]

    (cherry picked from commit 6b3994e8507b32dfb956329395dbe33a2a8fee14)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1146813
    Change-Id: I4ee59084e4184f2e9039208e4e6db43482cefde6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2593333
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Thibaud Michaud <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#71785}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2731535
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Reviewed-by: Jana Grill <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#66}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@8130669
targos added a commit to targos/node that referenced this issue Apr 27, 2021
Original commit message:

    [macro-assembler] Avoid using the isolate in CallRecordWriteStub

    CallRecordWriteStub is used in a background compile thread for
    JS-to-Wasm wrapper compilation, so it should avoid accessing the
    isolate.
    Call the builtin using CallBuiltin which does not require a Handle<Code>
    object and instead gets the call target directly from the embedded data.

    R=​[email protected]

    (cherry picked from commit 6b3994e8507b32dfb956329395dbe33a2a8fee14)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1146813
    Change-Id: I4ee59084e4184f2e9039208e4e6db43482cefde6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2593333
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Thibaud Michaud <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#71785}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2731535
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Reviewed-by: Jana Grill <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#66}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@d2283ba
targos added a commit to targos/node that referenced this issue Apr 27, 2021
Original commit message:

    [macro-assembler] Avoid using the isolate in CallRecordWriteStub

    CallRecordWriteStub is used in a background compile thread for
    JS-to-Wasm wrapper compilation, so it should avoid accessing the
    isolate.
    Call the builtin using CallBuiltin which does not require a Handle<Code>
    object and instead gets the call target directly from the embedded data.

    R=​[email protected]

    (cherry picked from commit 6b3994e8507b32dfb956329395dbe33a2a8fee14)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1146813
    Change-Id: I4ee59084e4184f2e9039208e4e6db43482cefde6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2593333
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Thibaud Michaud <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#71785}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2731535
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Reviewed-by: Jana Grill <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#66}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@8130669
targos added a commit that referenced this issue Apr 30, 2021
Original commit message:

    [macro-assembler] Avoid using the isolate in CallRecordWriteStub

    CallRecordWriteStub is used in a background compile thread for
    JS-to-Wasm wrapper compilation, so it should avoid accessing the
    isolate.
    Call the builtin using CallBuiltin which does not require a Handle<Code>
    object and instead gets the call target directly from the embedded data.

    R=​[email protected]

    (cherry picked from commit 6b3994e8507b32dfb956329395dbe33a2a8fee14)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1146813
    Change-Id: I4ee59084e4184f2e9039208e4e6db43482cefde6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2593333
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Thibaud Michaud <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#71785}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2731535
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Reviewed-by: Jana Grill <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#66}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@d2283ba

PR-URL: #38275
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
targos added a commit that referenced this issue Apr 30, 2021
Original commit message:

    [macro-assembler] Avoid using the isolate in CallRecordWriteStub

    CallRecordWriteStub is used in a background compile thread for
    JS-to-Wasm wrapper compilation, so it should avoid accessing the
    isolate.
    Call the builtin using CallBuiltin which does not require a Handle<Code>
    object and instead gets the call target directly from the embedded data.

    R=​[email protected]

    (cherry picked from commit 6b3994e8507b32dfb956329395dbe33a2a8fee14)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1146813
    Change-Id: I4ee59084e4184f2e9039208e4e6db43482cefde6
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2593333
    Reviewed-by: Clemens Backes <[email protected]>
    Commit-Queue: Thibaud Michaud <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#71785}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2731535
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Reviewed-by: Jana Grill <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#66}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@8130669

PR-URL: #38275
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
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.
Projects
None yet
Development

No branches or pull requests