[PATCH shepherd] Register SIGCHLD handler after primitive fork

  • Done
  • quality assurance status badge
Details
2 participants
  • Jelle Licht
  • Ludovic Courtès
Owner
unassigned
Submitted by
Jelle Licht
Severity
normal
J
J
Jelle Licht wrote on 2 Jul 2017 03:11
(address . guix-patches@gnu.org)
CAPsKtf+QdRA-xJBddN2DU1Ad7hYntwmZVQ9RtTaLcFZ58aa5YA@mail.gmail.com
Hi all,

I am not sure if this is also the proper ML for the GNU Shepherd, but
looking in the archives lead me to believe it actually is. If not, I
suggest the gnu.org page for shepherd be updated with the correct info.

I recently starting playing around with user shepherd, and found out that
when running a shepherd 0.3.2 daemonized as non-init process (via "(action
'shepherd 'daemonize)"), zombie processes are created whenever you start
and subsequently stop any service.

Thinking I did something wrong, I asked lfam on #guix to share his (very
helpful) init.scm for user shepherd, yet I still noticed the same behaviour.

I believe commit `efa2f45c5f7dc735407381b7b8a83d6c37f828db' inadvertently
introduced an ordering issue, where the SIGCHLD handler is registered
/before/ shepherd has the chance to daemonize. I believe the following
trivial patch addresses this snafu.

Regards,
Jelle
Attachment: file
From 2603a1f4201668c13f992dbde3dd46be4dc2c31d Mon Sep 17 00:00:00 2001
From: Jelle Licht <jlicht@fsfe.org>
Date: Sun, 2 Jul 2017 02:58:36 +0200
Subject: [PATCH] Register SIGCHLD handler after primitive-fork.

* modules/shepherd.scm (main): Move call to 'sigaction' so daemonized process
can handle SIGCHLD signal.
---
modules/shepherd.scm | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

Toggle diff (26 lines)
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index f7c169d..dfae7d6 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -142,9 +142,6 @@
;; Start the 'root' service.
(start root-service)
- ;; Install the SIGCHLD handler.
- (sigaction SIGCHLD respawn-service SA_NOCLDSTOP)
-
;; This _must_ succeed. (We could also put the `catch' around
;; `main', but it is often useful to get the backtrace, and
;; `caught-error' does not do this yet.)
@@ -164,6 +161,9 @@
(apply format #f (gettext (cadr args)) (caddr args))
(quit 1))))
+ ;; Install the SIGCHLD handler.
+ (sigaction SIGCHLD respawn-service SA_NOCLDSTOP)
+
(when (provided? 'threads)
;; XXX: This terrible hack allows us to make sure that signal handlers
;; get a chance to run in a timely fashion. Without it, after an EINTR,
--
2.13.2
L
L
Ludovic Courtès wrote on 12 Jul 2017 23:34
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 27553@debbugs.gnu.org)
87a849bar1.fsf@gnu.org
Hi Jelle,

Jelle Licht <jlicht@fsfe.org> skribis:

Toggle quote (4 lines)
> I am not sure if this is also the proper ML for the GNU Shepherd, but
> looking in the archives lead me to believe it actually is. If not, I
> suggest the gnu.org page for shepherd be updated with the correct info.

It’s the right list. :-)

