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 #75

Closed
wants to merge 2 commits into from
Closed

Deprecate domains #75

wants to merge 2 commits into from

Conversation

tellnes
Copy link
Contributor

@tellnes tellnes commented Dec 5, 2014

ref #66

Also, I did notice that domain.dispose() is deprecated but missing a deprecation message. So it includes a commit which fixes it. Please inform me if you want it as a separate pull request.

I've also changed two test assertions. I belive usage of domain in lib/repl.js was swallowing the assert errors and that those are currently broken.

@tellnes tellnes mentioned this pull request Dec 5, 2014
@@ -290,4 +290,5 @@ Domain.prototype.dispose = util.deprecate(function() {
// mark this domain as 'no longer relevant'
// so that it can't be entered or activated.
this._disposed = true;
});
}, 'domain.dispose() is deprecated. Please recover from failed IO actions ' +
'explicitly via error event handlers set on the domain.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the advice we want to give?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is copied directly from the documentation in doc/api/domain.markdown (d86814a)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch. Can you amend the commit log to mention that?

EDIT: On a general note, commit logs should explain what changed and why. One-liners are almost never sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some more info to the commit message.

@tellnes tellnes force-pushed the domain branch 2 times, most recently from 53ed3ec to 6805268 Compare December 5, 2014 21:40
@bnoordhuis
Copy link
Member

The PR mostly LGTM. The commit logs could perhaps still use some love and it would be good to have closure on the failing test but the basic approach seems sound.

I'd like to get one more LGTM. @chrisdickinson or @cjihrig Can one of you take a look?

return true;
}
}
var asyncListener = tracing.createAsyncListener({ error: errorListener });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that tracing.createAsyncListener() was going away as part of @trevnorris's work on removing the JS-side AsyncListener API. Am I incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, what is the correct api to use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The safest thing would be to stick with domains for now, perhaps with a workaround to allow the REPL to use them without printing a deprecation warning. If you felt ambitious, you might build a lightweight layer atop the C++ AsyncWrap implementation (which will still be exposed via process.binding). But since domains are just getting deprecated, not removed, it's fine to still be using them in core, in my opinion. @trevnorris, do you have thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about some code for the repl in process._fatalException in src/node.js?

Alternatively we can just listen to uncaughtException. This functionality is about not breaking the repl when there is an uncaughtException and instead just displaying the error to the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would lean more towards using process._fatalException than uncaughtException, because the latter is built on the former anyway. I'm sure a decent top-level error handler could be built atop _fatalException alone, but, out of curiosity: why not just leave the domains code in the REPL for now? It works fine in the repl, the domains code will still be there for a while, and as far as I know, many people near to core are still committed to the idea of coming up with an improved async error-handling solution for Node in the not too distant future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the domain code is still in the REPL. I've just changed it to only use domains if the user provides one (which is possible, but undocumented). It does not fallback to calling domain.create() which trigger the deprecation warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, how it is implemented now makes the REPL catch all uncaught exceptions. Maybe that should be possible to change with an option and be disabled by default if you are using the REPL as a library.

I think this is getting out of scope for this pull request. Maybe it should be addressed in its own? But it needs to be addressed before the deprecation can be merged.

@tellnes
Copy link
Contributor Author

tellnes commented Dec 6, 2014

About the test I've changed.

After some digging I've found out it actually was broken by 7afdba6.

I've changed the test to look like the following code. This fails on the current v0.12 branch which proves the error was swallowed.

testMe.complete('toSt', function(error, data) {
  try {
    assert.deepEqual(data, [['toString'], 'toSt']);
  } catch (err) {
    console.log('error');
    process.exit(1);
    return;
  }
  console.log('ok');
});

