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

workers: --require module injects module to worker thread #28518

Closed
alexkozy opened this issue Jul 3, 2019 · 7 comments
Closed

workers: --require module injects module to worker thread #28518

alexkozy opened this issue Jul 3, 2019 · 7 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@alexkozy
Copy link
Member

alexkozy commented Jul 3, 2019

  • Version: v12.5.0
  • Platform: Mac
  • Subsystem: worker_threads

Steps to reproduce:

  1. Create two scripts, a.js:
const { Worker } = require('worker_threads');
const worker = new Worker('console.log(42)', { eval: true, data: {} });

and b.js:

const { isMainThread } = require('worker_threads');
console.log('b', isMainThread);
  1. Run node --require b.js a.js
  2. Take a look on output:
b true
b false
42

I am wondering is it feature or bug that we inject --require module to workers. If it is feature, should we mention it in workers doc and in --require flag description?

In my use case - I use --require to implement inspector socket discovery in user land - I need --require only for main thread. I can workaround it on my side but do we need some additional flag that will inject to workers instead of reusing existing flag.

@alexkozy
Copy link
Member Author

alexkozy commented Jul 3, 2019

cc @addaleax

@devsnek devsnek added the worker Issues and PRs related to Worker support. label Jul 3, 2019
@devsnek
Copy link
Member

devsnek commented Jul 3, 2019

i wouldn't expect this to happen, but it's not exactly a leap to want this behaviour either...

maybe we can add a flag to workers to disallow loading these (default enabled)

@cjihrig
Copy link
Contributor

cjihrig commented Jul 3, 2019

Just noting that both child_process.fork() and the cluster module behave this way as well.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 3, 2019

Is this really not expected? This seems to me as if it would be working as intended.

@Fishrock123
Copy link
Contributor

The documentation is, unfortunately, not very clear about side-effects: https://nodejs.org/dist/latest-v12.x/docs/api/cli.html#cli_r_require_module

@addaleax
Copy link
Member

addaleax commented Jul 3, 2019

From the Worker constructor options documentation:

execArgv {string[]} List of node CLI options passed to the worker. V8 options […] are not supported. If set, this will be provided as [process.execArgv][] inside the worker. By default, options will be inherited from the parent thread.

So, yes, inheriting CLI flags is intentional, and there is a way to override this, and I would say that this seems like a documentation visibility issue than a bug?

@alexkozy
Copy link
Member Author

alexkozy commented Jul 3, 2019

So, yes, inheriting CLI flags is intentional, and there is a way to override this, and I would say that this seems like a documentation visibility issue than a bug?

Should we split process-only options (e.g., --title) and all other in two different groups in documentation? It will give me explicit signal where each option works. Since from my use case point of view --require should be process-only option, but in case of custom module loader it should be process and worker option - it might be source of confusion.

In my use case I am not controlling spawning workers or creating child processes, so I can not override it. I can workaround it on my side by checking for worker in required module but I prefer --process-require option that will inject module only to process or maybe some generic option modifier that can make any options process-only, e.g. --process-only --require a.js --proces-only --inspect.

HarshithaKP added a commit to HarshithaKP/node that referenced this issue Jan 16, 2020
MylesBorins pushed a commit that referenced this issue Mar 4, 2020
Fixes: #28518
PR-URL: #31380
Fixes: #28518
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this issue Mar 16, 2020
Fixes: #28518
PR-URL: #31380
Fixes: #28518
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this issue Mar 17, 2020
Fixes: #28518
PR-URL: #31380
Fixes: #28518
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this issue Mar 23, 2020
Fixes: #28518
PR-URL: #31380
Fixes: #28518
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this issue Mar 30, 2020
Fixes: #28518
PR-URL: #31380
Fixes: #28518
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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
worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants