eio_posix: initial support for subprocesses #461

talex5 · 2023-03-15T11:37:32Z

To spawn a child executable on Unix, the parent forks a copy of itself, then has the child copy set up the environment and execve the new program.

However, we cannot run any OCaml code in the forked child process. This is because fork only duplicates its own domain. To the child, it appears that all other domains have stopped responding and if it tries to e.g. perform a GC then the child process will hang.

Therefore, the fork and all child actions need to be written in C. To keep this flexible, this PR adds a new fork_action type and lets the user provide a list of such actions to perform.

Some of this support is in Eio_unix as it should be possible to share it with Eio_linux.

At present, there are three actions (chdir, fchdir and execve). It should be fairly easy to plug in new ones (e.g. for renumbering file descriptors, dropping privileges, execveat, etc). Different backends may provide different functions.

In future, it might be good to support vfork too, as well as clone3 on Linux. The SIGCHLD stuff could also be massively simplified on systems with process descriptors, but I think that's only Linux and FreeBSD at the moment (and Eio_linux can do its own thing anyway).

To spawn a child executable on Unix, the parent `fork`s a copy of itself, then has the child copy set up the environment and `execve` the new program. However, we cannot run any OCaml code in the forked child process. This is because `fork` only duplicates its own domain. To the child, it appears that all other domains have stopped responding and if it tries to e.g. perform a GC then the child process will hang. Therefore, the `fork` and all child actions need to be written in C. To keep this flexible, this PR adds a new `fork_action` type and lets the user provide a list of such actions to perform.

haesbaert · 2023-03-15T14:47:54Z

lib_eio_posix/eio_posix_stubs.c

+    uerror("fork", Nothing);
+  }
+
+  CAMLreturn(Val_long(child_pid));


I'm surprised this holds, since a pid_t might not fit on an int. The standard library does the same though.

man system_data_types says:

According to POSIX, [pid_t] shall be a signed integer type, and the implementation shall support one or more programming environments where the width of pid_t is no greater than the width of the type long.

That could indeed be too big for an OCaml int. However, https://unix.stackexchange.com/questions/16883/what-is-the-maximum-value-of-the-process-id seems to indicate that the limit is usually quite small.

Interesting, I remember Stevens mentioning that the correct practice whas to for example cast a pid_t to (long long) in order to print it safely, since it was impossible to know its size.

haesbaert · 2023-03-15T14:49:43Z

lib_eio_posix/eio_posix_stubs.c

