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

process: stub unsupported worker methods #25587

Closed
wants to merge 0 commits into from
Closed

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 19, 2019

Some process methods are not supported in workers. This commit adds stubs that throw more informative errors.

Fixes: #25448

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

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jan 19, 2019
function unavailable(name) {
return function unavailableInWorker() {
throw new ERR_WORKER_UNSUPPORTED_OPERATION(name);
};
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion, but maybe add a disabled: true property on the function, so that feature detection remains about as easy as it is right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add that.

Follow up question. Isn't using isMainThread the preferable way to detect if you're running in a worker though?

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Follow up question. Isn't using isMainThread the preferable way to detect if you're running in a worker though?

@cjihrig Yes, I’d say so.

Also, fyi, we may want to provide a way in our embedder API to create a Worker-like environment, i.e. with access to per-process data disabled or restricted like we do for Workers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@refack I stuck with unavailable() just because it's a bit shorter and required fewer uses to be wrapped across multiple lines.

function unavailable(name) {
return function unavailableInWorker() {
throw new ERR_WORKER_UNSUPPORTED_OPERATION(name);
};

This comment was marked as resolved.

mainThreadSetup.setupChildProcessIpcChannel();
} else {
process.channel = null;
process.connected = false;
Copy link
Member

@joyeecheung joyeecheung Jan 19, 2019

Choose a reason for hiding this comment

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

I am not sure if these values make sense in the context of workers...what if the process (and the main thread) was spawned with child_process? Maybe we could just throw errors in getters for all these if they just do not make sense (or are too difficult to implement properly) for workers? As far as I can tell, if a user needs to be prepare for the cases where these properties are not available, they are probably just being prepared for the workers, and could use something like require('module').builtinModules.includes('worker_threads') && require('worker_threads').isMainThread as guards instead. Throwing in the getters should also help with debugging issues like #25448 .

@Fishrock123
Copy link
Contributor

Is this semver major? If they didn't exist, could someone be null-checking the properties to see if they can be called? (I doubt anyone is, but...)

@addaleax
Copy link
Member

@Fishrock123 Workers are still experimental so I think we’re good here

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Jan 28, 2019
@addaleax
Copy link
Member

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

I guess this is technically ready?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2019
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 29, 2019

There are some tests to fix here first.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 29, 2019

Oh, and I've also been thinking about if I really want to take the approach in this PR or not.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2019
@cjihrig cjihrig force-pushed the workers branch 3 times, most recently from 886b04b to e5f10ec Compare February 5, 2019 20:00
@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 5, 2019

Fixed the test issues. Green CI: https://ci.nodejs.org/job/node-test-pull-request/20600/

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved
@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 5, 2019

One last green CI: https://ci.nodejs.org/job/node-test-commit/25637/

cjihrig added a commit that referenced this pull request Feb 5, 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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig closed this Feb 5, 2019
@cjihrig cjihrig deleted the workers branch February 5, 2019 23:56
@addaleax
Copy link
Member

addaleax commented Feb 6, 2019

I’m adding the backport-requested-v11.x label, but other PRs probably just need to be backported first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants