(experimental) worker isolation #24947

FranckFreiburger · 2018-12-10T22:07:38Z

Version: 11.3.0
Platform:win 7 (64)
Subsystem:

I just wondering if this lack of isolation is expected:

new Worker(`process.env.FOO = 123`, { eval: true });
new Worker(`console.log(process.env.FOO)`, { eval: true }); // prints 123

The text was updated successfully, but these errors were encountered:

addaleax · 2018-12-10T22:19:07Z

I think at the very least the documentation does not match the actual behaviour here… I think in an earlier version of the workers implementation, we did what the doc states (not allow modifications from worker threads), but it seems that right now we also allow write access?

/cc @nodejs/workers

devsnek · 2018-12-10T22:21:41Z

i prefer the documented behaviour

FranckFreiburger · 2018-12-10T22:27:48Z

@addaleax, is this behavior thread-safe ?

addaleax · 2018-12-10T22:28:39Z

@FranckFreiburger We access the environment variables with a mutex, both for reading and for writing, so this should be okay (at least as far as Node is concerned). But like @devsnek, I think I’d prefer the documented behaviour.

joyeecheung · 2018-12-10T22:42:30Z

So this is a regression? (trying to apply labels)

addaleax · 2018-12-10T23:39:49Z

@joyeecheung This behaviour (and the wrong docs) have been present since workers have first been released; I assume these checks went lost during a rebase or similar… so I don’t think it’s a regression, it’s just a bug?

sagitsofan · 2018-12-22T19:57:18Z

@addaleax I will pick this one.
Should the appropriate solution needs to block the write access to process.env and emit a warning / exception ?

addaleax · 2018-12-22T20:01:33Z

@sagitsofan I don’t think we want a warning or exception; rather, not installing the setters for process.env should be the most JavaScript-y solution here, I think?

sagitsofan · 2018-12-22T20:06:46Z

@addaleax But if we silently will not install the setters of process.env the user will think this operation have done successfully, isn't it? shouldn't a warning is the appropriate way?

devsnek · 2018-12-22T20:49:40Z

@sagitsofan you should use strict mode, so it will throw an error.

Added strict mode inside the worker script. Fixes: nodejs#24947

sagitsofan · 2018-12-24T06:57:34Z

Opening a new PR

sagitsofan · 2018-12-24T09:31:12Z

PR #25202 is ready

Added strict mode inside the worker script. Fixes: nodejs#24947

isaacs · 2019-01-21T07:58:50Z

I am currently exploring using worker_threads to simulate processes in programs that would otherwise spawn a lot of child procs.

I'm very much in favor of making the env isolated, but can it be done without making process.env completely locked down? For example, if I can swap out the whole process.env object, or make the values set in the worker but not in the parent, that would be ideal.

(I realize that of course this is not going to work with native addons or other parts of the program that access the env, since there is actually only one environment per process.)

addaleax · 2019-01-21T21:54:43Z

I do like @isaacs’ suggestion.

@nodejs/workers Thoughts?

benjamingr · 2019-01-26T12:09:32Z

(I realize that of course this is not going to work with native addons or other parts of the program that access the env, since there is actually only one environment per process.)

This makes it sound leaky.

I think there is value in providing this functionality to simulate processes communicating through the environment. I'm not sure process.env is where I'd put this sort of "configuration shared state".

addaleax · 2019-01-27T18:47:05Z

@benjamingr It is a bit leaky, but really, native addons are the only items I could think of in terms of what would really be caught off guard by this.

benjamingr · 2019-01-30T15:13:39Z

@addaleax what about exposing a worker.env or something similar?

addaleax · 2019-02-22T19:10:50Z

@bmeck The variables would come from the Worker’s (foo’s) process.env, in your example including the modifications to process.env.PATH. (Unless we explicitly choose to change this in some way. But I don’t think we’d want to?)

FranckFreiburger · 2019-02-22T19:20:20Z

🎉
I think that semantically process is the process, and process.env is the process's env.
This mean the the current behavior is consistent.
Then I think that any other behavior should be subject to a new API (maybe even remove process access from worker threads).

I agree that Isaac’s suggestion is the most convenient (and is my use case ie. run in a worker thread like if run in normal process) but is the less consistent.

addaleax · 2019-02-22T19:28:21Z

