[bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork

[Jelle Licht <jlicht@fsfe.org>]

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

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 #2: Type: text/html, Size: 5076 bytes --]