FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: add `process` to internal module wrapper by addaleax · Pull Request #17198 · nodejs/node · GitHub

/ node Public

lib: add process to internal module wrapper - #17198

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:process-internal
ConversationCommits (1)ChecksFiles changed
Closed

lib: add process to internal module wrapper#17198
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:process-internal

Conversation

addaleax commented Nov 21, 2017

Copy link
Copy Markdown
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)

lib: add process to internal module wrapper
a5511e4 
Share `process` through the module wrapper rather than relying
on nobody messing with `global.process`.

Fixes: nodejs#6802
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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 21, 2017
addaleax mentioned this pull request Nov 21, 2017
Closed
bnoordhuis approved these changes Nov 21, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax commented Nov 21, 2017

Copy link
Copy Markdown
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…

jasnell approved these changes Nov 21, 2017
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lance approved these changes Nov 21, 2017
View reviewed changes

lance left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mscdex commented Nov 21, 2017

Copy link
Copy Markdown
Contributor

This should be semver-major IMHO.

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

addaleax commented Nov 21, 2017

Copy link
Copy Markdown
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 removed the repl Issues and PRs related to the REPL subsystem. label Nov 21, 2017

mscdex commented Nov 21, 2017

Copy link
Copy Markdown
Contributor

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

addaleax commented Nov 21, 2017

Copy link
Copy Markdown
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 commented Nov 21, 2017

Copy link
Copy Markdown
Contributor

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

jasnell commented Nov 21, 2017

Copy link
Copy Markdown
Member

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 commented Nov 21, 2017

Copy link
Copy Markdown
Member

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 27, 2017

addaleax commented Nov 27, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/14382/

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 28, 2017

addaleax commented Nov 28, 2017

Copy link
Copy Markdown
Member Author

Landed in e8a26e7

addaleax closed this Nov 28, 2017
addaleax deleted the process-internal branch November 28, 2017 01:57
addaleax added a commit that referenced this pull request Nov 28, 2017
lib: add process to internal module wrapper
e8a26e7 
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 commented Feb 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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 commented Feb 23, 2018

Copy link
Copy Markdown
Member

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

apapirovski commented Feb 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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...).

apapirovski mentioned this pull request Feb 23, 2018
Closed

targos commented Feb 23, 2018

Copy link
Copy Markdown
Member

+1 from me

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

addaleax commented Feb 23, 2018

Copy link
Copy Markdown
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
lib: add process to internal module wrapper
af42919 
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>
apapirovski mentioned this pull request Feb 26, 2018
Closed
4 tasks
addaleax added a commit that referenced this pull request Feb 26, 2018
lib: add process to internal module wrapper
337d529 
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
lib: add process to internal module wrapper
c7b66d0 
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
lib: add process to internal module wrapper
738b0a1 
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 mentioned this pull request Feb 27, 2018
Closed

MylesBorins commented Jul 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
+6 more reviewers
lance lance approved these changes
apapirovski apapirovski approved these changes
refack refack approved these changes
TimothyGu TimothyGu approved these changes
targos targos approved these changes
aqrln aqrln approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL