worker: use copy of process.env by addaleax · Pull Request #26544 · nodejs/node · GitHub

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

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2836/pipeline

LG with the comments addressed.

@ZYSzys @vsemozhetbyt @BridgeAR Your comments should be addressed, PTAL

Docs LGTM.

CI https://ci.nodejs.org/job/node-test-pull-request/21390/

LGTM

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

@ZYSzys Thanks, fixed the linter :)

CI: https://ci.nodejs.org/job/node-test-pull-request/21411/

Still LGTM

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…

Okay, I think I have everything fixed now (including having spent quite a bit of time on debugging until it turned out that object spread doesn’t work with Proxies before V8 7.4 🙁).

CI: https://ci.nodejs.org/job/node-test-pull-request/21485/

@benjamingr @jasnell @BridgeAR (and maybe also @joyeecheung, given the kind of code it touches) The new changes are in 493022133d217ca6df5662c0cc5b542bc8d03f9f...9a4e61f5d33142d83c7d8c176cb1978de0101ad0 can you take another look?

@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 ;)

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

CI: https://ci.nodejs.org/job/node-test-pull-request/22043/

Landed in e4e2b0c...9fbf0c6

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>

Allow a generic string-based backing store, with no significance to the remainder of the process, as a store for `process.env`. 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>

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>

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>

Allow a generic string-based backing store, with no significance to the remainder of the process, as a store for `process.env`. 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>

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>

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> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

Allow a generic string-based backing store, with no significance to the remainder of the process, as a store for `process.env`. 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> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

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> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

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> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

Allow a generic string-based backing store, with no significance to the remainder of the process, as a store for `process.env`. 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> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

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> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

Notable changes: - worker: use copy of process.env (Anna Henningsen) [#26544](#26544) - src: - implement generic backend for process.env (Anna Henningsen) [#26544](#26544) - allow per-Environment set of env vars (Anna Henningsen) [#26544](#26544)

Notable changes: - worker: use copy of process.env (Anna Henningsen) [#26544](#26544) PR-URL: #27163

Notable changes: - child_process: doc deprecate ChildProcess.\_channel (cjihrig) [#26982](#26982) - deps: update nghttp2 to 1.37.0 (gengjiawen) [#26990](#26990) - dns: - make dns.promises enumerable (cjihrig) [#26592](#26592) - remove dns.promises experimental warning (cjihrig) [#26592](#26592) - stream: make Symbol.asyncIterator support stable (Matteo Collina) [#26989](#26989) - worker: use copy of process.env (Anna Henningsen) [#26544](#26544) PR-URL: #27163

Notable changes: - child_process: doc deprecate ChildProcess.\_channel (cjihrig) [#26982](#26982) - deps: update nghttp2 to 1.37.0 (gengjiawen) [#26990](#26990) - dns: - make dns.promises enumerable (cjihrig) [#26592](#26592) - remove dns.promises experimental warning (cjihrig) [#26592](#26592) - fs: remove experimental warning for fs.promises (Anna Henningsen) [#26581] (#26581) - stream: make Symbol.asyncIterator support stable (Matteo Collina) [#26989](#26989) - worker: use copy of process.env (Anna Henningsen) [#26544](#26544) PR-URL: #27163

addaleax added the worker Issues and PRs related to Worker support. label Mar 9, 2019

ZYSzys reviewed Mar 9, 2019

View reviewed changes

Comment thread lib/internal/worker.js Outdated

vsemozhetbyt reviewed Mar 9, 2019

View reviewed changes

Comment thread doc/api/worker_threads.md Outdated

vsemozhetbyt reviewed Mar 9, 2019

View reviewed changes

Comment thread doc/api/worker_threads.md Outdated

BridgeAR approved these changes Mar 9, 2019

View reviewed changes

Comment thread lib/internal/main/worker_thread.js Outdated

vsemozhetbyt reviewed Mar 10, 2019

View reviewed changes

Comment thread doc/api/worker_threads.md Outdated

vsemozhetbyt approved these changes Mar 10, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 10, 2019

ZYSzys approved these changes Mar 10, 2019

View reviewed changes

jasnell approved these changes Mar 10, 2019

View reviewed changes

ZYSzys approved these changes Mar 11, 2019

View reviewed changes

benjamingr approved these changes Mar 11, 2019

View reviewed changes

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 outstanding review comments, and a CI started. labels Mar 12, 2019

addaleax force-pushed the worker-env branch from b9360a3 to 2176c6e Compare March 12, 2019 12:12

addaleax removed the wip Issues and PRs that are still a work in progress. label Mar 12, 2019

joyeecheung reviewed Mar 13, 2019

View reviewed changes

Comment thread lib/internal/worker.js Outdated

Comment thread src/node_credentials.cc Outdated

joyeecheung reviewed Mar 13, 2019

View reviewed changes

Comment thread lib/internal/main/worker_thread.js Outdated

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 13, 2019

addaleax added the wip Issues and PRs that are still a work in progress. label Mar 13, 2019

addaleax force-pushed the worker-env branch from 9a4e61f to 480e9bf Compare March 13, 2019 18:54

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 26, 2019

addaleax added 3 commits March 30, 2019 21:08

src: allow per-Environment set of env vars

127693b

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

src: implement generic backend for process.env

400aa06

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

addaleax force-pushed the worker-env branch from 721b0d8 to 838799b Compare March 30, 2019 20:22

addaleax closed this Mar 30, 2019

addaleax deleted the worker-env branch March 30, 2019 21:33

joyeecheung mentioned this pull request Apr 1, 2019

src: port coverage serialization to C++ #26874

Closed

4 tasks

chjj added a commit to chjj/bthreads that referenced this pull request Apr 1, 2019

worker: implement SHARE_ENV. see nodejs/node#26544 .

66f91af

BethGriggs mentioned this pull request Apr 9, 2019

v11.14.0 proposal #27163

Merged

BethGriggs added a commit that referenced this pull request Apr 10, 2019

2019-04-10, Version 11.14.0 (Current)

369e566

Notable changes: - worker: use copy of process.env (Anna Henningsen) [#26544](#26544) PR-URL: #27163

worker: use copy of process.env by addaleax · Pull Request #26544 · nodejs/node · GitHub

Uh oh!

Uh oh!

worker: use copy of process.env - #26544

worker: use copy of process.env#26544addaleax wants to merge 3 commits intonodejs:masternodejs/node:masterfrom addaleax:worker-envaddaleax/node:worker-envCopy head branch name to clipboard

Conversation

addaleax commented Mar 9, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax commented Mar 10, 2019

Uh oh!

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 10, 2019

Uh oh!

ZYSzys left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 10, 2019

Uh oh!

ZYSzys left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 12, 2019

Uh oh!

addaleax commented Mar 12, 2019

Uh oh!

addaleax commented Mar 13, 2019

Uh oh!

Trott commented Mar 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addaleax commented Mar 13, 2019

Uh oh!

nodejs-github-bot commented Mar 30, 2019

Uh oh!

addaleax commented Mar 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

worker: use copy of process.env#26544
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:worker-env