Toggle quote (13 lines)
> I recently starting playing around with user shepherd, and found out that
> when running a shepherd 0.3.2 daemonized as non-init process (via "(action
> 'shepherd 'daemonize)"), zombie processes are created whenever you start
> and subsequently stop any service.
>
> Thinking I did something wrong, I asked lfam on #guix to share his (very
> helpful) init.scm for user shepherd, yet I still noticed the same behaviour.
>
> I believe commit `efa2f45c5f7dc735407381b7b8a83d6c37f828db' inadvertently
> introduced an ordering issue, where the SIGCHLD handler is registered
> /before/ shepherd has the chance to daemonize. I believe the following
> trivial patch addresses this snafu.

The config file can start services, so the SIGCHLD handler must be
installed before we read the config file (otherwise we could be missing
some process termination notifications.)

Perhaps a solution would be to install the SIGCHLD handler lazily upon
the first ‘fork+exec-command’ call? That would ensure both that (1)
users have a chance to daemonize before the handler is installed, and
(2) that the handler is installed before services are started.

Thoughts?

Ludo’.
J
J
Jelle Licht wrote on 14 Jul 2017 14:19
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 27553@debbugs.gnu.org)
CAPsKtfKyjaoKmtekfFfM1ZWmE-eT8ibVqg-7QEA5UCTBQF4nNw@mail.gmail.com
Hi Ludo,

2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo@gnu.org>:

Toggle quote (10 lines)
> Hi Jelle,
>
> Jelle Licht <jlicht@fsfe.org> skribis:
>
> > I am not sure if this is also the proper ML for the GNU Shepherd, but
> > looking in the archives lead me to believe it actually is. If not, I
> > suggest the gnu.org page for shepherd be updated with the correct info.
>
> It’s the right list. :-)
>
I am glad it turned out to be :-). Perhaps [1] can be updated to the same
info as [2]?


Toggle quote (21 lines)
>
> > I recently starting playing around with user shepherd, and found out that
> > when running a shepherd 0.3.2 daemonized as non-init process (via
> "(action
> > 'shepherd 'daemonize)"), zombie processes are created whenever you start
> > and subsequently stop any service.
> >
> > Thinking I did something wrong, I asked lfam on #guix to share his (very
> > helpful) init.scm for user shepherd, yet I still noticed the same
> behaviour.
> >
> > I believe commit `efa2f45c5f7dc735407381b7b8a83d6c37f828db'
> inadvertently
> > introduced an ordering issue, where the SIGCHLD handler is registered
> > /before/ shepherd has the chance to daemonize. I believe the following
> > trivial patch addresses this snafu.
>
> The config file can start services, so the SIGCHLD handler must be
> installed before we read the config file (otherwise we could be missing
> some process termination notifications.)
>
What do you mean exactly? I think my config file does this, and I have not
yet noticed this issue,
but I might just be confused about what you mean here.


Toggle quote (8 lines)
>
> Perhaps a solution would be to install the SIGCHLD handler lazily upon
> the first ‘fork+exec-command’ call? That would ensure both that (1)
> users have a chance to daemonize before the handler is installed, and
> (2) that the handler is installed before services are started.
>
> Thoughts?
>
This seems like it would be for the best. I actually have no clue how to
implement this though.

Regards,
Jelle

Attachment: file
L
L
Ludovic Courtès wrote on 17 Jul 2017 10:33
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 27553@debbugs.gnu.org)
87wp77ea4g.fsf@gnu.org
Hi Jelle,

Jelle Licht <jlicht@fsfe.org> skribis:

Toggle quote (15 lines)
> 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo@gnu.org>:
>
>> Hi Jelle,
>>
>> Jelle Licht <jlicht@fsfe.org> skribis:
>>
>> > I am not sure if this is also the proper ML for the GNU Shepherd, but
>> > looking in the archives lead me to believe it actually is. If not, I
>> > suggest the gnu.org page for shepherd be updated with the correct info.
>>
>> It’s the right list. :-)
>>
> I am glad it turned out to be :-). Perhaps [1] can be updated to the same
> info as [2]?

Done!

