Add Eio.Vars.t to Stdenv by patricoferris · Pull Request #923 · ocaml-multicore/eio

patricoferris · 2026-08-19T11:42:19Z

Access to environment variables after some discussion in #922

avsm

Some high level thoughts:

getenv/putenv aren't multithreaded safe, so is this a good opportunity to provide domain safety by introducing a mutex, so that if accesses go via Eio.Vars they are synchronised? We cant do anything about direct uses of Unix but this is better than nothing
bear in mind that secure_getenv will result in a null if setuid binary, but I think this is handled at the Unix library level.
The Eio.Vars layer is where we should be dealing with portability, so a really useful future addition would be to add a PATH parser using our shiny new Nt_path and Posix_path modules. On Windows it's ; separated and on POSIX : separated.

patricoferris · 2026-08-19T18:24:38Z

Thanks for the review Anil!! All very reasonable to me and I'll incorporate this soon!

patricoferris · 2026-08-20T10:33:14Z

Re:threadsafety-- do you think, in practice, this is a big issue? It seems that the functions are mostly not thread safe when used together across domains. But are a lot of programs doing a lot of "putting"?

avsm · 2026-08-20T10:56:36Z

I think the putting mainly happens at start-of-day. I do this, for example, when linking to third party libraries who need environmental configuration. It also happens most obviously at exec time but that's covered by other code.

patricoferris · 2026-08-21T11:15:32Z

I think the putting mainly happens at start-of-day. I do this, for example, when linking to third party libraries who need environmental configuration. It also happens most obviously at exec time but that's covered by other code.

If it happens at start of day, do you think there's much chance of hitting race conditions with other domains?

6b4f6d7 adds get_path and put_path functions.

avsm · 2026-08-21T18:10:16Z

If it happens at start of day, do you think there's much chance of hitting race conditions with other domains?

I think if something's exposed in the API, it should be as safe as possible. If we're exposing putenv, then why not make it safe with a mutex? If we think it's not used, then it can also just not be included, which I'd prefer to a racy one. Unix.putenv still exists in practise for that.

patricoferris · 2026-09-01T12:46:15Z

I think if something's exposed in the API, it should be as safe as possible. If we're exposing putenv, then why not make it safe with a mutex?

That's fair enough. I have added a mutex in the eio_unix implementation which the three backends use: 8872eea

Access to environment variables.

Fix off-by-one error in splitting environment variables.

…v access.

avsm

This looks good to me! I took the liberty of pushing two changes:

c20fae7 allocates the mutex once per stdenv, as otherwise each invocation of vars won't synchronise
15018f8 makes ~sep a char for symmetry

talex5 · 2026-09-02T09:38:10Z

+val get_all : _ t -> (string * string) list
+(** [get_all vars] gets the full list of environment variables. *)
+
+val get : _ t -> string -> string


I suggest either making this get_opt, or raising a more useful Eio.Io exception (giving the name of the variable that was missing).

talex5 · 2026-09-02T09:49:27Z

+val get_path : _ t -> string list
+(** [get_path var] gets the ["PATH"] variable and returns the paths as a list.
+
+    @raise Not_found if ["PATH"] does not exist.*)


I'm not very keen on this. There are lots of PATH-like variables (e.g. MANPATH). Probably this should take the variable name too.

Ok, maybe something like?

val get_paths : _ t -> string -> string list (** [get_paths var name] gets the value associated with [name] and splits it using the backend-specific path separator. *)

That looks good to me. Probably also worth specifying what happens with repeated separators -- are empty segments preserved? (I'm not even sure if that's valid in PATHs)

talex5 · 2026-09-02T09:55:44Z

+
+let try_get t name =
+  try
+    Eio.traceln "%s is %a" name Fmt.(quote string) (Eio.Vars.get t name)


Suggested change

Eio.traceln "%s is %a" name Fmt.(quote string) (Eio.Vars.get t name)

Eio.traceln "%s is %S" name (Eio.Vars.get t name)

shorter, and works correctly with strings containing quotes.

talex5 · 2026-09-02T09:57:49Z

+module Vars = Vars
+(** An {! Eio.System} interface using standard {! Unix} functions. *)


Should be in Private (and remove the System bit, or the whole comment).

I'm not even sure it needs to be a module; we could just have Private.vars be the actual resource. It can work out the path separator for itself easily enough from Sys.os_type. Then each backend can just do method vars = Eio_unix.Private.vars.

talex5 · 2026-09-02T09:59:49Z

 (** Managing child processes. *)
 module Process = Process

+(** {2 Environment Variables} *)


Could leave out the heading. Putting environment variables under Processes seems fine.

talex5 · 2026-09-02T10:07:37Z

+    val get : t -> string -> string
+    val get_path : t -> string list
+    val put : t -> name:string -> value:string -> unit
+    val put_path : t -> string list -> unit


setenv probably makes more sense for the name than putenv here (in C, setenv is the newer one).

Would be good to have unsetenv too.

talex5 · 2026-09-02T10:12:23Z

+
+(** {1 Accessing environment variables} *)
+
+val get_all : _ t -> (string * string) list


It's not very helpful that get_all returns a list of pairs, but Process.spawn ~env expects an array of strings.

The reasoning here was that I saw a few distinct use cases with Process.spawn ~env:

The user wants to inherit the environment from the parent in which case they provide no value to ~env, so there is no need for get_all to agree on the type.

The user wants to provide a filtered set of the parent process's environment variables in which case they are likely going to want to have access to (name, value) pairs in order to do the filtering before having to rejoin them to be a string array. Of course, some filtering could be done on a version of get_all that returns a string array, for example, String.starts_with ~prefix:"OCAML" v.

The user wants to extend the variables to include everything from the parent in addition to some executable-specific environment variables: this is the most compelling case for get_all to return a string array.

This does not take into account other use cases for get_all like finding some environment variables and their values for in-process OCaml logic where, I think, (name, value) pairs make the most sense.

I'm happy to make this return a string array (and we could provide a split function) but I thought I would lay out some of the reasoning for the current API.

Lemme know what you think :-)

If anything, this is case to change the Process.spawn API to take a list instead. It's a bit of a throwback to the C API to access an OCaml array here

I've opened #930, which changes the type spawn uses to Process.Env.t. Currently it's an alias of string array, but we can change it later once people have a chance to start using the new functions in there.

get_all can then use the same type. Though possibly it should just be Process_mgr.current_environment or something. The process manager already implicitly has access to the environment, since you can use ?env:None so it probably makes sense to put it there.

talex5 · 2026-09-02T11:53:07Z

Now I'm wondering how useful it is to be able to mutable the environment globally like this. The uses I can think of are:

Changing the environment to be inherited by subprocesses. But then it would be safer to make a copy instead and pass it explicitly using Process.spawn ~env.
Passing values to other Eio code in the same process. But since you have to pass env around anyway, there's no advantage over passing the values directly. Or you could use fiber-local variables, which are typed and scoped sensibly.
Passing values to legacy/non-Eio code. But then the receiving code isn't using this system, so the mutex doesn't help, and you might as well use the Unix API directly.

It might be simpler if Stdenv.vars just returned the raw string array, and Eio_unix perhaps had some helpers to work with environments (e.g. convert to and from string maps, and convert values between strings and path lists).

patricoferris · 2026-09-02T12:29:05Z

It might be simpler if Stdenv.vars just returned the raw string array, and Eio_unix perhaps had some helpers to work with environments (e.g. convert to and from string maps, and convert values between strings and path lists).

I think this could be a fine approach, essentially making the environment as handled by Eio readonly (probably 90% of uses cases). It does not allow for @avsm's needs stated here #923 (comment) (though as you say, you can always default to the racier Unix.putenv should you need).