@FranckFreiburger One unfortunate circumstance is that process has historically more of a place to put random items, rather than something actually specifiying the current process (e.g.: process.nextTick(), process.version/process.versions, process.platform, process.domain, process.dlopen(), etc.).

In Workers, the closest thing we have is referring to the current thread. And we can’t really remove process access from worker threads without a ton of breakage, given how common usage of process.nextTick() or the stdio streams is; accessing a lot of per-process state, such as the current working directory, is already restricted.

bmeck · 2019-02-22T20:20:36Z

My concern here is that we get to a situation where emulating different process.env is possible with reasonable effort, but emulating having the same env is not. Technically, if it is normally shared you can always replace the .env descriptor via means like:

process.env = makeFakeENV();
// ... replace usages in child_process ...

However if we separate them by default, that makes it hard to share them, even though C++ would allow sharing them. I think choosing the one that matches the underlying structure and that still allows users to replace it with there is less likely to be something that causes inconsistency over time.

I think any solution (read only, or shared by default) would allow the separated form to be virtualized ontop of them, while the converse is not true.

addaleax · 2019-02-22T20:33:15Z

@bmeck I see … I think you could still reasonably implement sharing process.env through message passing (possibly making it synchronous with Atomics.wait), but I can see that there are downsides to that approach.

How would you feel about making sharing with the parent thread possible as an opt-in?

bmeck · 2019-02-22T20:35:40Z

@addaleax I think opt out makes more sense, you are opting out of the technical underpinnings threads would be expected to represent; this seems more like users making a choice about their intent than opting into the regular underpinnings that threads represent.

mcollina · 2019-02-23T00:11:41Z

I'm ok both ways.

Fishrock123 · 2019-02-23T00:15:55Z

I would certainly prefer that env be across the whole process.

My only worry is that... I dunno people might try use it to use it to share state lol

gireeshpunathil · 2019-02-23T05:18:55Z

I support the current behavior, and propose the doc to align with the code:

process.env reflects a single source of truth across the execution space.
provides simplicity and clarity for the users and the providers.
workers already enjoy the benefit of being in-process, in many cases. Also they share resources with main thread, and make side effects to the overall process. So why to isolate process.env alone?
Maintains consistency with other server side languages and platforms. In multi-threaded programs, threads are first class citizens and are privileged to run any code interchangeably with main thread (browsers may have specific reasons for isolation that is not relevant here IMO)

On the other hand, I think using process.env as a communication channel between running threads is a bad idea. We do have channel abstractions for the same (message ports) that provides fine grained, non-blocking communication facility.

However, ability for workers to use this entity to control child process environment improves the use case for workers by large. But isn't spawn(proc, args, env: Object.assign({}, process.env, { foo: 'bar' }) already possible?

joyeecheung · 2019-02-23T06:53:11Z

If we do not allow invocations of process.chdir and the mutable process.umask in workers, I think we should disable modifications to the environment variables in workers as well.

gireeshpunathil · 2019-02-23T07:30:14Z

I don't know the background of several process.x() being made unavailable to workers, I thought those are to do with either:

absence of a specification / requirement on the behavior of those, and / or
internal complexity of implementing those in the original version, but can incrementally add support for each of these APIs.

what drawback, in particular, do we see if we have a single read-write piece of memory for process.env that is protected by exclusive locks?

joyeecheung · 2019-02-23T07:36:37Z

I believe the process methods are made unavailable in workers because we generally only allow the main thread to own (and mutate) per-process states - however the workers are able to read these states just fine. It's always possible for the users to post messages to the main thread and handle them to perform these mutations while using Atomics.await to do whatever locking their use cases require, so the question is, what is the benefit of shifting this responsibility into core and make process.env mutable in workers? Should we allow multiple workers to mutate the environment variables at the same time and defer to them to perform proper locking? (the setter implementation is protected with a mutex, but the user code around the setter invocations still need to take care of any races on their own)

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

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

addaleax added question Issues that look for answers. worker Issues and PRs related to Worker support. doc Issues and PRs related to the documentations. and removed question Issues that look for answers. labels Dec 10, 2018

addaleax removed the doc Issues and PRs related to the documentations. label Dec 10, 2018

joyeecheung added the confirmed-bug Issues with confirmed bugs. label Dec 11, 2018

sagitsofan mentioned this issue Dec 23, 2018

worker: Fix workers isolation #25196

Closed

2 tasks

sagitsofan added a commit to sagitsofan/node that referenced this issue Dec 24, 2018

lib: worker fix workers isolation

f34f390

Added strict mode inside the worker script. Fixes: nodejs#24947

sagitsofan mentioned this issue Dec 24, 2018

lib: worker fix workers isolation #25202

Closed

2 tasks

Trott pushed a commit to sagitsofan/node that referenced this issue Dec 24, 2018

worker_threads: fix workers isolation

f4642f3

Added strict mode inside the worker script. Fixes: nodejs#24947

addaleax mentioned this issue Mar 9, 2019

worker: use copy of process.env #26544

Closed

4 tasks

addaleax closed this as completed in f5b5fe3 Mar 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(experimental) worker isolation #24947

(experimental) worker isolation #24947

FranckFreiburger commented Dec 10, 2018

addaleax commented Dec 10, 2018

devsnek commented Dec 10, 2018

FranckFreiburger commented Dec 10, 2018

addaleax commented Dec 10, 2018

joyeecheung commented Dec 10, 2018

addaleax commented Dec 10, 2018

sagitsofan commented Dec 22, 2018 •

edited

Loading

addaleax commented Dec 22, 2018

sagitsofan commented Dec 22, 2018 •

edited

Loading

devsnek commented Dec 22, 2018 •

edited

Loading

sagitsofan commented Dec 24, 2018

sagitsofan commented Dec 24, 2018

isaacs commented Jan 21, 2019

addaleax commented Jan 21, 2019

benjamingr commented Jan 26, 2019

addaleax commented Jan 27, 2019

benjamingr commented Jan 30, 2019

addaleax commented Feb 22, 2019

FranckFreiburger commented Feb 22, 2019 •

edited

Loading

addaleax commented Feb 22, 2019

bmeck commented Feb 22, 2019

addaleax commented Feb 22, 2019

bmeck commented Feb 22, 2019 •

edited

Loading

mcollina commented Feb 23, 2019

Fishrock123 commented Feb 23, 2019

gireeshpunathil commented Feb 23, 2019

joyeecheung commented Feb 23, 2019 •

edited

Loading

gireeshpunathil commented Feb 23, 2019

joyeecheung commented Feb 23, 2019 •

edited

Loading

(experimental) worker isolation #24947

(experimental) worker isolation #24947

Comments

FranckFreiburger commented Dec 10, 2018

addaleax commented Dec 10, 2018

devsnek commented Dec 10, 2018

FranckFreiburger commented Dec 10, 2018

addaleax commented Dec 10, 2018

joyeecheung commented Dec 10, 2018

addaleax commented Dec 10, 2018

sagitsofan commented Dec 22, 2018 • edited Loading

addaleax commented Dec 22, 2018

sagitsofan commented Dec 22, 2018 • edited Loading

devsnek commented Dec 22, 2018 • edited Loading

sagitsofan commented Dec 24, 2018

sagitsofan commented Dec 24, 2018

isaacs commented Jan 21, 2019

addaleax commented Jan 21, 2019

benjamingr commented Jan 26, 2019

addaleax commented Jan 27, 2019

benjamingr commented Jan 30, 2019

addaleax commented Feb 22, 2019

FranckFreiburger commented Feb 22, 2019 • edited Loading

addaleax commented Feb 22, 2019

bmeck commented Feb 22, 2019

addaleax commented Feb 22, 2019

bmeck commented Feb 22, 2019 • edited Loading

mcollina commented Feb 23, 2019

Fishrock123 commented Feb 23, 2019

gireeshpunathil commented Feb 23, 2019

joyeecheung commented Feb 23, 2019 • edited Loading

gireeshpunathil commented Feb 23, 2019

joyeecheung commented Feb 23, 2019 • edited Loading

sagitsofan commented Dec 22, 2018 •

edited

Loading

sagitsofan commented Dec 22, 2018 •

edited

Loading

devsnek commented Dec 22, 2018 •

edited

Loading

FranckFreiburger commented Feb 22, 2019 •

edited

Loading

bmeck commented Feb 22, 2019 •

edited

Loading

joyeecheung commented Feb 23, 2019 •

edited

Loading

joyeecheung commented Feb 23, 2019 •

edited

Loading