Toggle quote (24 lines)
>> > I recently starting playing around with user shepherd, and found out that
>> > when running a shepherd 0.3.2 daemonized as non-init process (via
>> "(action
>> > 'shepherd 'daemonize)"), zombie processes are created whenever you start
>> > and subsequently stop any service.
>> >
>> > Thinking I did something wrong, I asked lfam on #guix to share his (very
>> > helpful) init.scm for user shepherd, yet I still noticed the same
>> behaviour.
>> >
>> > I believe commit `efa2f45c5f7dc735407381b7b8a83d6c37f828db'
>> inadvertently
>> > introduced an ordering issue, where the SIGCHLD handler is registered
>> > /before/ shepherd has the chance to daemonize. I believe the following
>> > trivial patch addresses this snafu.
>>
>> The config file can start services, so the SIGCHLD handler must be
>> installed before we read the config file (otherwise we could be missing
>> some process termination notifications.)
>>
> What do you mean exactly? I think my config file does this, and I have not
> yet noticed this issue,
> but I might just be confused about what you mean here.

If the config file spawns a process and that process dies before we have
installed the SIGCHLD handler, then we’ll never know that it has
terminated.

Toggle quote (10 lines)
>> Perhaps a solution would be to install the SIGCHLD handler lazily upon
>> the first ‘fork+exec-command’ call? That would ensure both that (1)
>> users have a chance to daemonize before the handler is installed, and
>> (2) that the handler is installed before services are started.
>>
>> Thoughts?
>>
> This seems like it would be for the best. I actually have no clue how to
> implement this though.

I’d imagine something like a global variable (a Boolean) telling whether
the SIGCHLD handler is installed, and then:

(unless %sigchld-handler-installed?
(sigaction …)
(set! %sigchld-handler-installed? #t))

Thoughts?

Ludo’.
J
J
Jelle Licht wrote on 27 Jul 2017 16:32
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 27553@debbugs.gnu.org)
CAPsKtfJoxtOQ0rTjT8MHNx=WwVO52+SfCB1iaGQ0LH55_FWAwA@mail.gmail.com
Hi Ludo,

The documentation for the `daemonize' action specifies the following:

Toggle quote (7 lines)
> "Go into the background. Be careful, this means that a new
> process will be created, so shepherd will not get SIGCHLD signals anymore
> if previously spawned childs terminate. Therefore, this action should
> usually only be used (if at all) *before* childs get spawned for which
> we want to receive these signals."
>
>
In a sense, the problem that you describe can then be solved by having the
lazy SIGCHLD handler be registered in two places:
- Immediately after a call to the `daemonize' action, as its documentation
that if called, it should be done before starting any services
- Before calling the lambda stored in the `start' slot of any
non-root-service service

I know how to do the first one (the newly forked process should lazily
register the handler), but the second one seems a bit harder to do.
I could add a special case to the `start' method so that it will lazily
install the handler unless we are starting the root-service, but that seems
inelegant somehow.


2017-07-17 10:33 GMT+02:00 Ludovic Courtès <ludo@gnu.org>:

Toggle quote (75 lines)
> Hi Jelle,
>
> Jelle Licht <jlicht@fsfe.org> skribis:
>
> > 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo@gnu.org>:
> >
> >> Hi Jelle,
> >>
> >> Jelle Licht <jlicht@fsfe.org> skribis:
> >>
> >> > I am not sure if this is also the proper ML for the GNU Shepherd, but
> >> > looking in the archives lead me to believe it actually is. If not, I
> >> > suggest the gnu.org page for shepherd be updated with the correct
> info.
> >>
> >> It’s the right list. :-)
> >>
> > I am glad it turned out to be :-). Perhaps [1] can be updated to the same
> > info as [2]?
>
> Done!
>
> >> > I recently starting playing around with user shepherd, and found out
> that
> >> > when running a shepherd 0.3.2 daemonized as non-init process (via
> >> "(action
> >> > 'shepherd 'daemonize)"), zombie processes are created whenever you
> start
> >> > and subsequently stop any service.
> >> >
> >> > Thinking I did something wrong, I asked lfam on #guix to share his
> (very
> >> > helpful) init.scm for user shepherd, yet I still noticed the same
> >> behaviour.
> >> >
> >> > I believe commit `efa2f45c5f7dc735407381b7b8a83d6c37f828db'
> >> inadvertently
> >> > introduced an ordering issue, where the SIGCHLD handler is registered
> >> > /before/ shepherd has the chance to daemonize. I believe the following
> >> > trivial patch addresses this snafu.
> >>
> >> The config file can start services, so the SIGCHLD handler must be
> >> installed before we read the config file (otherwise we could be missing
> >> some process termination notifications.)
> >>
> > What do you mean exactly? I think my config file does this, and I have
> not
> > yet noticed this issue,
> > but I might just be confused about what you mean here.
>
> If the config file spawns a process and that process dies before we have
> installed the SIGCHLD handler, then we’ll never know that it has
> terminated.
>
> >> Perhaps a solution would be to install the SIGCHLD handler lazily upon
> >> the first ‘fork+exec-command’ call? That would ensure both that (1)
> >> users have a chance to daemonize before the handler is installed, and
> >> (2) that the handler is installed before services are started.
> >>
> >> Thoughts?
> >>
> > This seems like it would be for the best. I actually have no clue how to
> > implement this though.
>
> I’d imagine something like a global variable (a Boolean) telling whether
> the SIGCHLD handler is installed, and then:
>
> (unless %sigchld-handler-installed?
> (sigaction …)
> (set! %sigchld-handler-installed? #t))
>
> Thoughts?
>
> Ludo’.
>
Attachment: file
J
J
Jelle Licht wrote on 7 Sep 2017 00:56
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 27553@debbugs.gnu.org)
CAPsKtf+GHDagd9yLTYkSuYUts3tSvr8aE1m1XQ2xidZrQzQMVA@mail.gmail.com
I just tested what Ludo' proposed, and it seems to work like a charm.
Seeing as we might be seeing more non-init shepherd instances w.r.t.
user services and the possible service extension to `guix environment',
I think it would be a good call to fix this bug :-).

Regards,
Jelle



2017-07-27 16:32 GMT+02:00 Jelle Licht <jlicht@fsfe.org>:

Toggle quote (106 lines)
> Hi Ludo,
>
> The documentation for the `daemonize' action specifies the following:
>
>> "Go into the background. Be careful, this means that a new
>> process will be created, so shepherd will not get SIGCHLD signals anymore
>> if previously spawned childs terminate. Therefore, this action should
>> usually only be used (if at all) *before* childs get spawned for which
>> we want to receive these signals."
>>
>>
> In a sense, the problem that you describe can then be solved by having
> the lazy SIGCHLD handler be registered in two places:
> - Immediately after a call to the `daemonize' action, as its documentation
> that if called, it should be done before starting any services
> - Before calling the lambda stored in the `start' slot of any
> non-root-service service
>
> I know how to do the first one (the newly forked process should lazily
> register the handler), but the second one seems a bit harder to do.
> I could add a special case to the `start' method so that it will lazily
> install the handler unless we are starting the root-service, but that seems
> inelegant somehow.
>
>
> 2017-07-17 10:33 GMT+02:00 Ludovic Courtès <ludo@gnu.org>:
>
>> Hi Jelle,
>>
>> Jelle Licht <jlicht@fsfe.org> skribis:
>>
>> > 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo@gnu.org>:
>> >
>> >> Hi Jelle,
>> >>
>> >> Jelle Licht <jlicht@fsfe.org> skribis:
>> >>
>> >> > I am not sure if this is also the proper ML for the GNU Shepherd, but
>> >> > looking in the archives lead me to believe it actually is. If not, I
>> >> > suggest the gnu.org page for shepherd be updated with the correct
>> info.
>> >>
>> >> It’s the right list. :-)
>> >>
>> > I am glad it turned out to be :-). Perhaps [1] can be updated to the
>> same
>> > info as [2]?
>>
>> Done!
>>
>> >> > I recently starting playing around with user shepherd, and found out
>> that
>> >> > when running a shepherd 0.3.2 daemonized as non-init process (via
>> >> "(action
>> >> > 'shepherd 'daemonize)"), zombie processes are created whenever you
>> start
>> >> > and subsequently stop any service.
>> >> >
>> >> > Thinking I did something wrong, I asked lfam on #guix to share his
>> (very
>> >> > helpful) init.scm for user shepherd, yet I still noticed the same
>> >> behaviour.
>> >> >
>> >> > I believe commit `efa2f45c5f7dc735407381b7b8a83d6c37f828db'
>> >> inadvertently
>> >> > introduced an ordering issue, where the SIGCHLD handler is registered
>> >> > /before/ shepherd has the chance to daemonize. I believe the
>> following
>> >> > trivial patch addresses this snafu.
>> >>
>> >> The config file can start services, so the SIGCHLD handler must be
>> >> installed before we read the config file (otherwise we could be missing
>> >> some process termination notifications.)
>> >>
>> > What do you mean exactly? I think my config file does this, and I have
>> not
>> > yet noticed this issue,
>> > but I might just be confused about what you mean here.
>>
>> If the config file spawns a process and that process dies before we have
>> installed the SIGCHLD handler, then we’ll never know that it has
>> terminated.
>>
>> >> Perhaps a solution would be to install the SIGCHLD handler lazily upon
>> >> the first ‘fork+exec-command’ call? That would ensure both that (1)
>> >> users have a chance to daemonize before the handler is installed, and
>> >> (2) that the handler is installed before services are started.
>> >>
>> >> Thoughts?
>> >>
>> > This seems like it would be for the best. I actually have no clue how to
>> > implement this though.
>>
>> I’d imagine something like a global variable (a Boolean) telling whether
>> the SIGCHLD handler is installed, and then:
>>
>> (unless %sigchld-handler-installed?
>> (sigaction …)
>> (set! %sigchld-handler-installed? #t))
>>
>> Thoughts?
>>
>> Ludo’.
>>
>
>
Attachment: file
From db942182224dfc0accad94897dd2122b128eef07 Mon Sep 17 00:00:00 2001
From: Jelle Licht <jlicht@fsfe.org>
Date: Thu, 7 Sep 2017 00:52:49 +0200
Subject: [PATCH] Lazily register SIGCHLD hander on first call to
'fork+exec-command'.

* modules/shepherd.scm (main): Move unconditional top-level call to 'sigaction' to...
* modules/shepherd/service.scm (fork+exec-command): here. Use new variable.
(%sigchld-handler-installed?): New variable.
---
modules/shepherd.scm | 3 ---
modules/shepherd/service.scm | 7 +++++++
2 files changed, 7 insertions(+), 3 deletions(-)

Toggle diff (41 lines)
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index f7c169d..7ea6493 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -142,9 +142,6 @@
;; Start the 'root' service.
(start root-service)
- ;; Install the SIGCHLD handler.
- (sigaction SIGCHLD respawn-service SA_NOCLDSTOP)
-
;; This _must_ succeed. (We could also put the `catch' around
;; `main', but it is often useful to get the backtrace, and
;; `caught-error' does not do this yet.)
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index 72fbc3d..b2d8bc5 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -100,6 +100,9 @@
condition->sexp))
+;; Keep track of lazy initialization of SIGCHLD handler
+(define %sigchld-handler-installed? #f)
+
;; Type of service actions.
(define-record-type <action>
(make-action name proc doc)
@@ -787,6 +790,10 @@ false."
(default-environment-variables)))
"Spawn a process that executed COMMAND as per 'exec-command', and return
its PID."
+ ;; Install the SIGCHLD handler if this is the first fork+exec-command call
+ (unless %sigchld-handler-installed?
+ (sigaction SIGCHLD respawn-service SA_NOCLDSTOP)
+ (set! %sigchld-handler-installed? #t))
(let ((pid (primitive-fork)))
(if (zero? pid)
(exec-command command
--
2.14.1
L
L
Ludovic Courtès wrote on 7 Sep 2017 16:49
(name . Jelle Licht)(address . jlicht@fsfe.org)(address . 27553-done@debbugs.gnu.org)
87o9qmtvgf.fsf@gnu.org
Heya!

Jelle Licht <jlicht@fsfe.org> skribis:

Toggle quote (5 lines)
> I just tested what Ludo' proposed, and it seems to work like a charm.
> Seeing as we might be seeing more non-init shepherd instances w.r.t.
> user services and the possible service extension to `guix environment',
> I think it would be a good call to fix this bug :-).

Indeed, thanks for the reminder.

Toggle quote (10 lines)
> From db942182224dfc0accad94897dd2122b128eef07 Mon Sep 17 00:00:00 2001
> From: Jelle Licht <jlicht@fsfe.org>
> Date: Thu, 7 Sep 2017 00:52:49 +0200
> Subject: [PATCH] Lazily register SIGCHLD hander on first call to
> 'fork+exec-command'.
>
> * modules/shepherd.scm (main): Move unconditional top-level call to 'sigaction' to...
> * modules/shepherd/service.scm (fork+exec-command): here. Use new variable.
> (%sigchld-handler-installed?): New variable.

LGTM, applied!

Ludo’.
Closed
?