[PATCH 0/3] Remove uses of 'waitpid' in Shepherd services

  • Done
  • quality assurance status badge
Details
One participant
  • Ludovic Courtès
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
normal

Debbugs page

Ludovic Courtès wrote on 13 Feb 12:43 +0100
(address . guix-patches@gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
cover.1739446902.git.ludo@gnu.org
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
Ludovic Courtès wrote on 13 Feb 12:45 +0100
[PATCH 1/3] services: transmission: Remove custom ‘stop’ implementation.
(address . 76262@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
29a32c21470e99e9457c9f9258f45a84375c26ed.1739446902.git.ludo@gnu.org
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(-)

Toggle diff (45 lines)
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
Ludovic Courtès wrote on 13 Feb 12:45 +0100
[PATCH 2/3] services: Use ‘spawn-command ’ instead of ‘fork’ + ‘waitpid’.
(address . 76262@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
b1e7940c0c4186433ab6a44915624e2dbf41a3de.1739446902.git.ludo@gnu.org
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(-)

Toggle diff (125 lines)
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@gnu.org>
-;;; Copyright © 2015-2016, 2022-2023 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2015-2016, 2022-2023, 2025 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2016 Leo Famulari <leo@famulari.name>
;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>
;;; Copyright © 2018 Clément Lassieur <clement@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@gnu.org>
+;;; Copyright © 2013-2025 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2015 Mark H Weaver <mhw@netris.org>
;;; Copyright © 2016, 2018, 2020 Efraim Flashner <efraim@flashner.co.il>
;;; Copyright © 2016 John Darrington <jmd@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@gnu.org>
-;;; Copyright © 2015-2023 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2015-2023, 2025 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2016 Nikita <nikita@n0.is>
;;; Copyright © 2016, 2017, 2018 Julien Lepiller <julien@lepiller.eu>
;;; Copyright © 2017, 2018, 2019 Christopher Baines <mail@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
Ludovic Courtès wrote on 13 Feb 12:45 +0100
[PATCH 3/3] home: services: unclutter: Add a ‘stop’ method.
(address . 76262@debbugs.gnu.org)(name . Ludovic Courtès)(address . ludo@gnu.org)
be5707b2ec27fb0d15ea246e3263db3ff45f52ec.1739446902.git.ludo@gnu.org
* 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(-)

Toggle diff (31 lines)
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@gnu.org>
+;;; Copyright © 2022-2023, 2025 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2022 ( <paren@disroot.org>
;;; Copyright © 2023 conses <contact@conses.eu>
;;; Copyright © 2023 Janneke Nieuwenhuizen <janneke@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
Ludovic Courtès wrote 2 days ago
Re: [bug#76262] [PATCH 2/3] services: Use ‘spaw n-command’ instead of ‘fork’ + ‘waitpid’.
(address . 76262@debbugs.gnu.org)
87a5ahhc8q.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (13 lines)
> 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’:


Ludo’.
Ludovic Courtès wrote 15 hours ago
Re: [bug#76262] [PATCH 0/3] Remove uses of 'waitpid' in Shepherd services
(address . 76262-done@debbugs.gnu.org)
874j0n74j7.fsf@gnu.org
Ludovic Courtès <ludo@gnu.org> skribis:

Toggle quote (4 lines)
> 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’.
Closed
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 76262@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 76262
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch