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

[Jelle Licht]

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 <address@hidden>:

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 <address@hidden>:

Hi Jelle,

Jelle Licht <address@hidden> skribis:

> 2017-07-12 23:34 GMT+02:00 Ludovic Courtès <address@hidden>:
>
>> Hi Jelle,
>>
>> Jelle Licht <address@hidden> 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’.

0001-Lazily-register-SIGCHLD-hander-on-first-call-to-fork.patch
Description: Text Data