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

lib: add process to internal module wrapper #17198

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Share process through the module wrapper rather than relying
on nobody messing with global.process.

Fixes: #6802

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

lib (but in particular the REPL)

Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Fixes: nodejs#6802
@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. repl Issues and PRs related to the REPL subsystem. labels Nov 21, 2017
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 21, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I have a curious sense of double deja vu: both of reviewing something like this before and of writing it myself...

@addaleax
Copy link
Member Author

@bnoordhuis I think we might have tried to add it to the userland module wrapper at some point, which broke existing code that contained statements like const process = ...; – not sure I remember correctly though…

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Code change LGTM.
Should this be defensively marked semver-major?
Definitely needs a CITGM run.

Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

LGTM - and to think I was going down the ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES rabbit hole. This is nice and clean.

@mscdex
Copy link
Contributor

mscdex commented Nov 21, 2017

This should be semver-major IMHO.

@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 21, 2017
@addaleax
Copy link
Member Author

@mscdex I don’t have a strong opinion personally, but would you mind giving a reason for that? What scenario could this break?

@mscdex mscdex removed the repl Issues and PRs related to the REPL subsystem. label Nov 21, 2017
@mscdex
Copy link
Contributor

mscdex commented Nov 21, 2017

@addaleax For people that may rely on global.process being changed?

@addaleax
Copy link
Member Author

@mscdex Changed to what? If the new value doesn’t have the properties as the initial process, that would basically always crash the process from Node’s internals sooner or later anyway, right?

@mscdex
Copy link
Contributor

mscdex commented Nov 21, 2017

Feel free to remove the label, but I generally like to be defensive about such changes...

@jasnell
Copy link
Member

jasnell commented Nov 21, 2017

I think being defensive is the safest route, if citgm comes up without any problematic hits then I've no problem dropping the label when this lands.

@jasnell
Copy link
Member

jasnell commented Nov 21, 2017

to be clear, @addaleax, I cannot think of a single reasonable case where this would break anyone, but there's no harm in being cautious.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2017
@addaleax
Copy link
Member Author

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
@addaleax
Copy link
Member Author

Landed in e8a26e7

@addaleax addaleax closed this Nov 28, 2017
@addaleax addaleax deleted the process-internal branch November 28, 2017 01:57
addaleax added a commit that referenced this pull request Nov 28, 2017
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@apapirovski
Copy link
Member

apapirovski commented Feb 23, 2018

@nodejs/tsc Can we unmake this semver-major per chance? It's actually impossible for Node to get through a single run of the event loop if process gets set to a new object, because we rely on process._tickCallback, process.nextTick and many other things in our internal code.

It's blocking landing #17736 on v9.x

@targos
Copy link
Member

targos commented Feb 23, 2018

@apapirovski Are you asking for removing the semver-major label or something else?

@apapirovski
Copy link
Member

apapirovski commented Feb 23, 2018

@targos Yes, I'm looking to remove the semver-major label. I think it's overly cautious and we haven't seen any breakages in CitGM on master as a result of this.

In addition to what I said above, if someone just modifies the process object with new properties then those changes will still be available. This merely changes the behaviour if global.process is assigned a new value (which breaks Node anyway...).

@targos
Copy link
Member

targos commented Feb 23, 2018

+1 from me

@addaleax addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 23, 2018
@addaleax
Copy link
Member Author

Removed the label for now, we can see if anybody has a differing opinion.

apapirovski pushed a commit to apapirovski/node that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

PR-URL: nodejs#17198
Fixes: nodejs#6802
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax added a commit that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Backport-PR-URL: #19006
PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax added a commit that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Backport-PR-URL: #19006
PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Backport-PR-URL: #19006
PR-URL: #17198
Fixes: #6802
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Feb 27, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 31, 2018

This does not land cleanly in v8.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

edit: the backport to 9.x came with a number of other PRs (see #19006) it landed in 743cf33...f2dd17b

likely we would want to backport all at once?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.