GNU bug report logs - #76262
[PATCH 0/3] Remove uses of 'waitpid' in Shepherd services

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 76262 in the body.
You can then email your comments to 76262 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: Ludovic Courtès <ludo <at> gnu.org>
To: guix-patches <at> gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 0/3] Remove uses of 'waitpid' in Shepherd services
Date: Thu, 13 Feb 2025 12:43:47 +0100

Hello,

On IRC, xelxebar reported an issue with the ‘transmission-daemon’
service.  This patch series fixes this and related anti-patterns
found in several services.

Feedback welcome!

Ludo’.

Ludovic Courtès (3):
  services: transmission: Remove custom ‘stop’ implementation.
  services: Use ‘spawn-command’ instead of ‘fork’ + ‘waitpid’.
  home: services: unclutter: Add a ‘stop’ method.

 gnu/home/services/desktop.scm |  6 +++---
 gnu/services/databases.scm    | 16 +++++++++-------
 gnu/services/file-sharing.scm | 31 +++++--------------------------
 gnu/services/networking.scm   | 26 +++++++++++++-------------
 gnu/services/web.scm          | 31 +++++++++----------------------
 5 files changed, 39 insertions(+), 71 deletions(-)


base-commit: 2afb48804e4ff829ed5ed97757fde0a2cd8e39fc
-- 
2.48.1

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: 76262 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 1/3] services: transmission: Remove custom ‘stop’ implementation.
Date: Thu, 13 Feb 2025 12:45:46 +0100

This ‘stop’ methods had two problems:

  1. It is incompatible with the Shepherd 1.0, where the running value
     is a <process> record and not a PID.

  2. It is unreliable because its ‘waitpid’ calls compete with those
     made by shepherd’s main event loop upon SIGCHLD.

* gnu/services/file-sharing.scm (transmission-daemon-shepherd-service):
Change ‘stop’ to use ‘make-kill-destructor’.

Change-Id: I406eb619d4a72bb5afe6200ac5c8f68736a78d97
---
 gnu/services/file-sharing.scm | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/gnu/services/file-sharing.scm b/gnu/services/file-sharing.scm