if (!exports.repl) {
exports.repl = repl;
tracing.addAsyncListener(asyncListener);
repl.on('exit', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just me being paranoid, but could we use .once here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but shouldn't that be guaranteed by the api that the exit code is only fired once? If not, there probably a lot of broken code out there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason to prefer .once() over .on() is that the former lets the function's closure object get garbage collected before the EventEmitter object is collected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll change it.

@tellnes
Copy link
Contributor Author

tellnes commented Dec 7, 2014

Ok. I've removed the REPL changes and just fixed it so it doesn't shows a deprecation warning. I think the REPL issues should be addressed on its own.

I'll wait until @trevnorris or some one else comments on the usage of tracing.createAsyncListener() before I'd do something more about the REPL.

@caineio
Copy link

caineio commented Dec 8, 2014

Hello!

I am pleased to see your valuable contribution to this project. Would you
please mind answering a couple of questions to help me classify this submission
and/or gather required information for the core team members?

Questions:

  1. Which part of core do you think it might be related to?
    One of: debugger, http, assert, buffer, child_process, cluster, crypto, dgram, dns, domain, events, fs, http, https, module, net, os, path, querystring, readline, repl, smalloc, stream, timers, tls, url, util, vm, zlib, c++, docs, other (label)
  2. Which versions of io.js do you think are affected by this?
    One of: v0.10, v0.12, v1.0.0 (label)
  3. PR-only Does make test pass after applying this Pull Request.
    Expected: yes
  4. PR-only Is the commit message properly formatted? (See
    CONTRIBUTING.md for more information)
    Expected: yes

Please provide the answers in an ordered list like this:

  1. Answer for the first question
  2. Answer for the second question
  3. ...

Note that I am just a bot with a limited human-reply parsing abilities,
so please be very careful with numbers and don't skip the questions!

In case of success I will say: ...summoning the core team devs!.

In case of validation problem I will say: Sorry, but something is not right here:.

Truly yours,
Caine.

Responsibilities

  1. indutny: crypto, tls, https, child_process, c++
  2. trevnorris: buffer, http, https, smalloc
  3. bnoordhuis: http, cluster, child_process, dgram

@tellnes
Copy link
Contributor Author

tellnes commented Dec 8, 2014

  1. domain
  2. v0.12
  3. yes
  4. yes

@caineio
Copy link

caineio commented Dec 8, 2014

...summoning the core team devs!

@caineio caineio added v0.12 domain Issues and PRs related to the domain subsystem. and removed need info labels Dec 8, 2014
@bnoordhuis bnoordhuis added this to the v1.0.0 milestone Dec 9, 2014
@bnoordhuis
Copy link
Member

Ping @chrisdickinson, @cjihrig and @trevnorris. I'll bring this up in the TC meeting tomorrow and I'm assigning it to the v1.0.0 milestone so that we will have to make a decision before the first release.


child.stderr.setEncoding('utf8');
child.stderr.on('data', function (chunk) {
stderr += chunk;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just fail here and remove the child.on('exit', ...);

@cjihrig
Copy link
Contributor

cjihrig commented Dec 9, 2014

This LGTM besides one nit.

I'm in favor of deprecating domains, but has an replacement been discussed? For example, I've heard of (but not looked into whatsoever) StrongLoop's zones. Or, is the idea going forward not to have a replacement?

Deprecated in d86814a without a
deprecation message.

Deprecation message copied from documentation.
targos added a commit to targos/node that referenced this pull request Apr 17, 2021
Original commit message:

    [LTS-M86][compiler][x64] Fix bug in InstructionSelector::ChangeInt32ToInt64

    (cherry picked from commit 02f84c745fc0cae5927a66dc4a3e81334e8f60a6)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1196683
    Change-Id: Ib4ea738b47b64edc81450583be4c80a41698c3d1
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2820971
    Commit-Queue: Georg Neis <[email protected]>
    Reviewed-by: Nico Hartmann <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#73903}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2821959
    Commit-Queue: Jana Grill <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Reviewed-by: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#75}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@3066b7b
targos added a commit to targos/node that referenced this pull request Apr 27, 2021
Original commit message:

    [LTS-M86][compiler][x64] Fix bug in InstructionSelector::ChangeInt32ToInt64

    (cherry picked from commit 02f84c745fc0cae5927a66dc4a3e81334e8f60a6)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1196683
    Change-Id: Ib4ea738b47b64edc81450583be4c80a41698c3d1
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2820971
    Commit-Queue: Georg Neis <[email protected]>
    Reviewed-by: Nico Hartmann <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#73903}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2821959
    Commit-Queue: Jana Grill <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Reviewed-by: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#75}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@3066b7b
targos added a commit that referenced this pull request Apr 30, 2021
Original commit message:

    [LTS-M86][compiler][x64] Fix bug in InstructionSelector::ChangeInt32ToInt64

    (cherry picked from commit 02f84c745fc0cae5927a66dc4a3e81334e8f60a6)

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: chromium:1196683
    Change-Id: Ib4ea738b47b64edc81450583be4c80a41698c3d1
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2820971
    Commit-Queue: Georg Neis <[email protected]>
    Reviewed-by: Nico Hartmann <[email protected]>
    Cr-Original-Commit-Position: refs/heads/master@{#73903}
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2821959
    Commit-Queue: Jana Grill <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Reviewed-by: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#75}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@3066b7b

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

Successfully merging this pull request may close these issues.

8 participants