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 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.
@addaleax addaleax force-pushed the addaleax:fix-process-apis-switch branch from 6e65b57 to c13bb0f Jan 3, 2020
@jasnell
jasnell approved these changes Jan 3, 2020
@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Jan 3, 2020

@Trott
Trott approved these changes Jan 4, 2020
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 4, 2020

@cjihrig
cjihrig approved these changes Jan 4, 2020
Copy link
Member

joyeecheung left a comment

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()');

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Jan 5, 2020

Member

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)

This comment has been minimized.

Copy link
@Trott

Trott Jan 6, 2020

Member

Should this be changed before landing?

This comment has been minimized.

Copy link
@addaleax

addaleax Jan 6, 2020

Author Member

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

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 6, 2020

Landed in 20fd123

@Trott Trott closed this Jan 6, 2020
Trott added 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 added 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 addaleax:fix-process-apis-switch branch Jan 9, 2020
targos added 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 added 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.