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: inherits mutated NODE_OPTIONS #44732

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Sep 20, 2022

Changes made to the NODE_OPTIONS environment variables weren't properly taken into account when spawning workers, unlike how child_process.fork works.

Fixes part of #37410 (I didn't touch the process.execArgv part, I was worried it'd be a breaking change otherwise).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Sep 20, 2022
@aduh95
Copy link
Contributor

aduh95 commented Sep 20, 2022

@arcanis It looks like there are CI failures to address.

/cc @nodejs/workers

@arcanis arcanis force-pushed the mael/worker-process-env branch from 05cb903 to e369572 Compare September 21, 2022 14:47
@arcanis
Copy link
Contributor Author

arcanis commented Sep 21, 2022

I've investigated, but the issue is a little complicated for me, as I don't know very well the v8 C++ API. It turns out the real cause for the problem this PR tries to fix is that NODE_OPTIONS is only processed when env is explicitly forwarded:

https://github.com/nodejs/node/blob/main/src/node_worker.cc#L488-L527

However, doing this also creates a new per_isolate_opts object (required for options_parser::Parse to work, otherwise it will segfault) - when per_isolate_opts is set, it will eventually make its way into isolate_data_->options(), which I think causes env->options()->cpu_prof to be reset here.

What I'm not sure, and where I could use some help:

  • Would it be acceptable to hardcode a list of settings that should be persisted across workers (starting with --cpu-prof)? In that case, how can I get the parent option values? I'm not sure what code specifies that, if per_isolate_opts is NULL, then the options are inherited from the parent isolate.

  • That being said, is forwarding the main isolate options (only in some cases!) even intended? Shouldn't users pass the relevant flags to NODE_OPTIONS instead, if they wish to debug nested workers/threads? (Probably a major change, so I don't plan to change this behaviour here; it's just for my curiosity)

@robertsLando
Copy link

updates on this?

@arcanis
Copy link
Contributor Author

arcanis commented Oct 11, 2023

As far as I can tell my questions are still relevant, and I'd need some guidance on what's the appropriate strategy here.

@robertsLando
Copy link

@aduh95 Dunno if the @nodejs/workers worked, maybe you could tag someone that can help moving this forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants