[bug#61803] [PATCH 0/3] [shepherd] improve race-free spawn+wait

[Ludovic Courtès]

Hi Ulf,

Ulf Herrman <striness@tilde.club> skribis:

> These patches fill out shepherd's procedures for running processes to
> completion.  They add a replacement for 'system' to complement the
> existing replacement for 'system*', and add a 'fork+exec+wait-process'
> procedure so that the flexibility of that family of procedures is
> available for this use case as well.  It also improves error handling in
> the event that an exception occurs while spawning a process in the
> process monitor, which would normally kill that essential fiber.

Nice!

> Note: I previously tried to send this to guix-devel, but it didn't seem
> to make it (I didn't see it in the archives after half a day), and after
> some consideration I recalled that guix-patches exists.  Is this the
> right place for shepherd patches?

Yes, as Tobias confirmed already.  :-)

> From 64370a98dfc17f0531de7397a38362c03a1d89bc Mon Sep 17 00:00:00 2001
> From: ulfvonbelow <striness@tilde.club>
> Date: Sat, 25 Feb 2023 00:42:41 -0600
> Subject: [PATCH 1/3] service: Propagate exceptions while spawning in process
>  monitor.
>
> * modules/shepherd/service.scm (unboxed-errors): new procedure.
>   (boxed-errors): new syntax.
>   (process-monitor): use it to propagate exceptions from fork+exec-command via
>   reply channel.
>   (spawn-via-monitor): new procedure.
>   (spawn-command): use it.

Good catch!  I added a test and a copyright line for you (let me know if
I got it wrong) and pushed as 18989f2fffa6ecdbd0f9b77834e1a54c9c45ee73.

> From 51ee63ace6f3f52eb196c990664cc6b9af3d3683 Mon Sep 17 00:00:00 2001
> From: ulfvonbelow <striness@tilde.club>
> Date: Sat, 25 Feb 2023 00:46:27 -0600
> Subject: [PATCH 2/3] service: accept fork+exec-command argument list in
>  monitor.
>
> Sometimes it's necessary to run startup / shutdown programs as a certain user,
> in a certain directory, with certain environment variables, etc.  Shepherd
> currently provides a replacement for system* that won't race against the
> child process auto-reaper, but this lacks the flexibility Shepherd users are
> used to.
>
> * modules/shepherd/service.scm (process-monitor): treat command instead as
>   argument list to fork+exec-command.
>   (spawn-via-monitor): update to new convention.
>   (fork+exec+wait-command): new procedure.

I’ll take a closer look to this one and report back soon.

> From 177592ee9d4b7fc6dcc80e545e8ad615a1d6786c Mon Sep 17 00:00:00 2001
> From: ulfvonbelow <striness@tilde.club>
> Date: Sat, 25 Feb 2023 00:56:57 -0600
> Subject: [PATCH 3/3] service: add spawn-shell-command replacement for
>  `system'.
>
> We already have a replacement for `system*' that avoids racing, but not for
> `system'.
>
> * configure.ac (SHELL): new substitution variable.
> * modules/shepherd/system.scm.in (%shell-filename): new variable.
> * modules/shepherd/service.scm
>   (spawn-shell-command, real-system): new procedures.
> * modules/shepherd.scm (main): replace `system' with `spawn-shell-command'.

Out of curiosity, do you have a need for ‘system’?  I’m inclined to
recommend against its use, in which case this patch is unnecessary.

> +(define %shell-filename "@SHELL@")

This is the configure-time shell so it will be wrong when
cross-compiling.

I’d just do:

  (define %shell (or (getenv "SHELL") "/bin/sh"))

Thanks!

Ludo’.