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.

Sign up for GitHub

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

Jump to bottom

process: allow reading umask in workers #25526

Merged
merged 1 commit into from
Jan 17, 2019
Merged

process: allow reading umask in workers #25526

merged 1 commit into from
Jan 17, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 15, 2019

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

@nodejs-github-bot
Copy link
Collaborator

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2258/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. labels Jan 15, 2019
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 16, 2019

cc @targos because of #25448 (comment)

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 16, 2019
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 16, 2019

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

addaleax
addaleax approved these changes Jan 17, 2019
View reviewed changes
lib/internal/process/worker_thread_only.js Outdated
// The execution of this function itself should not cause any side effects.
function wrapProcessMethods(binding) {
function umask(mask) {
// process.umask() is a read only operation in workers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// process.umask() is a read only operation in workers.
// process.umask() is a read-only operation in workers.

@cjihrig
process: allow reading umask in workers
f6cd4e3 
Refs: nodejs#25448
PR-URL: nodejs#25526
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 17, 2019

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

Addressed the "read only" -> "read-only" nit when adding the commit metadata.

@cjihrig cjihrig merged commit f6cd4e3 into nodejs:master Jan 17, 2019
@cjihrig cjihrig deleted the umask branch January 17, 2019 21:36
@addaleax addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 18, 2019
@addaleax
Copy link
Member

@cjihrig I’m removing semver-major because, as far as I can tell, this only affects the still-experimental workers?

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Jan 18, 2019
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 18, 2019

@addaleax that's fine. I was being overly conservative 😄

@SimenB SimenB mentioned this pull request Jan 22, 2019
Closed
@SimenB
Copy link
Member

SimenB commented Jan 22, 2019

Oh perfect! Will this be backported to v11 (maybe even v10, although since it's behind a flag there not as big impact)? I just filed sindresorhus/make-dir#9 (with 6M weekly downloads), but that'll be fixed by this landing 🙂

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 22, 2019

My guess is that it will end up in Node 10 and up.

@SimenB
Copy link
Member

SimenB commented Jan 22, 2019

Perfect, thanks for the info!

@SimenB SimenB mentioned this pull request Jan 22, 2019
Merged
addaleax pushed a commit that referenced this pull request Jan 23, 2019
@cjihrig @addaleax
process: allow reading umask in workers
07f1bb0 
Refs: #25448
PR-URL: #25526
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
MylesBorins added a commit that referenced this pull request Jan 24, 2019
@MylesBorins
2019-01-24, Version 11.8.0 (Current)
11ee794 
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687
MylesBorins added a commit that referenced this pull request Jan 25, 2019
@MylesBorins
2019-01-24, Version 11.8.0 (Current)
641de82 
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687
@dylburger
Copy link

I saw some chatter on this thread about backporting this to v10, and I wanted to check if that was still being considered.

We use v10 worker threads and are trying to use a specific package that only supports up to v10, a dependency of which is trying to call process.umask().

No worries if not, just thought I'd check in!

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 24, 2019

It doesn't look like it has been backported. I think you'd have to ask @nodejs/backporters if they intend to or not.

@dylburger
Copy link

@cjihrig awesome, thanks! That reference directs me to a 404 — is that a private team in the Node.js Github org? Not sure the correct process but happy to follow it.

@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 25, 2019

@dylburger that was just me pinging the team of people that handle most of the backporting of commits from master to the release branches. You don't have to do anything.

@rudolf rudolf mentioned this pull request Jun 19, 2020
Closed
7 tasks
@snyk-bot snyk-bot mentioned this pull request Aug 11, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @nodejs-github-bot @addaleax @SimenB @dylburger @jasnell @targos @joyeecheung @BridgeAR