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

worker: allow copied NODE_OPTIONS in the env setting #53596

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jun 26, 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 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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Jun 26, 2024
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2024
src/node_worker.cc Outdated Show resolved Hide resolved
@joyeecheung joyeecheung force-pushed the worker-option branch 2 times, most recently from fd49c14 to d60bc23 Compare June 27, 2024 21:28
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.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2024
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM but linter is complaining...

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

lgtm with lint fixes

@joyeecheung
Copy link
Member Author

joyeecheung commented Jul 4, 2024

Fixed the linter error and skipped the main thread process.title assertion on SmartOS because the option itself isn't supported there. The assertion is only there in case someone runs the test locally without --title and isn't essential to what this tries to implement here (that the worker can inherit the option as-is without erroring). --title is picked because it's the most straightforward to test among all the per-process options.

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2024
@joyeecheung
Copy link
Member Author

@legendecas @jasnell @meixg can you take a look again? Thanks!

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 5, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 5, 2024
@nodejs-github-bot nodejs-github-bot merged commit b32c722 into nodejs:main Jul 5, 2024
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b32c722

aduh95 pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
aduh95 pushed a commit that referenced this pull request 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 <jasnell@gmail.com>
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
5 participants