index 6b25cd420fd..4b6867bc070 100644
--- a/gnu/services/file-sharing.scm
+++ b/gnu/services/file-sharing.scm
@@ -648,33 +648,12 @@ (define (transmission-daemon-shepherd-service config)
                 #:log-file #$%transmission-daemon-log-file
                 #:environment-variables
                 '("CURL_CA_BUNDLE=/etc/ssl/certs/ca-certificates.crt")))
-      (stop #~(lambda (pid)
-                (kill pid SIGTERM)
 
-                ;; Transmission Daemon normally needs some time to shut down,
-                ;; as it will complete some housekeeping and send a final
-                ;; update to trackers before it exits.
-                ;;
-                ;; Wait a reasonable period for it to stop before continuing.
-                ;; If we don't do this, restarting the service can fail as the
-                ;; new daemon process finds the old one still running and
-                ;; attached to the port used for peer connections.
-                (let wait-before-killing ((period #$stop-wait-period))
-                  (if (zero? (car (waitpid pid WNOHANG)))
-                      (if (positive? period)
-                          (begin
-                            (sleep 1)
-                            (wait-before-killing (- period 1)))
-                          (begin
-                            (format #t
-                                    #$(G_ "Wait period expired; killing \
-transmission-daemon (pid ~a).~%")
-                                    pid)
-                            (display #$(G_ "(If you see this message \
-regularly, you may need to increase the value
-of 'stop-wait-period' in the service configuration.)\n"))
-                            (kill pid SIGKILL)))))
-                #f))
+      ;; Transmission Daemon normally needs some time to shut down, as it will
+      ;; complete some housekeeping and send a final update to trackers before
+      ;; it exits.
+      (stop #~(make-kill-destructor #:grace-period #$stop-wait-period))
+
       (actions
        (list
         (shepherd-action
-- 
2.48.1

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: 76262 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 2/3] services: Use ‘spawn-command’ instead of ‘fork’ + ‘waitpid’.
Date: Thu, 13 Feb 2025 12:45:47 +0100

This is more concise and more robust: these ‘waitpid’ calls would
compete with those made by shepherd’s event loop upon SIGCHLD, and they
could hang forever.

* gnu/services/databases.scm (postgresql-role-shepherd-service): Use
‘spawn-command’ instead of ‘fork+exec-command’ followed by ‘waitpid’.
* gnu/services/networking.scm (dhcp-client-shepherd-service): Change
‘start’ to use ‘spawn-command’ instead of ‘fork+exec-command’ and
* gnu/services/web.scm (patchwork-django-admin-gexp): Use
‘spawn-command’ instead of ‘primitive-fork’ + ‘waitpid’.

Change-Id: I449290bfa46f8600e6ccdb5a6da990ad0cb7948c
---
 gnu/services/databases.scm  | 16 +++++++++-------
 gnu/services/networking.scm | 26 +++++++++++++-------------
 gnu/services/web.scm        | 31 +++++++++----------------------
 3 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/gnu/services/databases.scm b/gnu/services/databases.scm
index e8a4acc996d..6530c6f0a12 100644
--- a/gnu/services/databases.scm
+++ b/gnu/services/databases.scm
@@ -1,6 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015 David Thompson <davet <at> gnu.org>
-;;; Copyright © 2015-2016, 2022-2023 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2015-2016, 2022-2023, 2025 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2016 Leo Famulari <leo <at> famulari.name>
 ;;; Copyright © 2017 Christopher Baines <mail <at> cbaines.net>
 ;;; Copyright © 2018 Clément Lassieur <clement <at> lassieur.org>
@@ -448,12 +448,14 @@ (define (postgresql-role-shepherd-service config)
            (one-shot? #t)
            (start
             #~(lambda args
-                (let ((pid (fork+exec-command
-                            #$(postgresql-create-roles config)
-                            #:user "postgres"
-                            #:group "postgres"
-                            #:log-file #$log)))
-                  (zero? (cdr (waitpid pid))))))
+                (zero? (spawn-command
+                        #$(postgresql-create-roles config)
+                        #:user "postgres"
+                        #:group "postgres"
+                        ;; XXX: As of Shepherd 1.0.2, #:log-file is not
+                        ;; supported.
+                        ;; #:log-file #$log
+                        ))))
            (documentation "Create PostgreSQL roles.")))))
 
 (define postgresql-role-service-type
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index af28bd0626b..53f383f6f3d 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2013-2024 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2013-2025 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2015 Mark H Weaver <mhw <at> netris.org>
 ;;; Copyright © 2016, 2018, 2020 Efraim Flashner <efraim <at> flashner.co.il>
 ;;; Copyright © 2016 John Darrington <jmd <at> gnu.org>
@@ -389,18 +389,18 @@ (define dhcp-client-shepherd-service
                                  '()))
 
                            (false-if-exception (delete-file #$pid-file))
-                           (let ((pid (fork+exec-command
-                                       ;; By default dhclient uses a
-                                       ;; pre-standardization implementation of
-                                       ;; DDNS, which is incompatable with
-                                       ;; non-ISC DHCP servers; thus, pass '-I'.
-                                       ;; <https://kb.isc.org/docs/aa-01091>.
-                                       `(,dhclient "-nw" "-I"
-                                                   #$(string-append "-" version)
-                                                   "-pf" ,#$pid-file
-                                                   ,@config-file-args
-                                                   ,@ifaces))))
-                             (and (zero? (cdr (waitpid pid)))
+                           (let ((status (spawn-command
+                                          ;; By default dhclient uses a
+                                          ;; pre-standardization implementation of
+                                          ;; DDNS, which is incompatable with
+                                          ;; non-ISC DHCP servers; thus, pass '-I'.
+                                          ;; <https://kb.isc.org/docs/aa-01091>.
+                                          `(,dhclient "-nw" "-I"
+                                                      #$(string-append "-" version)
+                                                      "-pf" ,#$pid-file
+                                                      ,@config-file-args
+                                                      ,@ifaces))))
+                             (and (zero? status)
                                   (read-pid-file #$pid-file)))))
                 (stop #~(make-kill-destructor)))))))
     (package
diff --git a/gnu/services/web.scm b/gnu/services/web.scm
index f5de5997acb..d42ef09c3c0 100644
--- a/gnu/services/web.scm
+++ b/gnu/services/web.scm
@@ -1,6 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015 David Thompson <davet <at> gnu.org>
-;;; Copyright © 2015-2023 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2015-2023, 2025 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2016 Nikita <nikita <at> n0.is>
 ;;; Copyright © 2016, 2017, 2018 Julien Lepiller <julien <at> lepiller.eu>
 ;;; Copyright © 2017, 2018, 2019 Christopher Baines <mail <at> cbaines.net>
@@ -1902,27 +1902,14 @@ (define (patchwork-httpd-configuration patchwork-configuration)
 
 (define (patchwork-django-admin-gexp patchwork settings-module)
   #~(lambda command
-      (let ((pid (primitive-fork))
-            (user (getpwnam "httpd")))
-        (if (eq? pid 0)
-            (dynamic-wind
-              (const #t)
-              (lambda ()
-                (setgid (passwd:gid user))
-                (setuid (passwd:uid user))
-
-                (setenv "DJANGO_SETTINGS_MODULE" "guix.patchwork.settings")
-                (setenv "PYTHONPATH" #$settings-module)
-                (primitive-exit
-                 (if (zero?
-                      (apply system*
-                             #$(file-append patchwork "/bin/patchwork-admin")
-                             command))
-                     0
-                     1)))
-              (lambda ()
-                (primitive-exit 1)))
-            (zero? (cdr (waitpid pid)))))))
+      (zero? (spawn-command
+              `(#$(file-append patchwork "/bin/patchwork-admin")
+                ,command)
+              #:user "httpd"
+              #:group "httpd"
+              #:environment-variables
+              `("DJANGO_SETTINGS_MODULE=guix.patchwork.settings"
+                ,(string-append "PYTHONPATH=" #$settings-module))))))
 
 (define (patchwork-django-admin-action patchwork settings-module)
   (shepherd-action
-- 
2.48.1

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: 76262 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 3/3] home: services: unclutter: Add a ‘stop’ method.
Date: Thu, 13 Feb 2025 12:45:48 +0100

* gnu/home/services/desktop.scm (home-unclutter-shepherd-service):
Remove ‘one-shot?’ field and set ‘stop’.

Change-Id: I82b915d4260a62e628b419a497c50ecf2cbc356c
---
 gnu/home/services/desktop.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gnu/home/services/desktop.scm b/gnu/home/services/desktop.scm
index fc96ce92954..859dba776a0 100644
--- a/gnu/home/services/desktop.scm
+++ b/gnu/home/services/desktop.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2022-2023 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2022-2023, 2025 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2022 ( <paren <at> disroot.org>
 ;;; Copyright © 2023 conses <contact <at> conses.eu>
 ;;; Copyright © 2023 Janneke Nieuwenhuizen <janneke <at> gnu.org>
@@ -353,7 +353,6 @@ (define (home-unclutter-shepherd-service config)
     (modules '((shepherd support) ;for %user-log-dir
                (srfi srfi-1)
                (srfi srfi-26)))
-    (one-shot? #t)
     (start #~(lambda _
                (fork+exec-command
                 (list
@@ -369,7 +368,8 @@ (define (home-unclutter-shepherd-service config)
                       (remove (cut string-prefix? "DISPLAY=" <>)
                               (default-environment-variables)))
                 #:log-file
-                (string-append %user-log-dir "/unclutter.log")))))))
+                (string-append %user-log-dir "/unclutter.log"))))
+    (stop #~(make-kill-destructor)))))
 
 (define home-unclutter-service-type
   (service-type
-- 
2.48.1

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: 76262 <at> debbugs.gnu.org
Subject: Re: [bug#76262] [PATCH 2/3] services: Use ‘spawn-command’ instead
 of ‘fork’ + ‘waitpid’.
Date: Wed, 19 Feb 2025 22:10:29 +0100

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

> This is more concise and more robust: these ‘waitpid’ calls would
> compete with those made by shepherd’s event loop upon SIGCHLD, and they
> could hang forever.
>
> * gnu/services/databases.scm (postgresql-role-shepherd-service): Use
> ‘spawn-command’ instead of ‘fork+exec-command’ followed by ‘waitpid’.
> * gnu/services/networking.scm (dhcp-client-shepherd-service): Change
> ‘start’ to use ‘spawn-command’ instead of ‘fork+exec-command’ and
> * gnu/services/web.scm (patchwork-django-admin-gexp): Use
> ‘spawn-command’ instead of ‘primitive-fork’ + ‘waitpid’.
>
> Change-Id: I449290bfa46f8600e6ccdb5a6da990ad0cb7948c

Concrete manifestation of the bug with ‘dhcp-client-service-type’:

  https://issues.guix.gnu.org/76315

Ludo’.

Message #22 received at 76262-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: 76262-done <at> debbugs.gnu.org
Cc: Janneke Nieuwenhuizen <janneke <at> gnu.org>,
 Tanguy Le Carrour <tanguy <at> bioneland.org>, Andrew Tropin <andrew <at> trop.in>
Subject: Re: [bug#76262] [PATCH 0/3] Remove uses of 'waitpid' in Shepherd
 services
Date: Fri, 21 Feb 2025 15:31:40 +0100

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

> On IRC, xelxebar reported an issue with the ‘transmission-daemon’
> service.  This patch series fixes this and related anti-patterns
> found in several services.

Pushed:

  90aa90eb054 origin/master home: services: unclutter: Add a ‘stop’ method.
  e36d6ab24b7 services: Use ‘spawn-command’ instead of ‘fork’ + ‘waitpid’.
  9f77db78e6b services: transmission: Remove custom ‘stop’ implementation.

Ludo’.

Previous Next

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