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

Missing process functions under workers should throw explicit error #25448

Closed
SimenB opened this issue Jan 11, 2019 · 5 comments
Closed

Missing process functions under workers should throw explicit error #25448

SimenB opened this issue Jan 11, 2019 · 5 comments
Labels
feature request Issues that request new features to be added to Node.js. process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support.

Comments

@SimenB
Copy link
Member

SimenB commented Jan 11, 2019

Is your feature request related to a problem? Please describe.
Jest calls mkdirp during its unit tests. Jest has also recently added support for worker_threads. However, when I wanted to try it out, hundreds of tests failed with TypeError: process.umask is not a function

It turn out mkdirp uses process.umask() if no mode is provided: https://github.com/substack/node-mkdirp/blob/f2003bbcffa80f8c9744579fabab1212fc84545a/index.js#L64

I've since found that this function missing in worker_threads is well documented: https://nodejs.org/api/worker_threads.html#worker_threads_class_worker

process.chdir() and process methods that set group or user ids are not available.

Describe the solution you'd like
If node could throw "process.umask is not available in worker threads" or similar it would have made my debugging way easier, rather than the function just be missing.

I'm not sure how that would play with typeof process.umask === 'function' checks people might have?

Describe alternatives you've considered
My solution was to pass 777 as mode explicitly to mkdirp, but a clearer error would have made the debugging way easier.

My use case might be a bit special since Jest reconstructs a fake process object for every single test (by inspecting the real one), so my rabbit hole before checking worker docs were probably deeper than most people in the same situation. I was also testing node 12 (where threads are unflagged), so I wasn't even aware I was running in threads at first.

@bnoordhuis bnoordhuis added feature request Issues that request new features to be added to Node.js. process Issues and PRs related to the process subsystem. labels Jan 14, 2019
@bnoordhuis
Copy link
Member

I think the fact that it breaks feature detection means this proposal is DOA. Maybe other collaborators feel differently?

@targos
Copy link
Member

targos commented Jan 14, 2019

It sounds reasonable to me to have a version of process.umask in worker threads that can only get the value and not modify it.

@SimenB
Copy link
Member Author

SimenB commented Jan 14, 2019

It sounds reasonable to me to have a version of process.umask in worker threads that can only get the value and not modify it.

That'd certainly be nice! That would help in mkdirp's case, but not necessarily in other cases, such as missing process.chdir.


Would it be possible to get a list of missing APIs from process? Then we could throw an error in environments where we want to warn (like potentially Jest). E.g.

const { removedProcessFunctions, isMainThread } = require('worker_threads');

if (!isMainThread) {
  removedProcessFunctions.forEach(func => {
    Object.defineProperty(process, func, {
      value: () => {
        throw new Error(func + ' is not available inside a worker');
      }
    });
  });
}

Not sure how that plays with feature detection either (or how prevalent feature detection on process is)

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

cjihrig commented Jan 14, 2019

Not sure how that plays with feature detection either (or how prevalent feature detection on process is)

It would break detection. However, since worker threads are still experimental, I think it makes sense to add worker-specific stubs that throw.

cjihrig added a commit to cjihrig/node that referenced this issue Jan 17, 2019
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>
addaleax pushed a commit that referenced this issue Jan 23, 2019
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>
@cjihrig cjihrig closed this as completed in 12b3cfc Feb 5, 2019
@SimenB
Copy link
Member Author

SimenB commented Feb 6, 2019

Thank you! 🎉

targos pushed a commit that referenced this issue Feb 10, 2019
Some process methods are not supported in workers. This commit
adds stubs that throw more informative errors.

PR-URL: #25587
Fixes: #25448
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Development

No branches or pull requests

5 participants