However, I think it would be nice to be able to write portable Eio applications that can Eio.Var.get_opt at the very least (without linking unix). For example, in a jsoo application where we can provide the environment from the jsooEnvvariable (see the documentation). Though, they have made that work with unix so maybe the point is a bit moot. I think this happens by default if Stdenv.vars returns a (string * string) list and users will naturally use List.assoc_opt or (for heavier use cases) transform it into a map (StringMap.of_list).

avsm · 2026-09-02T13:20:03Z

I agree it's useful to not have a dependency on Eio_unix for 'pure' Eio applications. However, I'm also happy with a read-only environment for now, given how horrendously messy mutating it is...

patricoferris · 2026-09-05T12:03:26Z

Thanks for adding those functions in #930 @talex5 !

Should the vars provided by the environment be a readonly copy of the vars when the program starts or should it be a function somewhat equivalent to:

let vars () = Unix.environment () |> Eio.Process.Env.of_array

so that it reflects Unix.putenv?

EDIT: I suspect it should be somewhat lazy. I'm running nix and my environment is pretty big (env | wc -c gives 14765). We probably shouldn't inflict that upon every Eio program?

talex5 · 2026-09-05T19:06:09Z

I think it should read the current environment each time you call it. Otherwise, there's no way to get that.

avsm reviewed Aug 19, 2026

View reviewed changes

Comment thread lib_eio/unix/vars.ml Outdated

Comment thread lib_eio/vars.ml Outdated

Comment thread tests/vars.md

avsm reviewed Aug 20, 2026

View reviewed changes

Comment thread lib_eio/eio.mli Outdated

avsm reviewed Aug 20, 2026

View reviewed changes

Comment thread lib_eio/vars.mli Outdated

avsm reviewed Aug 20, 2026

View reviewed changes

Comment thread lib_eio/vars.mli Outdated

patricoferris force-pushed the eio-system branch 2 times, most recently from 2887bce to 6b4f6d7 Compare August 21, 2026 11:12

patricoferris and others added 8 commits September 1, 2026 19:41

Add Eio.Vars.t to Stdenv

a632797

Access to environment variables.

Test get_all

7cc9ccf

Fix off-by-one error in splitting environment variables.

Remove unnecessary reraise_with_context

635db92

Provide Eio.Vars.{get/put}_path

c7dcbcf

Fix occurrence spelling

6c78817

Add mutex to Eio.Vars interface

d65059d

Allocate the Vars resource once per event loop instead of on every en…

c20fae7

…v access.

Use a char separator for put_path to match get_path.

15018f8

avsm force-pushed the eio-system branch from 8872eea to 15018f8 Compare September 1, 2026 18:46

avsm approved these changes Sep 1, 2026

View reviewed changes

talex5 reviewed Sep 2, 2026

View reviewed changes

talex5 mentioned this pull request Sep 3, 2026

Add Process.Env #930

Merged

	Eio.traceln "%s is %a" name Fmt.(quote string) (Eio.Vars.get t name)
	Eio.traceln "%s is %S" name (Eio.Vars.get t name)

		module Vars = Vars
		(** An {! Eio.System} interface using standard {! Unix} functions. *)


		(** {1 Accessing environment variables} *)

		val get_all : _ t -> (string * string) list

Uh oh!

Conversation

patricoferris commented Aug 19, 2026

Uh oh!

avsm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patricoferris commented Aug 19, 2026

Uh oh!

patricoferris commented Aug 20, 2026

Uh oh!

avsm commented Aug 20, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patricoferris commented Aug 21, 2026

Uh oh!

avsm commented Aug 21, 2026

Uh oh!

patricoferris commented Sep 1, 2026

Uh oh!

avsm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talex5 commented Sep 2, 2026

Uh oh!

patricoferris commented Sep 2, 2026

Uh oh!

avsm commented Sep 2, 2026

Uh oh!

patricoferris commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talex5 commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

patricoferris commented Sep 5, 2026 •

edited

Loading