worker: use copy of process.env by addaleax · Pull Request #26544 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation67 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Instead of sharing the OS-backed store for all process.env instances,
create a copy of process.env for every worker that is created.

The copies do not interact. Native-addons do not see modifications to
process.env from Worker threads, but child processes started from
Workers do default to the Worker’s copy of process.env.

This makes Workers behave like child processes as far as process.env
is concerned, and an option corresponding to the child_process
module’s env option is added to the constructor.

Fixes: #24947

/cc @nodejs/workers

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Choose a reason for hiding this comment

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

LG with the comments addressed.

Choose a reason for hiding this comment

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

Docs LGTM.

BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 10, 2019

Choose a reason for hiding this comment

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

LGTM

BTW, don't we need to fix the lint ?

Choose a reason for hiding this comment

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

Still LGTM

addaleax added wip

Issues and PRs that are still a work in progress.

and removed author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

labels

Mar 12, 2019

I’ve updated this to address some test failures, but it’s still not complete because we check environment variables before we actually receive the startup message, and the whole pre-execution setup has become kind of icky to deal with recently…

addaleax removed the wip

Issues and PRs that are still a work in progress.

label

Mar 12, 2019

@Trott I can’t make much of the test failures with --worker … they don’t happen locally for me, but it looks like they might be due to some kind of issue with common.PORT that’s specific to the CI setup?

@Trott I can’t make much of the test failures with --worker … they don’t happen locally for me, but it looks like they might be due to some kind of issue with common.PORT that’s specific to the CI setup?

Doesn't fail locally for me either. @nodejs/build Is there something unusual about the custom-suites-freestyle job that might provide a hint here?

@joyeecheung I think I’ll try the key-value-store based solution again. That should resolve your concerns, and if you want to review it, great ;)

addaleax added the wip

Issues and PRs that are still a work in progress.

label

Mar 13, 2019

addaleax added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 26, 2019

Abstract the process.env backing mechanism in C++ to allow different kinds of backing stores for process.env for different Environments.

Allow a generic string-based backing store, with no significance to the remainder of the process, as a store for process.env.

Instead of sharing the OS-backed store for all process.env instances, create a copy of process.env for every worker that is created.

The copies do not interact. Native-addons do not see modifications to process.env from Worker threads, but child processes started from Workers do default to the Worker’s copy of process.env.

This makes Workers behave like child processes as far as process.env is concerned, and an option corresponding to the child_process module’s env option is added to the constructor.

Fixes: nodejs#24947

addaleax added a commit that referenced this pull request

Mar 30, 2019

Abstract the process.env backing mechanism in C++ to allow different kinds of backing stores for process.env for different Environments.

PR-URL: #26544 Fixes: #24947 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Vse Mozhet Byt vsemozhetbyt@gmail.com Reviewed-By: Yongsheng Zhang zyszys98@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Joyee Cheung joyeec9h3@gmail.com

addaleax added a commit that referenced this pull request

Mar 30, 2019

Allow a generic string-based backing store, with no significance to the remainder of the process, as a store for process.env.

addaleax added a commit that referenced this pull request

Mar 30, 2019

Instead of sharing the OS-backed store for all process.env instances, create a copy of process.env for every worker that is created.

The copies do not interact. Native-addons do not see modifications to process.env from Worker threads, but child processes started from Workers do default to the Worker’s copy of process.env.

This makes Workers behave like child processes as far as process.env is concerned, and an option corresponding to the child_process module’s env option is added to the constructor.

Fixes: #24947

PR-URL: #26544 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: Vse Mozhet Byt vsemozhetbyt@gmail.com Reviewed-By: Yongsheng Zhang zyszys98@gmail.com Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Joyee Cheung joyeec9h3@gmail.com

chjj added a commit to chjj/bthreads that referenced this pull request

Apr 1, 2019