From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58981) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dajqe-0000OZ-6g for guix-patches@gnu.org; Thu, 27 Jul 2017 10:33:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dajqY-0006pc-0E for guix-patches@gnu.org; Thu, 27 Jul 2017 10:33:08 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:56083) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dajqX-0006pG-UD for guix-patches@gnu.org; Thu, 27 Jul 2017 10:33:01 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dajqX-0004VH-LO for guix-patches@gnu.org; Thu, 27 Jul 2017 10:33:01 -0400 Subject: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork Resent-Message-ID: MIME-Version: 1.0 In-Reply-To: <87wp77ea4g.fsf@gnu.org> References: <87a849bar1.fsf@gnu.org> <87wp77ea4g.fsf@gnu.org> From: Jelle Licht Date: Thu, 27 Jul 2017 16:32:03 +0200 Message-ID: Content-Type: multipart/alternative; boundary="001a1143ea62ab712405554d6d14" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 27553@debbugs.gnu.org --001a1143ea62ab712405554d6d14 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=C3=A8s : > Hi Jelle, > > Jelle Licht skribis: > > > 2017-07-12 23:34 GMT+02:00 Ludovic Court=C3=A8s : > > > >> Hi Jelle, > >> > >> Jelle Licht skribis: > >> > >> > I am not sure if this is also the proper ML for the GNU Shepherd, bu= t > >> > 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=E2=80=99s the right list. :-) > >> > > I am glad it turned out to be :-). Perhaps [1] can be updated to the sa= me > > 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 registere= d > >> > /before/ shepherd has the chance to daemonize. I believe the followi= ng > >> > 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 missin= g > >> 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=E2=80=99ll never know that it has > terminated. > > >> Perhaps a solution would be to install the SIGCHLD handler lazily upon > >> the first =E2=80=98fork+exec-command=E2=80=99 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 t= o > > implement this though. > > I=E2=80=99d imagine something like a global variable (a Boolean) telling = whether > the SIGCHLD handler is installed, and then: > > (unless %sigchld-handler-installed? > (sigaction =E2=80=A6) > (set! %sigchld-handler-installed? #t)) > > Thoughts? > > Ludo=E2=80=99. > --001a1143ea62ab712405554d6d14 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ludo,

The documentation for the `daem= onize' action specifies the following:
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Go into the backg= round.=C2=A0 Be careful, this means that a new
process will be created, = so shepherd will not get SIGCHLD signals anymore
if previously spawned c= hilds terminate.=C2=A0 Therefore, this action should
usually only be use= d (if at all) *before* childs get spawned for which
we want to receive t= hese signals."


In a sense, the= problem that you describe can then be solved=C2=A0 by having the lazy SIGC= HLD 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 calli= ng the lambda stored in the `start' slot of any non-root-service servic= e

I know how to do the first one (the newly forked proces= s should lazily register the handler), but the second one seems a bit harde= r 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=C3=A8s <ludo@gnu.org>:
Hi Jelle,

Jelle Licht <jlicht@fsfe.org> = skribis:

> 2017-07-12 23:34 GMT+02:00 Ludovic Court=C3=A8s <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 Sheph= erd, 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 in= fo.
>>
>> It=E2=80=99s the right list.=C2=A0 :-)
>>
> I am glad it turned out to be :-). Perhaps [1] can be updated to the s= ame
> info as [2]?

Done!

>> > I recently starting playing around with user shepherd, and fo= und out that
>> > when running a shepherd 0.3.2 daemonized as non-init process = (via
>> "(action
>> > 'shepherd 'daemonize)"), zombie processes are cr= eated whenever you start
>> > and subsequently stop any service.
>> >
>> > Thinking I did something wrong, I asked lfam on #guix to shar= e his (very
>> > helpful) init.scm for user shepherd, yet I still noticed the = same
>> behaviour.
>> >
>> > I believe commit `efa2f45c5f7dc735407381b7b8a83d6c3= 7f828db'
>> inadvertently
>> > introduced an ordering issue, where the SIGCHLD handler is re= gistered
>> > /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 mi= ssing
>> 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=E2=80=99ll never know that it has terminated.

>> Perhaps a solution would be to install the SIGCHLD handler lazily = upon
>> the first =E2=80=98fork+exec-command=E2=80=99 call?=C2=A0 That wou= ld 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=E2=80=99d imagine something like a global variable (a Boolean) tel= ling whether
the SIGCHLD handler is installed, and then:

=C2=A0 (unless %sigchld-handler-installed?
=C2=A0 =C2=A0 (sigaction =E2=80=A6)
=C2=A0 =C2=A0 (set! %sigchld-handler-installed? #t))

Thoughts?

Ludo=E2=80=99.

--001a1143ea62ab712405554d6d14--