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

Stop throwing for unsupported options in Worker's execArgv and env.NODE_OPTIONS. #41103

Open
nicolo-ribaudo opened this issue Dec 6, 2021 · 12 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Dec 6, 2021

Version

v17.2.0

Platform

No response

Subsystem

worker_threads

What steps will reproduce the bug?

Consider this test.js file:

const { isMainThread, Worker } = require("worker_threads");

if (isMainThread) {
  new Worker(__filename, {
    // execArgv: process.execArgv
  });
} else {
  console.log("Hello from the worker!", process.execArgv);
}

NOTE: --experimental-json-modules and --title are just examples

Run these two commands:

node --experimental-json-modules test.js
node --experimental-json-modules --title foo test.js

They will log

Hello from the worker! [ '--experimental-json-modules' ]
Hello from the worker! [ '--experimental-json-modules', '--title', 'foo' ]

Now, uncomment the execArgv: process.execArgv line and run the two commands again. The first command will still log

Hello from the worker! [ '--experimental-json-modules' ]

however, the second one throws:

Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --title
    at new NodeError (node:internal/errors:371:5)
    at new Worker (node:internal/worker:191:13)
    at Object.<anonymous> (/home/nicolo/Documenti/dev/babel/babel/test.js:4:3)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1149:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  code: 'ERR_WORKER_INVALID_EXEC_ARGV'
}

This mans that the default behavior of the Worker execArgv option is not to inherit execArgv from the parent thread (contrary to what the documentation says).

Why would you want to explicitly pass execArgv: process.execArgv anyway?

A few days ago I was trying to implement a require hook that internally uses a Worker, and in order to avoid registering it recursively in an infinite worker-spawn loop I tried to filter out the --require flag:

new Worker(workerFile, {
  execArgv: process.execArgv.filter((val, i, argv) => val === "--requrie" || argv[i-1] === "--requrie"),
});

this seemed to work, until it crashed in a test that was using the --expose_gc option.

Today, I found the exact same problem in jest-worker (a Jest subpackage that is often also used outside of Jest): jestjs/jest#12103 was opened because jest-worker didn't work when running Node.js with --max_old_space_size.

Since Node.js doesn't provide the list of flags supported by workers progrmmatically (and I didn't find it in the docs either 😅) there is no way to filter the execArgv used by a worker.

What is the current behavior?

I initially expected the real default behavior to be something like execArgv: process.execArgv.filter(isSupportedByWorker), but it looks like it's not either. Consider for example the --expose_gc (which is disallowed in workers) option with this example:

const { isMainThread, Worker } = require("worker_threads");

if (isMainThread) {
  new Worker(__filename);
} else {
  console.log("Hello from the worker!", process.execArgv, globalThis.gc);
}

If you run node --expose_gc test.js, it will log

Hello from the worker! [ '--expose_gc' ] [Function: gc]

