[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Carlo Zancanaro]

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

Hey,

I use Shepherd to manage my user session, and if I log out then 
Shepherd leaves all my services running. This patch handles 
SIGTERM and SIGHUP to prevent that.

I hope the guix-patches mailing list was the right place to send 
it. From what I understand Shepherd development is done through 
the guix lists, so it seemed to make the most sense to me.

It's also worth noting that I had to run `gettextize` to get 
Shepherd to build on current master, but I haven't included those 
changes in this commit (because I don't know what should be 
committed and what shouldn't).

Carlo


[-- Attachment #1.2: 0001-Terminate-all-services-upon-SIGTERM-or-SIGHUP.patch --]
[-- Type: text/x-patch, Size: 6455 bytes --]

From aabb9c6b1b52189d20339531de0b8b96bcace69f Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Tue, 20 Feb 2018 02:52:47 +1100
Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP

* modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
  root-service.
* tests/sigterm.sh, tests/sighup.sh: New files.
* Makefile.am (TESTS): Add tests/sigterm.sh and tests/sighup.sh.
---
 Makefile.am          |  5 ++++-
 modules/shepherd.scm | 11 ++++++++++
 tests/sighup.sh      | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/sigterm.sh     | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 tests/sighup.sh
 create mode 100644 tests/sigterm.sh

diff --git a/Makefile.am b/Makefile.am
index a30b11d..021857d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,7 @@
 # Makefile.am -- How to build and install the Shepherd.
 # Copyright © 2002, 2003 Wolfgang Jährling <wolfgang@pro-linux.de>
 # Copyright © 2013, 2014, 2015, 2016, 2018 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 #
 # This file is part of the GNU Shepherd.
 #
@@ -188,7 +189,9 @@ TESTS =						\
   tests/no-home.sh				\
   tests/pid-file.sh				\
   tests/status-sexp.sh				\
-  tests/sigint.sh
+  tests/sigint.sh				\
+  tests/sigterm.sh				\
+  tests/sighup.sh
 
 TEST_EXTENSIONS = .sh
 EXTRA_DIST += $(TESTS)
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index 5334657..650ba63 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -1,6 +1,7 @@
 ;; shepherd.scm -- The daemon shepherd.
 ;; Copyright (C) 2013, 2014, 2016, 2018 Ludovic Courtès <ludo@gnu.org>
 ;; Copyright (C) 2002, 2003 Wolfgang Jährling <wolfgang@pro-linux.de>
+;; Copyright (C) 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;
 ;; This file is part of the GNU Shepherd.
 ;;
@@ -182,6 +183,16 @@
       (lambda _
         (stop root-service)))
 
+    ;; Stop everything when we get SIGTERM.
+    (sigaction SIGTERM
+      (lambda _
+        (stop root-service)))
+
+    ;; Stop everything when we get SIGHUP.
+    (sigaction SIGHUP
+      (lambda _
+        (stop root-service)))
+
     ;; Ignore SIGPIPE so that we don't die if a client closes the connection
     ;; prematurely.
     (sigaction SIGPIPE SIG_IGN)
diff --git a/tests/sighup.sh b/tests/sighup.sh
new file mode 100644
index 0000000..e9ca84b
--- /dev/null
+++ b/tests/sighup.sh
@@ -0,0 +1,58 @@
+# GNU Shepherd --- Make sure SIGHUP is correctly handled.
+# Copyright © 2014, 2016 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
+#
+# 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-$$"
+stamp="t-stamp-$$"
+pid="t-pid-$$"
+
+herd="herd -s $socket"
+
+trap "rm -f $socket $conf $stamp $log;
+      test -f $pid && kill \`cat $pid\` || true; rm -f $pid" EXIT
+
+cat > "$conf"<<EOF
+(use-modules (srfi srfi-26))
+(register-services
+ (make <service>
+   #:provides '(test)
+   #:start (const #t)
+   #:stop  (lambda _
+             (call-with-output-file "$stamp"
+               (lambda (port)
+                 (display "stopped" port))))
+   #:respawn? #f))
+ (start 'test)
+EOF
+
+rm -f "$pid" "$stamp"
+shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log" &
+
+while [ ! -f "$pid" ] ; do sleep 0.5 ; done
+
+# Send SIGTERM to shepherd.
+kill -HUP "`cat "$pid"`"
+while kill -0 "`cat "$pid"`" ; do sleep 0.5 ; done
+
+# Make sure the service's 'stop' method was called.
+test -f "$stamp"
diff --git a/tests/sigterm.sh b/tests/sigterm.sh
new file mode 100644
index 0000000..f6b66be
--- /dev/null
+++ b/tests/sigterm.sh
@@ -0,0 +1,58 @@
+# GNU Shepherd --- Make sure SIGTERM is correctly handled.
+# Copyright © 2014, 2016 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
+#
+# 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-$$"
+stamp="t-stamp-$$"
+pid="t-pid-$$"
+
+herd="herd -s $socket"
+
+trap "rm -f $socket $conf $stamp $log;
+      test -f $pid && kill \`cat $pid\` || true; rm -f $pid" EXIT
+
+cat > "$conf"<<EOF
+(use-modules (srfi srfi-26))
+(register-services
+ (make <service>
+   #:provides '(test)
+   #:start (const #t)
+   #:stop  (lambda _
+             (call-with-output-file "$stamp"
+               (lambda (port)
+                 (display "stopped" port))))
+   #:respawn? #f))
+ (start 'test)
+EOF
+
+rm -f "$pid" "$stamp"
+shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log" &
+
+while [ ! -f "$pid" ] ; do sleep 0.5 ; done
+
+# Send SIGTERM to shepherd.
+kill -TERM "`cat "$pid"`"
+while kill -0 "`cat "$pid"`" ; do sleep 0.5 ; done
+
+# Make sure the service's 'stop' method was called.
+test -f "$stamp"
-- 
2.16.1


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

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Ludovic Courtès]

Hi Carlo,

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

> I use Shepherd to manage my user session, and if I log out then
> Shepherd leaves all my services running. This patch handles SIGTERM
> and SIGHUP to prevent that.

Good catch!

> I hope the guix-patches mailing list was the right place to send
> it. From what I understand Shepherd development is done through the
> guix lists, so it seemed to make the most sense to me.

Yes, that’s perfect.

> It's also worth noting that I had to run `gettextize` to get Shepherd
> to build on current master, but I haven't included those changes in
> this commit (because I don't know what should be committed and what
> shouldn't).

Right, I’m not sure what’s wrong with that.  I’ve committed the same
files under po/ as I did for Guix:

--8<---------------cut here---------------start------------->8---
$ git ls-files po/
po/Makevars
po/POTFILES.in
--8<---------------cut here---------------end--------------->8---

> From aabb9c6b1b52189d20339531de0b8b96bcace69f Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Tue, 20 Feb 2018 02:52:47 +1100
> Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP
>
> * modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
>   root-service.
> * tests/sigterm.sh, tests/sighup.sh: New files.
> * Makefile.am (TESTS): Add tests/sigterm.sh and tests/sighup.sh.

Excellent!

I have one request: since the three tests differ only in the signal
name, could you make it a single test file and have a loop like:

  for signal in SIGTERM SIGHUP SIGINT

TIA,
Ludo’.

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

On Tue, Feb 27, 2018 at 10:45:58AM +0100, Ludovic Courtès wrote:
> Carlo Zancanaro <carlo@zancanaro.id.au> skribis:
> 
> > I use Shepherd to manage my user session, and if I log out then
> > Shepherd leaves all my services running. This patch handles SIGTERM
> > and SIGHUP to prevent that.
> 
> Good catch!

"This update broke my workflow" <https://xkcd.com/1172/>

Joking aside, I think this change is correct, but it would be great to
be able to have long-running unprivileged processes, as on systemd.
There, the administrator can use `loginctl enable-linger $USER`. We'd
want to do it in the system configuration. From loginctl(1):

------
Enable/disable user lingering for one or more users. If enabled for a
specific user, a user manager is spawned for the user at boot and kept
around after logouts. This allows users who are not logged in to run
long-running services. Takes one or more user names or numeric UIDs as
argument. If no argument is specified, enables/disables lingering for
the user of the session of the caller.
------

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

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> On Tue, Feb 27, 2018 at 10:45:58AM +0100, Ludovic Courtès wrote:
>> Carlo Zancanaro <carlo@zancanaro.id.au> skribis:
>> 
>> > I use Shepherd to manage my user session, and if I log out then
>> > Shepherd leaves all my services running. This patch handles SIGTERM
>> > and SIGHUP to prevent that.
>> 
>> Good catch!
>
> "This update broke my workflow" <https://xkcd.com/1172/>
>
> Joking aside, I think this change is correct, but it would be great to
> be able to have long-running unprivileged processes, as on systemd.
> There, the administrator can use `loginctl enable-linger $USER`. We'd
> want to do it in the system configuration. From loginctl(1):
>
> ------
> Enable/disable user lingering for one or more users. If enabled for a
> specific user, a user manager is spawned for the user at boot and kept
> around after logouts. This allows users who are not logged in to run
> long-running services. Takes one or more user names or numeric UIDs as
> argument. If no argument is specified, enables/disables lingering for
> the user of the session of the caller.
> ------

Indeed, that sounds useful.

I suppose on GuixSD PID 1 could start subprocesses (with an initially
empty config file?) for selected users at boot time.  Would that make
sense?

Ludo’.

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Carlo Zancanaro]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

On Tue, Feb 27 2018, Leo Famulari wrote:
> "This update broke my workflow" <https://xkcd.com/1172/>

How seriously should I take this? I understand that it's a joke, 
but are you currently relying on this behaviour of shepherd? I 
could add an action to the root service to tell shepherd to leave 
processes behind when it is killed, if we wanted to expose that 
behaviour.

Carlo

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

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Carlo Zancanaro]

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

Hey Ludo,

On Tue, Feb 27 2018, Ludovic Courtès wrote:
>> It's also worth noting that I had to run `gettextize` to get 
>> Shepherd
>> to build on current master, but I haven't included those 
>> changes in
>> this commit (because I don't know what should be committed and 
>> what
>> shouldn't).
>
> Right, I’m not sure what’s wrong with that.  I’ve committed the 
> same
> files under po/ as I did for Guix:
>
> --8<---------------cut 
> here---------------start------------->8---
> $ git ls-files po/
> po/Makevars
> po/POTFILES.in
> --8<---------------cut 
> here---------------end--------------->8---

If you clone shepherd from the repository, can you build it? 
Comparing the guix repository to the shepherd one I can't easily 
work out what needs to be done for this, but hopefully you can at 
least reproduce my problem.

> I have one request: since the three tests differ only in the 
> signal
> name, could you make it a single test file and have a loop like:
>
>   for signal in SIGTERM SIGHUP SIGINT

Updated patch is attached.

Carlo


[-- Attachment #1.2: 0001-Terminate-all-services-upon-SIGTERM-or-SIGHUP.patch --]
[-- Type: text/x-patch, Size: 3604 bytes --]

From 717456edd92ba753daf5dd40e2f8d51eab3e7a73 Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Tue, 20 Feb 2018 02:52:47 +1100
Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP

* modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
  root-service.
* tests/sigint.sh: Rename to...
* tests/signals.sh: ... this, and add tests for SIGTERM and SIGUP.
---
 Makefile.am                     |  3 ++-
 modules/shepherd.scm            | 11 +++++++++++
 tests/{sigint.sh => signals.sh} | 23 ++++++++++++++---------
 3 files changed, 27 insertions(+), 10 deletions(-)
 rename tests/{sigint.sh => signals.sh} (72%)

diff --git a/Makefile.am b/Makefile.am
index a30b11d..1c394e1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,6 +1,7 @@
 # Makefile.am -- How to build and install the Shepherd.
 # Copyright © 2002, 2003 Wolfgang Jährling <wolfgang@pro-linux.de>
 # Copyright © 2013, 2014, 2015, 2016, 2018 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 #
 # This file is part of the GNU Shepherd.
 #
@@ -188,7 +189,7 @@ TESTS =						\
   tests/no-home.sh				\
   tests/pid-file.sh				\
   tests/status-sexp.sh				\
-  tests/sigint.sh
+  tests/signals.sh
 
 TEST_EXTENSIONS = .sh
 EXTRA_DIST += $(TESTS)
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index 5334657..650ba63 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -1,6 +1,7 @@
 ;; shepherd.scm -- The daemon shepherd.
 ;; Copyright (C) 2013, 2014, 2016, 2018 Ludovic Courtès <ludo@gnu.org>
 ;; Copyright (C) 2002, 2003 Wolfgang Jährling <wolfgang@pro-linux.de>
+;; Copyright (C) 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;
 ;; This file is part of the GNU Shepherd.
 ;;
@@ -182,6 +183,16 @@
       (lambda _
         (stop root-service)))
 
+    ;; Stop everything when we get SIGTERM.
+    (sigaction SIGTERM
+      (lambda _
+        (stop root-service)))
+
+    ;; Stop everything when we get SIGHUP.
+    (sigaction SIGHUP
+      (lambda _
+        (stop root-service)))
+
     ;; Ignore SIGPIPE so that we don't die if a client closes the connection
     ;; prematurely.
     (sigaction SIGPIPE SIG_IGN)
diff --git a/tests/sigint.sh b/tests/signals.sh
similarity index 72%
rename from tests/sigint.sh
rename to tests/signals.sh
index 7354848..acb254a 100644
--- a/tests/sigint.sh
+++ b/tests/signals.sh
@@ -1,5 +1,6 @@
-# GNU Shepherd --- Make sure SIGINT is correctly handled.
+# GNU Shepherd --- Make sure SIGINT, SIGTERM, and SIGHUP are correctly handled.
 # Copyright © 2014, 2016 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2018 Carlo Zancanaro <carlo@zancanaro.id.au>
 #
 # This file is part of the GNU Shepherd.
 #
@@ -44,14 +45,18 @@ cat > "$conf"<<EOF
  (start 'test)
 EOF
 
-rm -f "$pid" "$stamp"
-shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log" &
+for signal in INT TERM HUP; do
 
-while [ ! -f "$pid" ] ; do sleep 0.5 ; done
+  rm -f "$pid" "$stamp" "$socket"
+  shepherd -I -s "$socket" -c "$conf" --pid="$pid" --log="$log" &
 
-# Send SIGINT to shepherd.
-kill -INT "`cat "$pid"`"
-while kill -0 "`cat "$pid"`" ; do sleep 0.5 ; done
+  while [ ! -f "$pid" ] ; do sleep 0.5 ; done
 
-# Make sure the service's 'stop' method was called.
-test -f "$stamp"
+  # Send signal to shepherd.
+  kill -$signal "`cat "$pid"`"
+  while kill -0 "`cat "$pid"`" ; do sleep 0.5 ; done
+
+  # Make sure the service's 'stop' method was called.
+  test -f "$stamp"
+
+done
-- 
2.16.1


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

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Ludovic Courtès]

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

> On Tue, Feb 27 2018, Leo Famulari wrote:
>> "This update broke my workflow" <https://xkcd.com/1172/>
>
> How seriously should I take this? I understand that it's a joke, but
> are you currently relying on this behaviour of shepherd? I could add
> an action to the root service to tell shepherd to leave processes
> behind when it is killed, if we wanted to expose that behaviour.

Maybe with an option: --signal-handling-style=leo's?

:-)

Ludo’.

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Ludovic Courtès]

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

> From 717456edd92ba753daf5dd40e2f8d51eab3e7a73 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Tue, 20 Feb 2018 02:52:47 +1100
> Subject: [PATCH] Terminate all services upon SIGTERM or SIGHUP
>
> * modules/shepherd.scm (main): Add SIGTERM and SIGHUP handlers which stop
>   root-service.
> * tests/sigint.sh: Rename to...
> * tests/signals.sh: ... this, and add tests for SIGTERM and SIGUP.

Perfect.  Applied, thanks!

Ludo’.

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Wed, Feb 28, 2018 at 08:19:15AM +1100, Carlo Zancanaro wrote:
> On Tue, Feb 27 2018, Leo Famulari wrote:
> > "This update broke my workflow" <https://xkcd.com/1172/>
> 
> How seriously should I take this? I understand that it's a joke, but are you
> currently relying on this behaviour of shepherd? I could add an action to
> the root service to tell shepherd to leave processes behind when it is
> killed, if we wanted to expose that behaviour.

I am using it, but it's not critical. I can stay logged in with tmux or
something after this change. I'd rather we address this use case as
described previously, with something like 'enable-linger'.

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

[bug#30532] [PATCH] Shepherd: Terminate all services upon SIGTERM or SIGHUP

[Carlo Zancanaro]

[-- Attachment #1: Type: text/plain, Size: 1200 bytes --]

Hey Leo,

On Fri, Mar 02 2018, Leo Famulari wrote:
> I'd rather we address this use case as described previously, 
> with something like 'enable-linger'.

I've not used the enable-linger functionality of systemd, but 
after doing some reading, maybe it could work like this?

At boot-time we could start an instance of shepherd for each 
permitted user (maybe with a `lingering-user-service-type` which 
adds a shepherd service?). It loads ~/.config/shepherd/init.scm, 
and starts any services which are enabled. It also adds a 
pseudo-service "login", which allows a user to specify a 
dependency on there being a current user session (so shepherd can 
be told to start processes on login, and to terminate them on 
logout).

For users who don't have a `lingering-user-service-type` running, 
pid 1 will listen until they log-in and will start a shepherd 
instance for them on login, loading ~/.config/shepherd/init.scm. 
When they logout pid 1 will terminate their user shepherd instance 
(along with any services it started).

This will require a few more changes to shepherd before it can 
work, but does that sound like the sort of behaviour you want? 
Could you open a new bug about it?

Carlo

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