all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [bug#61803] [PATCH 0/3] [shepherd] improve race-free spawn+wait
@ 2023-02-25 22:08 Ulf Herrman
  2023-02-26 14:17 ` Tobias Geerinckx-Rice via Guix-patches via
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ulf Herrman @ 2023-02-25 22:08 UTC (permalink / raw)
  To: 61803


[-- Attachment #1.1: Type: text/plain, Size: 729 bytes --]

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-service-Propagate-exceptions-while-spawning-in-proce.patch --]
[-- Type: text/x-patch, Size: 3628 bytes --]

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.
---
 modules/shepherd/service.scm | 47 ++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index fd2ef1b..196ee44 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -1825,6 +1825,24 @@ otherwise by updating its state."
        ;; loop so we don't miss any terminated child process.
        (loop)))))
 
+(define-syntax-rule (boxed-errors exps ...)
+  (catch #t
+    (lambda ()
+      (call-with-values
+          (lambda ()
+            exps ...)
+        (lambda results
+          (list 'success results))))
+    (lambda args
+      (list 'exception args))))
+
+(define unboxed-errors
+  (match-lambda
+    (('success vals)
+     (apply values vals))
+    (('exception args)
+     (apply throw args))))
+
 (define (process-monitor channel)
   "Run a process monitor that handles requests received over @var{channel}."
   (let loop ((waiters vlist-null))
@@ -1860,11 +1878,17 @@ otherwise by updating its state."
                          waiters)))
 
       (('spawn command reply)
-       ;; Spawn COMMAND; send its exit status to REPLY when it terminates.
-       ;; This operation is atomic: the WAITERS table is updated before
-       ;; termination of PID can possibly be handled.
-       (let ((pid (fork+exec-command command)))
-         (loop (vhash-consv pid reply waiters))))
+       ;; Spawn COMMAND; send the spawn result (pid or exception) to REPLY;
+       ;; send its exit status to REPLY when it terminates.  This operation is
+       ;; atomic: the WAITERS table is updated before termination of PID can
+       ;; possibly be handled.
+       (let ((result (boxed-errors (fork+exec-command command))))
+         (put-message reply result)
+         (match result
+           (('exception . _)
+            (loop waiters))
+           (('success (pid))
+            (loop (vhash-consv pid reply waiters))))))
 
       (('await pid reply)
        ;; Await the termination of PID and send its status on REPLY.
@@ -1900,14 +1924,17 @@ context.  The process monitoring fiber is responsible for handling
 @code{SIGCHLD} and generally dealing with process creation and termination."
   (call-with-process-monitor (lambda () exp ...)))
 
+(define (spawn-via-monitor command)
+  (let ((reply (make-channel)))
+    (put-message (current-process-monitor)
+                 `(spawn ,command ,reply))
+    (unboxed-errors (get-message reply))
+    (get-message reply)))
+
 (define (spawn-command program . arguments)
   "Like 'system*' but do not block while waiting for PROGRAM to terminate."
   (if (current-process-monitor)
-      (let ((reply (make-channel)))
-        (put-message (current-process-monitor)
-                     `(spawn ,(cons program arguments)
-                             ,reply))
-        (get-message reply))
+      (spawn-via-monitor (cons program arguments))
       (apply system* program arguments)))
 
 (define default-process-termination-grace-period
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-service-accept-fork-exec-command-argument-list-in-mo.patch --]
[-- Type: text/x-patch, Size: 3365 bytes --]

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.
---
 modules/shepherd/service.scm | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index 196ee44..a36e486 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -94,6 +94,7 @@
             default-process-termination-grace-period
             exec-command
             fork+exec-command
+            fork+exec+wait-command
             default-pid-file-timeout
             read-pid-file
             make-system-constructor
@@ -1877,12 +1878,12 @@ otherwise by updating its state."
                          vlist-null
                          waiters)))
 
-      (('spawn command reply)
+      (('spawn args reply)
        ;; Spawn COMMAND; send the spawn result (pid or exception) to REPLY;
        ;; send its exit status to REPLY when it terminates.  This operation is
        ;; atomic: the WAITERS table is updated before termination of PID can
        ;; possibly be handled.
-       (let ((result (boxed-errors (fork+exec-command command))))
+       (let ((result (boxed-errors (apply fork+exec-command args))))
          (put-message reply result)
          (match result
            (('exception . _)
@@ -1924,19 +1925,26 @@ context.  The process monitoring fiber is responsible for handling
 @code{SIGCHLD} and generally dealing with process creation and termination."
   (call-with-process-monitor (lambda () exp ...)))
 
-(define (spawn-via-monitor command)
+(define (spawn-via-monitor arguments)
   (let ((reply (make-channel)))
     (put-message (current-process-monitor)
-                 `(spawn ,command ,reply))
+                 `(spawn ,arguments ,reply))
     (unboxed-errors (get-message reply))
     (get-message reply)))
 
 (define (spawn-command program . arguments)
   "Like 'system*' but do not block while waiting for PROGRAM to terminate."
   (if (current-process-monitor)
-      (spawn-via-monitor (cons program arguments))
+      (spawn-via-monitor (list (cons program arguments)))
       (apply system* program arguments)))
 
+(define (fork+exec+wait-command command . arguments)
+  "Like 'fork+exec' but also wait for PROGRAM to terminate, giving its exit
+status."
+  (if (current-process-monitor)
+      (spawn-via-monitor (cons command arguments))
+      (waitpid (apply fork+exec-command command arguments))))
+
 (define default-process-termination-grace-period
   ;; Default process termination "grace period" before we send SIGKILL.
   (make-parameter 5))
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: 0003-service-add-spawn-shell-command-replacement-for-syst.patch --]
[-- Type: text/x-patch, Size: 4417 bytes --]

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'.
---
 configure.ac                   |  1 +
 modules/shepherd.scm           |  7 +++++--
 modules/shepherd/service.scm   | 13 +++++++++++++
 modules/shepherd/system.scm.in |  5 ++++-
 4 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 6f681dc..19c177a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -32,6 +32,7 @@ guilemoduledir="${datarootdir}/guile/site/$GUILE_EFFECTIVE_VERSION"
 guileobjectdir="${libdir}/guile/$GUILE_EFFECTIVE_VERSION/site-ccache"
 AC_SUBST([guilemoduledir])
 AC_SUBST([guileobjectdir])
+AC_SUBST([SHELL])
 
 dnl Check for extra dependencies.
 GUILE_MODULE_AVAILABLE([have_fibers], [(fibers)])
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index cce0507..1f6342e 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -420,8 +420,10 @@ already ~a threads running, disabling 'signalfd' support")
 
                ;; Replace the default 'system*' binding with one that
                ;; cooperates instead of blocking on 'waitpid'.
-               (let ((real-system* system*))
+               (let ((real-system* system*)
+                     (real-system system))
                  (set! system* spawn-command)
+                 (set! system spawn-shell-command)
 
                  ;; Restore 'system*' after fork.
                  (set! primitive-fork
@@ -430,7 +432,8 @@ already ~a threads running, disabling 'signalfd' support")
                            (let ((result (real-fork)))
                              (when (zero? result)
                                (set! primitive-fork real-fork)
-                               (set! system* real-system*))
+                               (set! system* real-system*)
+                               (set! system real-system))
                              result)))))
 
                (run-daemon #:socket-file socket-file
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index a36e486..f8df3a9 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -81,6 +81,7 @@
             handle-SIGCHLD
             with-process-monitor
             spawn-command
+            spawn-shell-command
             %precious-signals
             register-services
             provided-by
@@ -1938,6 +1939,18 @@ context.  The process monitoring fiber is responsible for handling
       (spawn-via-monitor (list (cons program arguments)))
       (apply system* program arguments)))
 
+(define real-system system)
+
+(define* (spawn-shell-command #:optional command)
+  "Like 'system' but do not block while waiting for COMMAND to terminate."
+  (if (current-process-monitor)
+      (if command
+          (spawn-command %shell-filename "-c" command)
+          #t)
+      (if command
+          (real-system command)
+          (real-system))))
+
 (define (fork+exec+wait-command command . arguments)
   "Like 'fork+exec' but also wait for PROGRAM to terminate, giving its exit
 status."
diff --git a/modules/shepherd/system.scm.in b/modules/shepherd/system.scm.in
index 29357aa..4646e81 100644
--- a/modules/shepherd/system.scm.in
+++ b/modules/shepherd/system.scm.in
@@ -41,7 +41,8 @@
             unblock-signals
             set-blocked-signals
             with-blocked-signals
-            without-automatic-finalization))
+            without-automatic-finalization
+            %shell-filename))
 
 ;; The <sys/reboot.h> constants.
 (define RB_AUTOBOOT @RB_AUTOBOOT@)
@@ -328,3 +329,5 @@ Turning finalization off shuts down the finalization thread as a side effect."
         exp ...)
       (lambda ()
         (%set-automatic-finalization-enabled?! enabled?)))))
+
+(define %shell-filename "@SHELL@")
-- 
2.38.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-11 14:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-25 22:08 [bug#61803] [PATCH 0/3] [shepherd] improve race-free spawn+wait Ulf Herrman
2023-02-26 14:17 ` Tobias Geerinckx-Rice via Guix-patches via
2023-03-02 22:16 ` Ludovic Courtès
2023-03-09  3:48   ` Ulf Herrman
2023-03-11 14:57     ` Ludovic Courtès
2023-03-04 22:09 ` Ludovic Courtès

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.