(experimental) worker isolation

[FranckFreiburger]

• 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

[addaleax]

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]

i prefer the documented behaviour

[FranckFreiburger]

@addaleax, is this behavior thread-safe ?

[addaleax]

@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]

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

[addaleax]

@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]

@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]

@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]

@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]

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

[sagitsofan]

Opening a new PR

[sagitsofan]

PR #25202 is ready

[isaacs]

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]

I do like @isaacs’ suggestion.

@nodejs/workers Thoughts?

[benjamingr]

(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]

@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]

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

[addaleax]

@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]

🎉
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]

@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]

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]

@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]

@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]

I'm ok both ways.

[Fishrock123]

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]

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]

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]

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]

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)