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

domain: Make .domain on async resources non-enumerable #26210

Closed
wants to merge 1 commit into from

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Feb 20, 2019

In particular, this comes into play in the node repl, which apparently enables domains by default. Whenever any Promise gets inspected, a .domain property is displayed, which is very confusing, especially since it has some kind of WeakReference attached to it, which is not yet a language feature.

This change will prevent it from showing up in casual inspection, but will leave it available for use.

I have not run the tests locally yet, because they're incredibly slow and I need to do other things with my machine, and because the last time I tried some of them failed on master anyways.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

cc @addaleax @devsnek

In particular, this comes into play in the node repl, which apparently
enables domains by default. Whenever any Promise gets inspected, a
`.domain` property is displayed, which is *very confusing*, especially
since it has some kind of WeakReference attached to it, which is not yet
a language feature.

This change will prevent it from showing up in casual inspection, but
will leave it available for use.
@nodejs-github-bot nodejs-github-bot added the domain Issues and PRs related to the domain subsystem. label Feb 20, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM

/cc @BridgeAR

@ljharb ljharb force-pushed the domain_non_enumerable branch 2 times, most recently from 9d0013e to ed34d24 Compare February 20, 2019 00:46
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. It would be nice to add a test to it as well.

@ljharb ljharb force-pushed the domain_non_enumerable branch 2 times, most recently from b233a06 to 084f7b2 Compare February 22, 2019 07:40
@MylesBorins
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/20960/

@Trott
Copy link
Member

Trott commented Feb 22, 2019

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20973/

@ljharb ljharb force-pushed the domain_non_enumerable branch 2 times, most recently from 11c1de7 to caa10ed Compare February 27, 2019 04:02
@addaleax
Copy link
Member

addaleax commented Mar 1, 2019

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM!

Does this need tests?

What's the semver status of this? I don't think this is a major, but is it a minor or patch?

@ljharb
Copy link
Member Author

ljharb commented Mar 1, 2019

I’d call it a patch; imo it fixes confusion in the repl.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

@ljharb would you mind adding a test?

@Trott
Copy link
Member

Trott commented Mar 5, 2019

I have not run the tests locally yet, because they're incredibly slow and I need to do other things with my machine, and because the last time I tried some of them failed on master anyways.

@ljharb If you want to only run domain tests, you can do tools/test.py domain. On my machine, that runs 50 test files in 11 seconds. (No failures on master branch.)

@Trott
Copy link
Member

Trott commented Mar 5, 2019

@ljharb
Copy link
Member Author

ljharb commented Mar 5, 2019

@BridgeAR I've added some; let me know if that's not what you had in mind :-)

@Trott

no dice :-/
$ tools/test.py domain
Traceback (most recent call last):
  File "tools/test.py", line 1766, in <module>
    sys.exit(Main())
  File "tools/test.py", line 1639, in Main
    vm = context.GetVm(arch, mode)
  File "tools/test.py", line 953, in GetVm
    raise ValueError('Could not find executable. Should be ' + name)
ValueError: Could not find executable. Should be out/Release/node

@addaleax

This comment has been minimized.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

@ljharb you have to compile Node.js first. Otherwise the tests can't run.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

@ljharb I just started a CI and it seems like there are linter errors.

@ljharb
Copy link
Member Author

ljharb commented Mar 6, 2019

@BridgeAR thanks, fixed.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Do we really need all of these defineProperty calls? If I am not mistaken we only have to set the property to enumerable false once and every following assignment will keep the descriptor in place.

@ljharb
Copy link
Member Author

ljharb commented Mar 10, 2019

@BridgeAR certainly you're right, but for all of the objects that aren't previously defined - like newly created resources, errors, event emitters, etc - it'd need the defineProperty every time.

lib/domain.js Outdated Show resolved Hide resolved
lib/domain.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member Author

ljharb commented Mar 11, 2019

@BridgeAR updated

@devsnek
Copy link
Member

devsnek commented Mar 12, 2019

CI https://ci.nodejs.org/job/node-test-pull-request/21481/ (:white_check_mark:)

@MylesBorins
Copy link
Contributor

@misterdjules do you have any thoughts on this?

@ljharb
Copy link
Member Author

ljharb commented Mar 12, 2019

As far as semver - this could be considered semver-major if someone was using the domains flag, and also enumerating an async resource, and then relying on domain to be present there - but I can't imagine why anyone would be doing that. That would make this either a major or a patch, depending on the level of conservatism.

@devsnek devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2019
@addaleax
Copy link
Member

Landed in 377c583 :)

@addaleax addaleax closed this Mar 13, 2019
addaleax pushed a commit that referenced this pull request Mar 13, 2019
In particular, this comes into play in the node repl, which apparently
enables domains by default. Whenever any Promise gets inspected, a
`.domain` property is displayed, which is *very confusing*, especially
since it has some kind of WeakReference attached to it, which is not yet
a language feature.

This change will prevent it from showing up in casual inspection, but
will leave it available for use.

PR-URL: #26210
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@ljharb ljharb deleted the domain_non_enumerable branch March 13, 2019 06:00
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
In particular, this comes into play in the node repl, which apparently
enables domains by default. Whenever any Promise gets inspected, a
`.domain` property is displayed, which is *very confusing*, especially
since it has some kind of WeakReference attached to it, which is not yet
a language feature.

This change will prevent it from showing up in casual inspection, but
will leave it available for use.

PR-URL: #26210
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 14, 2019
In particular, this comes into play in the node repl, which apparently
enables domains by default. Whenever any Promise gets inspected, a
`.domain` property is displayed, which is *very confusing*, especially
since it has some kind of WeakReference attached to it, which is not yet
a language feature.

This change will prevent it from showing up in casual inspection, but
will leave it available for use.

PR-URL: nodejs#26210
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
In particular, this comes into play in the node repl, which apparently
enables domains by default. Whenever any Promise gets inspected, a
`.domain` property is displayed, which is *very confusing*, especially
since it has some kind of WeakReference attached to it, which is not yet
a language feature.

This change will prevent it from showing up in casual inspection, but
will leave it available for use.

PR-URL: #26210
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@misterdjules
Copy link

@MylesBorins This is great!

I ran into this recently when logging restify errors created from within a domain and we had to filter the domain property out. Having that property not enumerable would have avoided that issue in the first place.

And thanks for pinging me here, very much appreciated ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. domain Issues and PRs related to the domain subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.