GNU bug report logs - #27553
[PATCH shepherd] Register SIGCHLD handler after primitive fork

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 27553 in the body.
You can then email your comments to 27553 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: Jelle Licht <jlicht <at> fsfe.org>
To: guix-patches <at> gnu.org
Subject: [PATCH shepherd] Register SIGCHLD handler after primitive fork
Date: Sun, 2 Jul 2017 03:11:01 +0200

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/html, inline)]

[0001-Register-SIGCHLD-handler-after-primitive-fork.patch (text/x-patch, attachment)]

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 27553 <at> debbugs.gnu.org
Subject: Re: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after
 primitive fork
Date: Wed, 12 Jul 2017 23:34:10 +0200

Hi Jelle,

Jelle Licht <jlicht <at> 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 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’.

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

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 27553 <at> debbugs.gnu.org
Subject: Re: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after
 primitive fork
Date: Fri, 14 Jul 2017 14:19:12 +0200

[Message part 1 (text/plain, inline)]

Hi Ludo,

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

> Hi Jelle,
>
> Jelle Licht <jlicht <at> 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]?


>
> > 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.


>
> 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

[1]: https://www.gnu.org/software/shepherd/
[2]: https://www.gnu.org/software/guix/about/#contact

[Message part 2 (text/html, inline)]

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 27553 <at> debbugs.gnu.org
Subject: Re: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after
 primitive fork
Date: Mon, 17 Jul 2017 10:33:03 +0200

Hi Jelle,

Jelle Licht <jlicht <at> fsfe.org> skribis:

> 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo <at> gnu.org>:
>
>> Hi Jelle,
>>
>> Jelle Licht <jlicht <at> 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’.

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

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 27553 <at> debbugs.gnu.org
Subject: Re: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after
 primitive fork
Date: Thu, 27 Jul 2017 16:32:03 +0200

[Message part 1 (text/plain, inline)]

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 <at> gnu.org>:

> Hi Jelle,
>
> Jelle Licht <jlicht <at> fsfe.org> skribis:
>
> > 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo <at> gnu.org>:
> >
> >> Hi Jelle,
> >>
> >> Jelle Licht <jlicht <at> 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’.
>

[Message part 2 (text/html, inline)]

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

From: Jelle Licht <jlicht <at> fsfe.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 27553 <at> debbugs.gnu.org
Subject: Re: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after
 primitive fork
Date: Thu, 7 Sep 2017 00:56:41 +0200

[Message part 1 (text/plain, inline)]

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 <at> fsfe.org>:

> 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 <at> gnu.org>:
>
>> Hi Jelle,
>>
>> Jelle Licht <jlicht <at> fsfe.org> skribis:
>>
>> > 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <ludo <at> gnu.org>:
>> >
>> >> Hi Jelle,
>> >>
>> >> Jelle Licht <jlicht <at> 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’.
>>
>
>

[Message part 2 (text/html, inline)]

[0001-Lazily-register-SIGCHLD-hander-on-first-call-to-fork.patch (text/x-patch, attachment)]

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: 27553-done <at> debbugs.gnu.org
Subject: Re: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after
 primitive fork
Date: Thu, 07 Sep 2017 16:49:36 +0200

Heya!

Jelle Licht <jlicht <at> fsfe.org> skribis:

> 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.

> From db942182224dfc0accad94897dd2122b128eef07 Mon Sep 17 00:00:00 2001
> From: Jelle Licht <jlicht <at> 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’.

Previous Next

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