RFC: Immutable ENV

[ysbaddaden]

Preview:

https://github.com/crystal-lang/rfcs/blob/rfc-immutable-env/text/0019-immutable-env.md

Related to:

• https://forum.crystal-lang.org/t/eliminate-environment-modifications/8533
• Mutability of ENV crystal#16449
• PoC: immutable ENV + unsafe get/set + safe spec mock + optimizations crystal#16567
• Protect Crystal::System::Env with Sync::RWLock (UNIX) crystal#16591
• Add ENV.unsafe_set and deprecate mutable methods on ENV crystal#16601

[RX14]

No strong opinion, but I do like the Go approach.

[ysbaddaden]

I'm personally leaning on the startup snapshot (Java / Swift / Go) + explicitly unsafe method to set a variable (Rust).

We're safe by default. We encourage to never set a variable, but developers can choose to be unsafe at their own perils (you've been warned).

[RX14]

I think the calculus is a bit different when we currently allow ENV being mutable, it's probably worth the extra effort to make it work.

[straight-shoota]

I think it might be confusing if Crystal's ENV has a different opinion of the process' environment than any C library.
If you need to store this kind of "environment" values in the process that only need to be accessible from Crystal code, class vars or constants are simple alternatives.

So ultimately, I'd prefer not being able to mutate the snapshot independently of $environ. This could potentially serve as an intermediary step while mutating ENV is deprecated but still supported.
However, unsafe mutation that also affects the system environment should be acceptable for the deprecated behaviour. It stays true to current semantics. It's not as safe, but that's why it's deprecated.

[straight-shoota]

thought: We're missing the use case of mutating ENV["PATH"] for looking up executable paths.
A workaround is to explicitly resolve the path with Process.executable_path. But that's a bit bulky.
Maybe we could consider adding a path parameter to Process.run?

# old
ENV["PATH"] = "#{mypath};#{ENV["PATH"]}"
Process.run("foo")

# workaround
Process.run(Process.executable_path("foo", mypath))

# alternative?
Process.run("foo", path: mypath)

[ysbaddaden]

Process.run already has far too much params 🙈

[straight-shoota]

Let's make a process builder then... 🚀
(only half-joking here; it could be a useful pattern and many other languages have this)

[ysbaddaden]

Also Process.executable_path isn't exactly a replacement for how CreateProcessW searches executables on Windows because the former only searches in PATH, while the latter searches a bunch of places before PATH.

We might consider fixing Process.executable_path on Windows.

[straight-shoota]

suggestion: I would move up alternatives to ENV as primary recommendation. ENV is great for ingesting external configuration. But when you load configuration data directly in the process, there are much better alternatives.

For example, class variables are type safe and can be easily documented.
If you really need it, you can store free-form key-value mappings in a custom Hash(String, String), accessible as a constant.
More complex configuration libraries can help as well.

Prepping the environment before executing the application can work well, but it's a deployment problem, not an engineering solution.

[straight-shoota]

thought: I'm not sure about such a strong endorsement for a specific library.
I personally have no experience with totem. Maybe it is that good?
It doesn't seem to be used a lot, though.

[RX14]

I think it might be confusing if Crystal's ENV has a different opinion of the process' environment than any C library.

I agree, but my understanding was that the Go scheme would take a read snapshot at the start, and any mutations would apply to both the snapshot and the system env vars. However I did overlook that a C library writing to env would not be picked up by Go, only the other way around.

[ysbaddaden]

Johannes point was that mutating the environment in Crystal shall make it visible to external libraries because we mutated the environment. An external C library calling setenv is so unlikely that it's acceptable to not notice it.

I believe us three are aligned on the following: we want the initial snapshot of environ into ENV, all read operations to only access the snapshot (we never call getenv), and for write operations to mutate both the snapshot and environ.

The topics we're not necessarily aligned are:

1. deprecate ENV.[]=, ENV.delete and ENV.clear
2. introduce a new ENV.unsafe_set method

I believe deprecating will bring awareness to the problem (there's no @[Unsafe] annotation). We can delay a possible ENV.unsafe_set to popular demand, but shall rewrite the "Guide-level explanation" section (help wanted).

Probably expand on the Environment variables are meant to externally configure a program. We can explictly pass a modified copy of the environment when spawning a subprocess. sentences of the "Motivation" section, with how to read ENV variables and how to customize the environment for sub processes, segue to class variables, centralizing settings and how to leverage structs & type safety (e.g. YAML::Serialization) using overrides from ENV, and finally edge cases, such as modifying PATH, TMPDIR, etc.

[RX14]

deprecation + unsafe_set sounds good to me

[straight-shoota]

Before settling on anything final, we should do a bit more exploration to better understand the cases where there might be a need for environment modification.

Deprecating the existing methods would help to raise awareness and identify affected code. But then it's not great to issue deprecation warnings without clarity on how a replacement might look like.

We should just start with grepping through source code to identify ENV mutations in the ecosystems and help library authors to replace them. For engaging in discussions it would be nice to collect a document with a guide-level explanation and descriptions for common patterns. I'm not sure if an RFC is the best place for that, but it might do fine.
For this purpose we would need to merge the RFC in order to create a stable URL to link to. Even if it's not final. So maybe we should consider this a preliminary RFC which describes the problem and offers workarounds. As such it can grow over time as we discover more cases to document, for example. At some point we start a new RFC for deciding on whehter and how mutability stays in the stdlib.

I've already began some identifications, and I've deployed on a yak shaving tour with shards.

[ysbaddaden]

I checked my own projects: I already only read from ENV in actual application code.

For example instead of ENV["APP_ENV"] = "test" I use an explicit setup like App.setup(env: "test") (reminiscent of Log.setup_from_env), and the same project uses an external config files with overrides from ENV (read-only).

The few cases where I write to ENV are all in test suites.

For example tests for methods that use ENV (like the configurator above) that could just use mocking, which makes me think an official ENV.mock might be nice:

ENV.mock({ "KEY" => "value" }) do
  ENV["KEY"]?.should eq("value")
end

Other cases are to cleanup ENV for the duration of the test suite (a docker client) that could just use mocking too, or use globals like:

# foo.cr
module Foo
  class_properties(value) { ENV["DEFAULT"]? }
end

# spec/spec_helper.cr
require "foo"
Foo.value = "explicit"

# spec:
Foo.value.should eq("explicit")

Edit: and these examples should be in the guide level section.