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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61803 in the body.
You can then email your comments to 61803 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ulf Herrman <striness <at> tilde.club>
To: guix-patches <at> gnu.org
Subject: [PATCH 0/3] [shepherd] improve race-free spawn+wait
Date: Sat, 25 Feb 2023 16:08:01 -0600

[Message part 1 (text/plain, inline)]

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.

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?

[0001-service-Propagate-exceptions-while-spawning-in-proce.patch (text/x-patch, attachment)]

[0002-service-accept-fork-exec-command-argument-list-in-mo.patch (text/x-patch, attachment)]

[0003-service-add-spawn-shell-command-replacement-for-syst.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 61803 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Ulf Herrman <striness <at> tilde.club>
Cc: 61803 <at> debbugs.gnu.org
Subject: Re: [bug#61803] [PATCH 0/3] [shepherd] improve race-free spawn+wait
Date: Sun, 26 Feb 2023 15:17:08 +0100

[Message part 1 (text/plain, inline)]

Ulf,

Ulf Herrman 写道：
> 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),

Note: I only check the spam/moderation queue once a day, most days 
;-)

>                                                                and 
>                                                                after
> some consideration I recalled that guix-patches exists.  Is this 
> the
> right place for shepherd patches?

It is!

If you like, you can configure ‘git send-email’ to default to 
guix-patches <at> gnu.org (sendemail.to).  It will send separate mails 
for each patch rather than attachments.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 61803 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ulf Herrman <striness <at> tilde.club>
Cc: 61803 <at> debbugs.gnu.org
Subject: Re: bug#61803: [PATCH 0/3] [shepherd] improve race-free spawn+wait
Date: Thu, 02 Mar 2023 23:16:33 +0100

Hi Ulf,

Ulf Herrman <striness <at> 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 <at> 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 <at> 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 <at> 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’.

Message #14 received at 61803 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ulf Herrman <striness <at> tilde.club>
Cc: 61803 <at> debbugs.gnu.org, 60106-done <at> debbugs.gnu.org
Subject: Re: bug#61803: [PATCH 0/3] [shepherd] improve race-free spawn+wait
Date: Sat, 04 Mar 2023 23:09:49 +0100

Hi Ulf,

Ulf Herrman <striness <at> tilde.club> skribis:

> From 51ee63ace6f3f52eb196c990664cc6b9af3d3683 Mon Sep 17 00:00:00 2001
> From: ulfvonbelow <striness <at> 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.

On this one I took a similar approach but chose to extend
‘spawn-command’ instead of introducing a new procedure—see commit
0f3276a9c3dafbef41b0aab88ba5dda1bb78dc99.

Another difference is explicitly listing keyword arguments so that their
default values are taken from the caller’s dynamic state and not from
that of the process monitoring fiber.  This fixes
<https://issues.guix.gnu.org/60106>.

Let me know what you think!

Thanks,
Ludo’.

Message #17 received at 61803 <at> debbugs.gnu.org (full text, mbox):

From: Ulf Herrman <striness <at> tilde.club>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 61803 <at> debbugs.gnu.org, Ulf Herrman <striness <at> tilde.club>
Subject: Re: bug#61803: [PATCH 0/3] [shepherd] improve race-free spawn+wait
Date: Wed, 08 Mar 2023 21:48:54 -0600

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

>> From 177592ee9d4b7fc6dcc80e545e8ad615a1d6786c Mon Sep 17 00:00:00 2001
>> From: ulfvonbelow <striness <at> 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 don't.

> I’m inclined to recommend against its use, in which case this patch is
> unnecessary.

I tend to agree, but make-system-constructor and make-system-destructor
both use it and are documented in the manual, so we should either make
them work properly or remove them.

>> +(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"))
>

The rationale behind not taking that straightforward approach was to
closely emulate the normal behavior of 'system' on guix, where the shell
path used is a hardcoded store path, though since guix's libc is likely
the only one where this is anything other than /bin/sh, I suppose it
does make a lot more sense to patch it in the guix package definition
(or accept the minor behavioral difference) than to try to automagically
figure it out at configure-time, which also has the problems you
mentioned.

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 61803 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ulf Herrman <striness <at> tilde.club>
Cc: 61803 <at> debbugs.gnu.org
Subject: Re: bug#61803: [PATCH 0/3] [shepherd] improve race-free spawn+wait
Date: Sat, 11 Mar 2023 15:57:43 +0100

Hi Ulf,

Ulf Herrman <striness <at> tilde.club> skribis:

> I tend to agree, but make-system-constructor and make-system-destructor
> both use it and are documented in the manual, so we should either make
> them work properly or remove them.

Yeah, you’re right.  So I guess we need to support it for now and start
a deprecation cycle so we can eventually remove it.

For now, I’ve pushed a simplified version of your patch as
89dd3bb57fa3e3a23cf85385b0788046b7e45170.

Let me know if you notice something wrong!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.