[shepherd] branch master updated: shepherd: Restart signal handler after 'daemonize'.

[Ludovic Courtès]

This is an automated email from the git hooks/post-receive script.

civodul pushed a commit to branch master
in repository shepherd.

The following commit(s) were added to refs/heads/master by this push:
     new f4272d2  shepherd: Restart signal handler after 'daemonize'.
f4272d2 is described below

commit f4272d2f0f393d2aa3e9d76b36ab6aa5f2fc72c2
Author: Ludovic Courtès <ludo@gnu.org>
AuthorDate: Wed Jul 12 18:58:39 2023 +0200

    shepherd: Restart signal handler after 'daemonize'.
    
    Fixes <https://issues.guix.gnu.org/63982>.
    
    * modules/shepherd/service.scm (%post-daemonize-hook): New variable.
    (root-service): Run it upon 'daemonize'.
    * modules/shepherd.scm (run-daemon): Call 'add-hook!' on
    '%post-daemonize-hook'.  Rename 'signal-handler' to 'signal-thunk' and
    turn it into a thunk.
    * tests/daemonize.sh: New file.
    * Makefile.am (TESTS): Add it.
    * NEWS: Update.
    
    Reported-by: Maxim Cournoyer <maxim.cournoyer@gmail.com>
---
 Makefile.am                  |  1 +
 NEWS                         |  7 ++++++
 modules/shepherd.scm         | 19 ++++++++++++++--
 modules/shepherd/service.scm |  6 +++++
 tests/daemonize.sh           | 54 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 871b78d..fdfcf3d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -268,6 +268,7 @@ TESTS =                                             \
   tests/signals.sh                             \
   tests/system-star.sh                         \
   tests/close-on-exec.sh                       \
+  tests/daemonize.sh                           \
   tests/eval-load.sh                           \
   tests/services/monitoring.sh                 \
   tests/services/repl.sh
diff --git a/NEWS b/NEWS
index f17a155..b3d7d2e 100644
--- a/NEWS
+++ b/NEWS
@@ -47,6 +47,13 @@ When replacing a service, for instance by running ‘herd load 
root conf.scm’
 by running ‘guix system reconfigure’, the service replacement starts as
 disabled if the original service was disabled.
 
+** Signals are properly handled after ‘daemonize’
+   (<https://issues.guix.gnu.org/63982>)
+
+Starting with version 0.9.0, calling the ‘daemonize’ action on the ‘root’
+service would cause shepherd to miss signals; in particular, it would miss
+SIGCHLD signals, making it hardly usable.  This is now fixed.
+
 ** New Bash completion
 
 A Bash completion file is now installed, providing tab completion for the
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index e1e7669..2730213 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -188,7 +188,7 @@ already ~a threads running, disabling 'signalfd' support")
 
 (define* (run-daemon #:key (config-file (default-config-file))
                      socket-file pid-file signal-port poll-services?)
-  (define signal-handler
+  (define (signal-thunk signal-port)
     ;; Thunk that waits for signals (particularly SIGCHLD) and handles them.
     (if signal-port
         (lambda ()
@@ -212,9 +212,24 @@ already ~a threads running, disabling 'signalfd' support")
   ;; mark them all as FD_CLOEXEC so child processes do not inherit them.
   (mark-as-close-on-exec)
 
+  (when signal-port
+    ;; When the 'daemonize' action is invoked, open a new signal port in the
+    ;; child process and spawn a new fiber reading it.  This required because
+    ;; after fork(2), epoll_wait(2), which is used by Fibers, does not signal
+    ;; that the signal FD is ready for reading--see the signalfd(2) man page.
+    (add-hook! %post-daemonize-hook
+               (lambda ()
+                 (local-output (l10n "Restarting signal handler."))
+                 (close-port signal-port)
+                 (spawn-fiber
+                  (essential-task-thunk
+                   'signal-handler
+                   (signal-thunk (maybe-signal-port %precious-signals)))))))
+
   ;; Spawn a signal handling fiber.
   (spawn-fiber
-   (essential-task-thunk 'signal-handler signal-handler))
+   (essential-task-thunk 'signal-handler
+                         (signal-thunk signal-port)))
 
   ;; Load CONFIG-FILE in another fiber.  If loading fails, report it but keep
   ;; going: the user can use 'herd load root' with a new config file if
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index 4fb811a..6b8c562 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -128,6 +128,7 @@
 
             check-for-dead-services
             root-service
+            %post-daemonize-hook
 
             &service-error
             service-error?
@@ -2715,6 +2716,10 @@ where prctl/PR_SET_CHILD_SUBREAPER is unsupported."
                                         running (service-canonical-name 
service))
                             (respawn-service service))))))
 
+(define %post-daemonize-hook
+  ;; Hook invoked after the 'daemonize' action in the child process.
+  (make-hook))
+
 (define root-service
   (service
    '(root shepherd)
@@ -2827,6 +2832,7 @@ we want to receive these signals."
                (if (zero? (primitive-fork))
                    (begin
                      (catch-system-error (prctl PR_SET_CHILD_SUBREAPER 1))
+                     (run-hook %post-daemonize-hook)
                      (local-output (l10n "Now running as process ~a.")
                                    (getpid))
                      #t)
diff --git a/tests/daemonize.sh b/tests/daemonize.sh
new file mode 100644
index 0000000..b6bd640
--- /dev/null
+++ b/tests/daemonize.sh
@@ -0,0 +1,54 @@
+# GNU Shepherd --- Check the 'daemonize' action of the root service.
+# Copyright © 2023 Ludovic Courtès <ludo@gnu.org>
+#
+# This file is part of the GNU Shepherd.
+#
+# The GNU Shepherd is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or (at
+# your option) any later version.
+#
+# The GNU Shepherd is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with the GNU Shepherd.  If not, see <http://www.gnu.org/licenses/>.
+
+shepherd --version
+herd --version
+
+socket="t-socket-$$"
+conf="t-conf-$$"
+log="t-log-$$"
+pid="t-pid-$$"
+
+herd="herd -s $socket"
+
+trap "rm -f $socket $conf $log;
+      test -f $pid && kill \`cat $pid\` || true; rm -f $pid" EXIT
+
+cat > "$conf" <<EOF
+(define s
+  (service
+    '(a)
+    #:start (make-system-constructor "echo STARTING")
+    #:stop (make-system-destructor "echo STOPPING")))
+
+(register-services (list s))
+
+(perform-service-action root-service 'daemonize)
+
+(start-service s)
+EOF
+
+rm -f "$pid" "$socket"
+shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log"
+
+until test -f "$pid"; do sleep 0.5; done
+
+$herd status
+until $herd status a | grep running; do sleep 0.3; done
+
+$herd stop root