You can see that the gc function is present. This means that the disallowed flags are not ignored (and not even "ignored but still used to populate the process.execArgv array): they are used and affect the runtime behavior.

How often does it reproduce? Is there a required condition?

Only for the options marked as "unsupported" in workers.

What is the expected behavior?

I see four possible solutions:

  • Just ignore the unsupported options
  • Never throw, but warn when passing an unsupported option to a worker (potentially even if it's passed by default without an explicit execArgv).
  • Only throw if the unsupported option is not also used as an execArgv of the parent thread.
  • Always throw for unsupported options (so that execArgv: process.execArgv becomes the default behavior), and provide a Worker.filterExecArgv utility that removes disallowed options, and that I would have used like
    Worker.filterExecArgv(process.execArgv).filter((val, i, argv) => val === "--requrie" || argv[i-1] === "--requrie")

If the current behavior is expected, then the docs should be updated to mention that the default value cannot be replicated by explicitly passing an execArgv option.

What do you see instead?

No response

Additional information

No response

@targos
Copy link
Member

targos commented Dec 7, 2021

@nodejs/workers

@Mesteery Mesteery added the worker Issues and PRs related to Worker support. label Dec 7, 2021
@nicolo-ribaudo nicolo-ribaudo changed the title Stop throwing for unsupported options in Worker's execArgv. Stop throwing for unsupported options in Worker's execArgv and env.NODE_OPTIONS. Dec 11, 2021
@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented Dec 11, 2021

The same happens with env.

When running NODE_OPTIONS=--openssl-legacy-provider node test.js with Node.js 17.2.0, this works:

const { Worker, isMainThread } = require("worker_threads");

if (isMainThread) {
  new Worker(__filename);
} else {
  console.log("From worker!", process.env.NODE_OPTIONS);
}

but this throws

const { Worker, isMainThread } = require("worker_threads");

if (isMainThread) {
  new Worker(__filename, { env: process.env });
} else {
  console.log("From worker!", process.env.NODE_OPTIONS);
}

@meixg
Copy link
Member

meixg commented Feb 28, 2022

You can see that the gc function is present. This means that the disallowed flags are not ignored (and not even "ignored but still used to populate the process.execArgv array): they are used and affect the runtime behavior.

This may be not right, as:

const { isMainThread, Worker } = require("worker_threads");

if (isMainThread) {
  new Worker(__filename, {
    execArgv: []
  });
} else {
  console.log("Hello from the worker!", process.execArgv, globalThis.gc);
}

run node --expose_gc test.js will log:

Hello from the worker! [] [Function: gc]

Seems that this behavior is not controlled by work's execArgv.

@meixg
Copy link
Member

meixg commented Feb 28, 2022

If execArgv is not specified, all parent's args indeed are used:

node/src/node_worker.cc

Lines 551 to 553 in 4cb2a47

} else {
exec_argv_out = env->exec_argv();
}

node/src/node_worker.cc

Lines 308 to 315 in 4cb2a47

env_.reset(CreateEnvironment(
data.isolate_data_.get(),
context,
std::move(argv_),
std::move(exec_argv_),
static_cast<EnvironmentFlags::Flags>(environment_flags_),
thread_id_,
std::move(inspector_parent_handle_)));

But since these Per-Process options and V8 flags are from parents, I guess it is ok?

#25467

@jimmywarting
Copy link

jimmywarting commented May 9, 2023

this have for sure been bugging me too, some flags gets added or removed in different versions.
one needs to be able to figure out what flags are supported or not before you can pass this execArgv commands

@shellscape
Copy link

Just ran into this using ava avajs/ava#3207

@aaronovz1
Copy link

What's the word on this? Just hit this on taskforcesh/bullmq#2075 which is a pain because without a change to that package, worker_threads can't be used.

@bcomnes
Copy link

bcomnes commented May 24, 2024

+1 for Ignore the unsupported options

@joyeecheung
Copy link
Member

Opened #53596 for the simpler case of NODE_OPTIONS, when the worker spawning code does not intend to modify NODE_OPTIONS and only copies the one from the parent because they want to update other environment variables.

RedYetiDev pushed a commit to RedYetiDev/node that referenced this issue Jul 5, 2024
When the worker spawning code copies NODE_OPTIONS from process.env,
previously we do a check again on the copied NODE_OPTIONS and throw
if it contains per-process settings. This can be problematic
if the end user starts the process with a NODE_OPTIONS and the
worker is spawned by a third-party that tries to extend process.env
with some overrides before passing them into the worker. This patch
adds another exception that allows the per-process options in the
NODE_OPTIONS passed to a worker if the NODE_OPTIONS is
character-by-character equal to the parent NODE_OPTIONS.
While some more intelligent filter can be useful too,
this works good enough for the inheritance case, when the worker
spawning code does not intend to modify NODE_OPTIONS.

PR-URL: nodejs#53596
Refs: nodejs#41103
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
aduh95 pushed a commit that referenced this issue Jul 12, 2024
When the worker spawning code copies NODE_OPTIONS from process.env,
previously we do a check again on the copied NODE_OPTIONS and throw
if it contains per-process settings. This can be problematic
if the end user starts the process with a NODE_OPTIONS and the
worker is spawned by a third-party that tries to extend process.env
with some overrides before passing them into the worker. This patch
adds another exception that allows the per-process options in the
NODE_OPTIONS passed to a worker if the NODE_OPTIONS is
character-by-character equal to the parent NODE_OPTIONS.
While some more intelligent filter can be useful too,
this works good enough for the inheritance case, when the worker
spawning code does not intend to modify NODE_OPTIONS.

PR-URL: #53596
Refs: #41103
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
aduh95 pushed a commit that referenced this issue Jul 16, 2024
When the worker spawning code copies NODE_OPTIONS from process.env,
previously we do a check again on the copied NODE_OPTIONS and throw
if it contains per-process settings. This can be problematic
if the end user starts the process with a NODE_OPTIONS and the
worker is spawned by a third-party that tries to extend process.env
with some overrides before passing them into the worker. This patch
adds another exception that allows the per-process options in the
NODE_OPTIONS passed to a worker if the NODE_OPTIONS is
character-by-character equal to the parent NODE_OPTIONS.
While some more intelligent filter can be useful too,
this works good enough for the inheritance case, when the worker
spawning code does not intend to modify NODE_OPTIONS.

PR-URL: #53596
Refs: #41103
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@kirklimbu
Copy link

I came across this error while building angular 18 app. I solved it by disabling SSR "prerender": false, "ssr": false inside project.json file

@alirezaraha3
Copy link

Game_spiking @dance _

marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
When the worker spawning code copies NODE_OPTIONS from process.env,
previously we do a check again on the copied NODE_OPTIONS and throw
if it contains per-process settings. This can be problematic
if the end user starts the process with a NODE_OPTIONS and the
worker is spawned by a third-party that tries to extend process.env
with some overrides before passing them into the worker. This patch
adds another exception that allows the per-process options in the
NODE_OPTIONS passed to a worker if the NODE_OPTIONS is
character-by-character equal to the parent NODE_OPTIONS.
While some more intelligent filter can be useful too,
this works good enough for the inheritance case, when the worker
spawning code does not intend to modify NODE_OPTIONS.

PR-URL: #53596
Refs: #41103
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
When the worker spawning code copies NODE_OPTIONS from process.env,
previously we do a check again on the copied NODE_OPTIONS and throw
if it contains per-process settings. This can be problematic
if the end user starts the process with a NODE_OPTIONS and the
worker is spawned by a third-party that tries to extend process.env
with some overrides before passing them into the worker. This patch
adds another exception that allows the per-process options in the
NODE_OPTIONS passed to a worker if the NODE_OPTIONS is
character-by-character equal to the parent NODE_OPTIONS.
While some more intelligent filter can be useful too,
this works good enough for the inheritance case, when the worker
spawning code does not intend to modify NODE_OPTIONS.

PR-URL: #53596
Refs: #41103
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Xuguang Mei <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@everett1992
Copy link
Contributor

I ran into this when building an old project that uses mocha-parallel-test and node's --openssl-legacy-provider option.
mocha-parallel-tests spawns workers and specifies execArgv to remove some 'debug' flags from the process execArgv. But it doesn't remove openssl-legacy-provider so node throws Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --openssl-legacy-provider

https://github.com/mocha-parallel/mocha-parallel-tests/blob/master/src/main/thread/worker.ts#L35-L36

Either node should ignore these options by default, as it appears to when using the default execArgv, or node should provide a function to remove unsupported options, or return process.execArgv without per-process arguments.

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

No branches or pull requests