Skip to content

Conversation

@cjihrig
Copy link
Contributor

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-botnodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jan 19, 2019
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
ContributorAuthor

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
ContributorAuthor

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.

This comment was marked as resolved.

Copy link
Member

@joyeecheungjoyeecheungJan 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

@addaleaxaddaleax 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?

@addaleaxaddaleax 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
ContributorAuthor

There are some tests to fix here first.

@cjihrig
Copy link
ContributorAuthor

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

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

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

@cjihrig
Copy link
ContributorAuthor

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: #25587Fixes: #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]>
@cjihrigcjihrig closed this Feb 5, 2019
@cjihrigcjihrig deleted the workers branch February 5, 2019 23:56
@addaleax
Copy link
Member

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

processIssues and PRs related to the process subsystem.workerIssues and PRs related to Worker support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@cjihrig@nodejs-github-bot@Fishrock123@addaleax@refack@fhinkel@jasnell@joyeecheung@targos