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: use copy of process.env #26544

Closed
wants to merge 3 commits into from
Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 9, 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

/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 addaleax added the worker Issues and PRs related to Worker support. label Mar 9, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

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.

lib/internal/main/worker_thread.js Outdated Show resolved Hide resolved
@addaleax
Copy link
Member Author

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

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Docs LGTM.

@BridgeAR
Copy link
Member

@BridgeAR 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
Copy link
Member

@ZYSzys ZYSzys left a comment

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 ?

@addaleax
Copy link
Member Author

@ZYSzys Thanks, fixed the linter :)

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

Copy link
Member

@ZYSzys ZYSzys left a comment

Choose a reason for hiding this comment

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

Still LGTM

@addaleax 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
@addaleax
Copy link
Member Author

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 addaleax removed the wip Issues and PRs that are still a work in progress. label Mar 12, 2019
@addaleax
Copy link
Member Author

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?

@addaleax
Copy link
Member Author

@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
Copy link
Member

Trott commented Mar 13, 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?

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?

lib/internal/worker.js Show resolved Hide resolved
src/node_credentials.cc Outdated Show resolved Hide resolved
@addaleax addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 13, 2019
@addaleax
Copy link
Member Author

@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 addaleax added the wip Issues and PRs that are still a work in progress. label Mar 13, 2019
@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

Landed in e4e2b0c...9fbf0c6

@addaleax addaleax closed this Mar 30, 2019
@addaleax addaleax deleted the worker-env branch March 30, 2019 21:33
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`.

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
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
BethGriggs pushed a commit that referenced this pull request Apr 5, 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>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
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>
BethGriggs pushed a commit that referenced this pull request Apr 5, 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>
BethGriggs pushed a commit that referenced this pull request Apr 9, 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>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
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>
BethGriggs pushed a commit that referenced this pull request Apr 9, 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>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 9, 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>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
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>
BethGriggs pushed a commit that referenced this pull request Apr 9, 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>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
BethGriggs added a commit that referenced this pull request Apr 9, 2019
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)
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
BethGriggs added a commit that referenced this pull request Apr 10, 2019
Notable changes:

- worker: use copy of process.env (Anna Henningsen)
  [#26544](#26544)

PR-URL: #27163
BethGriggs added a commit that referenced this pull request Apr 10, 2019
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
BethGriggs added a commit that referenced this pull request Apr 11, 2019
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
BethGriggs added a commit that referenced this pull request Apr 11, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(experimental) worker isolation
9 participants