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: move initialization of APIs for changing process state #31172

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 3, 2020

Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 3, 2020

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

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 3, 2020
@Trott
Copy link
Member

Trott commented Jan 4, 2020

@joyeecheung

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

function wrappedUmask(mask) {
// process.umask() is a read-only operation in workers.
if (mask !== undefined) {
throw new ERR_WORKER_UNSUPPORTED_OPERATION('Setting process.umask()');
Copy link
Member

Choose a reason for hiding this comment

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

Note: this reads Setting process.umask() is not supported in workers, apart from that we need to get rid of the worker part from the message, maybe calling process.umask() as a setter would be less ambiguous (I thought it was about process.umask = something from the first glance)

Copy link
Member

Choose a reason for hiding this comment

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

Should this be changed before landing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t think so – it’s unrelated to the changes here.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jan 6, 2020

Landed in 20fd123

@Trott Trott closed this Jan 6, 2020
Trott pushed a commit that referenced this pull request Jan 6, 2020
Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

PR-URL: #31172
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jan 6, 2020
Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

PR-URL: #31172
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
@addaleax addaleax deleted the fix-process-apis-switch branch January 9, 2020 07:52
targos pushed a commit that referenced this pull request Jan 14, 2020
Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

PR-URL: #31172
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

PR-URL: #31172
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Whether these APIs should be available for Node.js instances
semantically depends on whether the current Node.js instance
was assigned ownership of process-wide state, and not whether
it refers to the main thread or not.

PR-URL: #31172
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
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. 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.

8 participants