+  child_pid = fork();
+  if (child_pid == 0) {
+    eio_unix_run_fork_actions(Int_val(v_errors), v_actions);
+  } else if (child_pid < 0) {


Suggested change

} else if (child_pid < 0) {

} else if (child_pid == -1) {

Why? PIDs can't be negative, so if it returns e.g. -5 then it would be better to raise an exception. The kernel may do interesting things if passed a negative PID. e.g. kill says:

If pid is less than -1, then sig is sent to every process in the process group whose ID is -pid.

Exactly, PIDs can't be negative and fork cannot return anything below -1.

If it never returns anything below -1 then this code change makes no difference. And if it does, then the original version was safer.

it was just more "precise" 😄

lib_eio/unix/fork_action.c

patricoferris

Looks good! I did a little bit of stress testing on macOS and nothing seemed to break! Would standard output redirection be another action?

patricoferris · 2023-03-16T10:36:16Z

lib_eio/unix/eio_unix.mli

@@ -102,6 +102,8 @@ module Private : sig
        (<Eio.Flow.source; Eio.Flow.close; unix_fd> * <Eio.Flow.sink; Eio.Flow.close; unix_fd>) Effect.t (** See {!pipe} *)

  module Rcfd = Rcfd
+
+  module Fork_action = Fork_action


I think Fork_action is a sensible name, but I wonder if people will get confused with Fiber.fork?

It could be Child_action I suppose. But if you're using the low-level API to get precise control over forking, you probably know what Process.Fork_action means.

patricoferris · 2023-03-16T10:42:57Z

lib_eio_posix/eio_posix.mli

@@ -22,3 +22,5 @@ val run : (stdenv -> 'a) -> 'a

 module Low_level = Low_level
 (** Low-level API for making POSIX calls directly. *)
+
+module Process = Process


Should Process be exposed at the top-level of Eio_posix or inside Low_level?

Yes, this was a bit of a hack because Process depends on Low_level. I've moved it now, by making Children a separate file instead.

patricoferris · 2023-03-16T11:08:11Z

lib_eio_posix/process.mli

+
+val spawn : sw:Switch.t -> Fork_action.t list -> t
+(** [spawn ~sw actions] forks a child process, which executes [actions].
+    The last action should be {!Fork_action.execve}.


Do we need to document this more? You could in theory infer that the following is allowed:

let p = Eio_posix.Process.(spawn ~sw [ Fork_action.chdir "./bench"; Fork_action.execve "/bin/ls" ~argv:[| "ls" |] ~env:(Unix.environment ()); Fork_action.chdir ".."; Fork_action.execve "/bin/ls" ~argv:[| "ls" |] ~env:(Unix.environment ()) ]) in ignore (Eio.Promise.await p.exit_status)

I've added a comment to execve that it only makes sense to put it last.

patricoferris · 2023-03-16T11:12:35Z

lib_eio_posix/process.ml

+
+  let rec reap_nonblocking () =
+    if Mutex.try_lock lock then (
+      reap ();


Is there a chance the reap function above raises a different exception than the ones in the pattern-match and we end up not unlocking the lock?

I hope not. It's not clear whether it would be safe to release the mutex in that case, as we're probably in a bad state anyway.

patricoferris · 2023-03-16T11:32:17Z

lib_eio_posix/process.mli

+
+type t = {
+  pid : int;
+  exit_status : Unix.process_status Promise.t;  (** Resolves once the process has finished. *)


Should we expose the concrete type for this, or abstract it and add functions to access pid and exit_status ?

Good idea. I've made it abstract now.

Also, make `Process.t` abstract and document that `execve` can only go last. Suggested by Patrick Ferris.

avsm · 2023-03-17T14:56:30Z

lib_eio/unix/fork_action.c

+  while (len > 0) {
+    int wrote = write(fd, buf, len);
+
+    if (wrote <= 0)


it's worth checking for EINTR here isn't it? Reasonable chance that if there's an error from a forked child, that some signal is coming back to the parent process and interrupting things.

This code runs in the child. Possibly we should be masking signals there though.

talex5 · 2023-03-17T16:47:02Z

Would standard output redirection be another action?

I've made a branch with some experimental support for inheriting FDs here: https://github.com/talex5/eio/compare/spawn...talex5:eio:inherit-fds?expand=1

We can operate directly on the safe wrapper, which also simplifies the code a bit. Also, add a test for `fchdir`.

This simplifies the code a bit. Also, check the type of the function passed to `Val_fork_fn`.

RyanGibb · 2023-04-03T15:12:27Z

This allows fixing the issue described at #435 (comment) by writing custom fork actions wrapping dup2 and ioctl: RyanGibb/ocaml-exec-shell@eedf2b...main#diff-5f310ca7290aedf0d4ea03186711c91b09aa4f211b0c4f3956260ea10ab98c18

How would you feel about exposing this to and documenting for users? (I.e. not putting it behind Eio_unix.Private)

talex5 · 2023-04-05T12:40:51Z

@RyanGibb : we could just add setsid and setting the tty actions to Eio itself.

If we want to make this public, it will need a bit of work to put the public header files where other packages can find them.

RyanGibb · 2023-04-05T12:45:12Z

If we want to make this public, it will need a bit of work to put the public header files where other packages can find them.

That makes sense, I did have to duplicate some declarations and definitions from fork_action.h.

we could just add setsid and setting the tty actions to Eio itself.

I'll try and create a separate PR doing that.

@talex5

CHANGES: New features: - Add eio_posix backend (@talex5 @haesbaert ocaml-multicore/eio#448 ocaml-multicore/eio#477, reviewed by @avsm @patricoferris @polytypic). This replaces eio_luv on all platforms except Windows (which will later switch to its own backend). It is a lot faster, provides access to more modern features (such as `openat`), and can safely share OS resources between domains. - Add subprocess support (@patricoferris @talex5 ocaml-multicore/eio#461 ocaml-multicore/eio#464 ocaml-multicore/eio#472, reviewed by @haesbaert @avsm). This is the low-level API support for eio_linux and eio_posix. A high-level cross-platform API will be added in the next release. - Add `Fiber.fork_seq` (@talex5 ocaml-multicore/eio#460, reviewed by @avsm). This is a light-weight alternative to using a single-producer, single-consumer, 0-capacity stream, similar to a Python generator function. Bug fixes: - eio_linux: make it safe to share FDs across domains (@talex5 ocaml-multicore/eio#440, reviewed by @haesbaert). It was previously not safe to share file descriptors between domains because if one domain used an FD just as another was closing it, and the FD got reused, then the original operation could act on the wrong file. - eio_linux: release uring if Linux is too old (@talex5 ocaml-multicore/eio#476). Avoids a small resource leak. - eio_linux: improve error handling creating pipes and sockets (@talex5 ocaml-multicore/eio#474, spotted by @avsm). If we get an error (e.g. too many FDs) then report it to the calling fiber, instead of exiting the event loop. - eio_linux: wait for uring to finish before exiting (@talex5 ocaml-multicore/eio#470, reviewed by @avsm). If the main fiber raised an exception then it was possible to exit while a cancellation operation was still in progress. - eio_main: make `EIO_BACKEND` handling more uniform (@talex5 ocaml-multicore/eio#447). Previously this environment variable was only used on Linux. Now all platforms check it. - Tell dune about `EIO_BACKEND` (@talex5 ocaml-multicore/eio#442). If this changes, dune needs to re-run the tests. - eio_linux: add some missing close-on-execs (@talex5 ocaml-multicore/eio#441). - eio_linux: `read_exactly` fails to update file offset (@talex5 ocaml-multicore/eio#438). - Work around dune `enabled_if` bug on non-Linux systems (@polytypic ocaml-multicore/eio#475, reviewed by @talex5). - Use raw system call of `getrandom` for glibc versions before 2.25 (@zenfey ocaml-multicore/eio#482). Documentation: - Add `HACKING.md` with hints for working on Eio (@talex5 ocaml-multicore/eio#443, reviewed by @avsm @polytypic). - Improve worker pool example (@talex5 ocaml-multicore/eio#454). - Add more Conditions documentation (@talex5 ocaml-multicore/eio#436, reviewed by @haesbaert). This adds a discussion of conditions to the README and provides examples using them to handle signals. - Condition: fix the example in the docstring (@avsm ocaml-multicore/eio#468). Performance: - Add a network benchmark using an HTTP-like protocol (@talex5 ocaml-multicore/eio#478, reviewed by @avsm @patricoferris). - Add a benchmark for reading from `/dev/zero` (@talex5 ocaml-multicore/eio#439). Other changes: - Add CI for macOS (@talex5 ocaml-multicore/eio#452). - Add tests for `pread`, `pwrite` and `readdir` (@talex5 ocaml-multicore/eio#451). - eio_linux: split into multiple files (@talex5 ocaml-multicore/eio#465 ocaml-multicore/eio#466, reviewed by @avsm). - Update Dockerfile (@talex5 ocaml-multicore/eio#471). - Use dune.3.7.0 (@patricoferris ocaml-multicore/eio#457). - Mint exclusive IDs across domains (@TheLortex ocaml-multicore/eio#480, reported by @haesbaert, reviewed by @talex5). The tracing currently only works with a single domain anyway, but this will change when OCaml 5.1 is released.

@talex5

CHANGES: New features: - Add eio_posix backend (@talex5 @haesbaert ocaml-multicore/eio#448 ocaml-multicore/eio#477, reviewed by @avsm @patricoferris @polytypic). This replaces eio_luv on all platforms except Windows (which will later switch to its own backend). It is a lot faster, provides access to more modern features (such as `openat`), and can safely share OS resources between domains. - Add subprocess support (@patricoferris @talex5 ocaml-multicore/eio#461 ocaml-multicore/eio#464 ocaml-multicore/eio#472, reviewed by @haesbaert @avsm). This is the low-level API support for eio_linux and eio_posix. A high-level cross-platform API will be added in the next release. - Add `Fiber.fork_seq` (@talex5 ocaml-multicore/eio#460, reviewed by @avsm). This is a light-weight alternative to using a single-producer, single-consumer, 0-capacity stream, similar to a Python generator function. Bug fixes: - eio_linux: make it safe to share FDs across domains (@talex5 ocaml-multicore/eio#440, reviewed by @haesbaert). It was previously not safe to share file descriptors between domains because if one domain used an FD just as another was closing it, and the FD got reused, then the original operation could act on the wrong file. - eio_linux: release uring if Linux is too old (@talex5 ocaml-multicore/eio#476). Avoids a small resource leak. - eio_linux: improve error handling creating pipes and sockets (@talex5 ocaml-multicore/eio#474, spotted by @avsm). If we get an error (e.g. too many FDs) then report it to the calling fiber, instead of exiting the event loop. - eio_linux: wait for uring to finish before exiting (@talex5 ocaml-multicore/eio#470, reviewed by @avsm). If the main fiber raised an exception then it was possible to exit while a cancellation operation was still in progress. - eio_main: make `EIO_BACKEND` handling more uniform (@talex5 ocaml-multicore/eio#447). Previously this environment variable was only used on Linux. Now all platforms check it. - Tell dune about `EIO_BACKEND` (@talex5 ocaml-multicore/eio#442). If this changes, dune needs to re-run the tests. - eio_linux: add some missing close-on-execs (@talex5 ocaml-multicore/eio#441). - eio_linux: `read_exactly` fails to update file offset (@talex5 ocaml-multicore/eio#438). - Work around dune `enabled_if` bug on non-Linux systems (@polytypic ocaml-multicore/eio#475, reviewed by @talex5). - Use raw system call of `getrandom` for glibc versions before 2.25 (@zenfey ocaml-multicore/eio#482). Documentation: - Add `HACKING.md` with hints for working on Eio (@talex5 ocaml-multicore/eio#443, reviewed by @avsm @polytypic). - Improve worker pool example (@talex5 ocaml-multicore/eio#454). - Add more Conditions documentation (@talex5 ocaml-multicore/eio#436, reviewed by @haesbaert). This adds a discussion of conditions to the README and provides examples using them to handle signals. - Condition: fix the example in the docstring (@avsm ocaml-multicore/eio#468). Performance: - Add a network benchmark using an HTTP-like protocol (@talex5 ocaml-multicore/eio#478, reviewed by @avsm @patricoferris). - Add a benchmark for reading from `/dev/zero` (@talex5 ocaml-multicore/eio#439). Other changes: - Add CI for macOS (@talex5 ocaml-multicore/eio#452). - Add tests for `pread`, `pwrite` and `readdir` (@talex5 ocaml-multicore/eio#451). - eio_linux: split into multiple files (@talex5 ocaml-multicore/eio#465 ocaml-multicore/eio#466, reviewed by @avsm). - Update Dockerfile (@talex5 ocaml-multicore/eio#471). - Use dune.3.7.0 (@patricoferris ocaml-multicore/eio#457). - Mint exclusive IDs across domains (@TheLortex ocaml-multicore/eio#480, reported by @haesbaert, reviewed by @talex5). The tracing currently only works with a single domain anyway, but this will change when OCaml 5.1 is released.

@talex5

CHANGES: New features: - Add eio_posix backend (@talex5 @haesbaert ocaml-multicore/eio#448 ocaml-multicore/eio#477, reviewed by @avsm @patricoferris @polytypic). This replaces eio_luv on all platforms except Windows (which will later switch to its own backend). It is a lot faster, provides access to more modern features (such as `openat`), and can safely share OS resources between domains. - Add subprocess support (@patricoferris @talex5 ocaml-multicore/eio#461 ocaml-multicore/eio#464 ocaml-multicore/eio#472, reviewed by @haesbaert @avsm). This is the low-level API support for eio_linux and eio_posix. A high-level cross-platform API will be added in the next release. - Add `Fiber.fork_seq` (@talex5 ocaml-multicore/eio#460, reviewed by @avsm). This is a light-weight alternative to using a single-producer, single-consumer, 0-capacity stream, similar to a Python generator function. Bug fixes: - eio_linux: make it safe to share FDs across domains (@talex5 ocaml-multicore/eio#440, reviewed by @haesbaert). It was previously not safe to share file descriptors between domains because if one domain used an FD just as another was closing it, and the FD got reused, then the original operation could act on the wrong file. - eio_linux: release uring if Linux is too old (@talex5 ocaml-multicore/eio#476). Avoids a small resource leak. - eio_linux: improve error handling creating pipes and sockets (@talex5 ocaml-multicore/eio#474, spotted by @avsm). If we get an error (e.g. too many FDs) then report it to the calling fiber, instead of exiting the event loop. - eio_linux: wait for uring to finish before exiting (@talex5 ocaml-multicore/eio#470, reviewed by @avsm). If the main fiber raised an exception then it was possible to exit while a cancellation operation was still in progress. - eio_main: make `EIO_BACKEND` handling more uniform (@talex5 ocaml-multicore/eio#447). Previously this environment variable was only used on Linux. Now all platforms check it. - Tell dune about `EIO_BACKEND` (@talex5 ocaml-multicore/eio#442). If this changes, dune needs to re-run the tests. - eio_linux: add some missing close-on-execs (@talex5 ocaml-multicore/eio#441). - eio_linux: `read_exactly` fails to update file offset (@talex5 ocaml-multicore/eio#438). - Work around dune `enabled_if` bug on non-Linux systems (@polytypic ocaml-multicore/eio#475, reviewed by @talex5). - Use raw system call of `getrandom` for glibc versions before 2.25 (@zenfey ocaml-multicore/eio#482). Documentation: - Add `HACKING.md` with hints for working on Eio (@talex5 ocaml-multicore/eio#443, reviewed by @avsm @polytypic). - Improve worker pool example (@talex5 ocaml-multicore/eio#454). - Add more Conditions documentation (@talex5 ocaml-multicore/eio#436, reviewed by @haesbaert). This adds a discussion of conditions to the README and provides examples using them to handle signals. - Condition: fix the example in the docstring (@avsm ocaml-multicore/eio#468). Performance: - Add a network benchmark using an HTTP-like protocol (@talex5 ocaml-multicore/eio#478, reviewed by @avsm @patricoferris). - Add a benchmark for reading from `/dev/zero` (@talex5 ocaml-multicore/eio#439). Other changes: - Add CI for macOS (@talex5 ocaml-multicore/eio#452). - Add tests for `pread`, `pwrite` and `readdir` (@talex5 ocaml-multicore/eio#451). - eio_linux: split into multiple files (@talex5 ocaml-multicore/eio#465 ocaml-multicore/eio#466, reviewed by @avsm). - Update Dockerfile (@talex5 ocaml-multicore/eio#471). - Use dune.3.7.0 (@patricoferris ocaml-multicore/eio#457). - Mint exclusive IDs across domains (@TheLortex ocaml-multicore/eio#480, reported by @haesbaert, reviewed by @talex5). The tracing currently only works with a single domain anyway, but this will change when OCaml 5.1 is released.

patricoferris self-requested a review March 15, 2023 12:33

talex5 added the enhancement New feature or request label Mar 15, 2023

haesbaert reviewed Mar 15, 2023

View reviewed changes

lib_eio/unix/fork_action.c Outdated Show resolved Hide resolved

patricoferris reviewed Mar 16, 2023

View reviewed changes

haesbaert and others added 3 commits March 16, 2023 15:58

Use calloc for safety

fb97c16

Fix infinite loop where waitpid returns 0 instead of ECHILD

3e8f8d6

Move Eio_posix.Process inside Low_level

56da38f

Also, make `Process.t` abstract and document that `execve` can only go last. Suggested by Patrick Ferris.

talex5 force-pushed the spawn branch from 9a74b34 to 56da38f Compare March 16, 2023 16:35

avsm reviewed Mar 17, 2023

View reviewed changes

talex5 added 3 commits March 20, 2023 10:15

Pass Rcfd to Fork_action.fchdir

16ec359

We can operate directly on the safe wrapper, which also simplifies the code a bit. Also, add a test for `fchdir`.

Have fork actions call _exit directly on error

d10a720

This simplifies the code a bit. Also, check the type of the function passed to `Val_fork_fn`.

Update some comments

970833b

talex5 merged commit a48368c into ocaml-multicore:main Mar 20, 2023

talex5 deleted the spawn branch March 20, 2023 12:28

This was referenced Mar 20, 2023

API for spawning sub-processes #126

Closed

eio_linux: add subprocess support #472

Merged

talex5 mentioned this pull request Apr 11, 2023

[new release] eio_posix, eio_main, eio_luv, eio_linux and eio (0.9) ocaml/opam-repository#23656

Merged

talex5 mentioned this pull request Jun 2, 2023

[new release] eio_windows, eio_posix, eio_main, eio_linux and eio (0.10) ocaml/opam-repository#23864

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eio_posix: initial support for subprocesses #461

eio_posix: initial support for subprocesses #461

talex5 commented Mar 15, 2023

haesbaert Mar 15, 2023

talex5 Mar 15, 2023

haesbaert Mar 15, 2023

haesbaert Mar 15, 2023

talex5 Mar 15, 2023

haesbaert Mar 15, 2023

talex5 Mar 15, 2023

haesbaert Mar 15, 2023

patricoferris left a comment

patricoferris Mar 16, 2023

talex5 Mar 16, 2023

patricoferris Mar 16, 2023

talex5 Mar 16, 2023

patricoferris Mar 16, 2023

talex5 Mar 16, 2023

patricoferris Mar 16, 2023

talex5 Mar 16, 2023

patricoferris Mar 16, 2023

talex5 Mar 16, 2023

avsm Mar 17, 2023

talex5 Mar 17, 2023

talex5 commented Mar 17, 2023

RyanGibb commented Apr 3, 2023

talex5 commented Apr 5, 2023

RyanGibb commented Apr 5, 2023

eio_posix: initial support for subprocesses #461

eio_posix: initial support for subprocesses #461

Conversation

talex5 commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricoferris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talex5 commented Mar 17, 2023

RyanGibb commented Apr 3, 2023

talex5 commented Apr 5, 2023

RyanGibb commented Apr 5, 2023