-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
worker: allow copied NODE_OPTIONS in the env setting #53596
base: main
Are you sure you want to change the base?
Conversation
src/node_worker.cc
Outdated
USE(env->env_vars()->Get("NODE_OPTIONS").To(&parent_node_options)); | ||
if (args[1]->IsObject() && | ||
env_vars->Get("NODE_OPTIONS").To(&node_options) && | ||
node_options != parent_node_options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worker will ignore valid per-env options in NODE_OPTIONS
when the process was started with a same NODE_OPTIONS
.
#!/usr/bin/env NODE_OPTIONS=--trace-exit ./out/Debug/node
const { Worker, isMainThread } = require('worker_threads')
if (isMainThread) {
new Worker(__filename, {
env: {
NODE_OPTIONS: '--trace-exit'
}
})
} else {
setTimeout(() => {
process.exit(1)
}, 1000)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, it still needs to be parsed to mutate the per_isolate_opts. Maybe we should just check in the error branch though that do require a pretty dumb error.ends_with("is not allowed in NODE_OPTIONS")
check for all the errors..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..or probably just switch to kDisallowedInEnvvar
if the NODE_OPTIONS are character-by-character equal with the parent one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to just wrap it around a throw-away PerProcessOption and added a test.
fd49c14
to
d60bc23
Compare
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.
d60bc23
to
780ade2
Compare
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 skips the validation if the NODE_OPTIONS in the env setting 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.
Refs: #41103