Support `environment` parameter for `Isolate.spawnUri`

[bkonyi]

The environment parameter for Isolate.spawnUri currently throws an UnimplementedError, and the documentation claims that this functionality isn't yet implemented on all platforms. Given that the VM is the only runtime that supports dart:isolate, this parameter is currently useless.

When provided, the environment parameter should be used as the environment for the spawned isolate. Currently, isolates inherit the environment from the spawning isolate as a single hashmap in native code is used to store the environment for the entire process. Instead, each isolate should hold its own environment hashmap and only inherit the global environment when no custom environment is provided.

There appears to be some embedding APIs that are related to this functionality (e.g., Dart_SetEnvironmentCallback) that should be used when spawning new isolates.

[a-siva]

@lrhn we seem to have some ambiguity between the way String.fromEnvironment is documented and implemented. The documentation seems to indicate that the constructor is only guaranteed to work when invoked as const and may work on some platforms that have access to compiler options. However the non const invocations of it seems to work in an AOT environment too, The following program:

void main() {
  print('Foo: ${String.fromEnvironment('foo')}');
  print('Bar: ${const String.fromEnvironment('bar')}');
}
dart run -Dfoo='foo' -Dbar='bar' test.dart results in:
Foo: foo
Bar: bar

If we compile this to an AOT snapshot with no defines provided (dart compile aot-snapshot test.dart) and invoke dartaotruntime with the same defines as the JIT run (dartaotruntime -Dfoo='foo' -Dbar='bar' test.aot), we see the following:

Foo: foo
Bar:

Is the documentation correct or the implementation in the VM.

The reason this comes up here is because this issue has been filed to implement support for environment variables in spawnUri and if String.fromEnviroment is supposed to work only for the const constructor than this support becomes meaningless for the AOT case and it should probably also not do it in the JIT case in order to keep the behavior consistent between AOT and JIT.

[lrhn]

Seems like the VM implementation used whatever is available at the time of execution of the constructor.

The const invocation uses what was provided when the AoT program was compiled, the runtime uses what was provided to it for the non-const invocation.

If we consider non-const invocation as unspecified behavior, then it cannot be wrong. I'd still suggest to make the dartaotruntime ignore any -D flags, and treat the compilation environment as empty at runtime. That way nobody can start depending on it.

(I'd even make JIT code treat non-const invocations as running on an empty environment for consistency, but it's less important since it'll show as a single consistent environment. The example here could probably show different values for the same key, depending on whether it's const or not.)

[lrhn]

About spawnUri, I don't really understand how it works if not JIT-compiled. If it can target an AoT compiled program too, then I also wouldn't let that program see any new compilation environment values, other than the ones it was compiled with.

For JIT spawnUri, compiler options could make sense, but I'd also be perfectly happy just using the same options as the running isolate. The use-case for changing compiler flags for an isolate seems likely to be testing, and JIT-only. Maybe it could be an option in dart:developer, so it's not used in production.

[bkonyi]

Is there any reason we wouldn't want to support runtime environment variables and just document the behavior? If this only worked for one of the runtimes, I'd agree it's worth making the behavior consistent. However, both native runtime modes support this behavior in what I'd consider an unsurprising way (the const constructor results in a compile time constant and the non-const constructor performs a lookup at runtime). Given the implementation can be done completely within the VM without explicit configuration by the embedder, I think it's safe to document this behavior as a feature.

If we're dead set against doing this, there really should be a way to prevent the non-const constructors from being invoked, either through a compile time error or throwing an exception.

[bkonyi]

Another issue came in today where this functionality has caused confusion: https://github.com/dart-lang/sdk/issues/56146. We should come to a conclusion on what we want to do to resolve this ambiguity and figure out how we want to move forward (e.g., support runtime vs compile time defines explicitly and fully, add a lint to warn against using the non-const constructors, or file a breaking change request and make the non-const constructors throw).

[lrhn]

Is there any reason we wouldn't want to support runtime environment variables and just document the behavior?

It's not available on all platforms. It only works on the VM, possibly only works consistently on the JIT VM. It's a bad feature from a cross-platform design perspective.

I'd make non-const invocations throw, or be compile-time errors, on all platforms. If the VM wants something different/more, it can introduce that in a VM-only library. Maybe dart:isolate, then it also makes sense for Isolate.spawnUri to be able to set runtime values. But it can also just be a different (third) environment then, not the compilation environment.

[bkonyi]

It's not available on all platforms. It only works on the VM, possibly only works consistently on the JIT VM.

This does work consistently on both the JIT and AOT runtimes and technically works on web (there's no such concept as a runtime define since web apps are precompiled anyway).

It's a bad feature from a cross-platform design perspective.

In its current state, I'd agree :-)

I'd make non-const invocations throw, or be compile-time errors, on all platforms. If the VM wants something different/more, it can introduce that in a VM-only library. Maybe dart:isolate, then it also makes sense for Isolate.spawnUri to be able to set runtime values. But it can also just be a different (third) environment then, not the compilation environment.

If we don't choose to support this, I don't see any particular need to introduce this in a VM specific library or through a third environment. Filing a breaking change for removing the ability to use the non-const constructors and the environment parameter in Isolate.spawnUri would be the best way forward in that scenario.

[a-siva]

I'd make non-const invocations throw, or be compile-time errors, on all platforms.

@lrhn should we go this path and file a breaking change request for this as it would potentially be considered as breaking by some folks.

[lrhn]

The current state is stable. It's unspecified behavior to use new X.fromEnvironment, so any behavior is valid. The VM it's not wrong. It might be surprising in some cases (const and new invocations with the same key not giving the same result), but if it's internally consistent, people can work with that.

The one requirement we do have is that for any Dart program (isolate) the value of const X.fromEnvironment for any key must be consistent across all libraries of the program.

I think it should be a VM decision whether to keep supporting using runtime access to the compilation environment. It's the VM's unspecified behavior. I'd do the breaking change, for great consistency, but I'm not the one who has to support the current behavior.

[a-siva]

I think we will leave the current state as is, closing issue.