From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59981) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dpjFs-0001H4-2s for guix-patches@gnu.org; Wed, 06 Sep 2017 18:57:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dpjFm-0001nq-Dz for guix-patches@gnu.org; Wed, 06 Sep 2017 18:57:08 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:44907) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dpjFm-0001nl-9s for guix-patches@gnu.org; Wed, 06 Sep 2017 18:57:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dpjFl-0006jP-U8 for guix-patches@gnu.org; Wed, 06 Sep 2017 18:57:01 -0400 Subject: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork Resent-Message-ID: MIME-Version: 1.0 In-Reply-To: References: <87a849bar1.fsf@gnu.org> <87wp77ea4g.fsf@gnu.org> From: Jelle Licht Date: Thu, 7 Sep 2017 00:56:41 +0200 Message-ID: Content-Type: multipart/mixed; boundary="001a114dba3ee0e75405588d416f" 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 --001a114dba3ee0e75405588d416f Content-Type: multipart/alternative; boundary="001a114dba3ee0e74f05588d416d" --001a114dba3ee0e74f05588d416d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 : > 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 anymor= e >> 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 documentatio= n > 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 see= ms > 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, b= ut >> >> > 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 >> same >> > info as [2]? >> >> Done! >> >> >> > I recently starting playing around with user shepherd, and found ou= t >> 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 register= ed >> >> > /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 missi= ng >> >> 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 upo= n >> >> the first =E2=80=98fork+exec-command=E2=80=99 call? That would ensur= e 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) 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. >> > > --001a114dba3ee0e74f05588d416d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I just tested what Ludo' proposed, and it seems to wor= k like a charm.
Seeing as we might be seeing more non-init shepherd ins= tances w.r.t.
=C2=A0user services and the possible service extens= ion 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.or= g>:
H= i Ludo,

The documentation for the `daemonize' action speci= fies the following:
= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "Go into the background.=C2=A0 Be caref= ul, this means that a new
process will be created, so shepherd will not = get SIGCHLD signals anymore
if previously spawned childs terminate.=C2= =A0 Therefore, this action should
usually only be used (if at all) *befo= re* childs get spawned for which
we want to receive these signals."=


In a sense, the problem that you d= escribe can then be solved=C2=A0 by having the lazy SIGCHLD handler be regi= stered in two places:
- Immediately after a call to the `daem= onize' action, as its documentation that if called, it should be done b= efore starting any services
- Before calling the lambda store= d in the `start' slot of any non-root-service service

I know how to do the first one (the newly forked process should lazily reg= ister the handler), but the second one seems a bit harder to do.
<= div>I could add a special case to the `start' method so that it will la= zily 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 &l= t;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 `efa2f45c5f7dc735407381b7b8a83d6c37f828= db'
>> 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.


--001a114dba3ee0e74f05588d416d-- --001a114dba3ee0e75405588d416f Content-Type: text/x-patch; charset="US-ASCII"; name="0001-Lazily-register-SIGCHLD-hander-on-first-call-to-fork.patch" Content-Disposition: attachment; filename="0001-Lazily-register-SIGCHLD-hander-on-first-call-to-fork.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_j79mfbz50 RnJvbSBkYjk0MjE4MjIyNGRmYzBhY2NhZDk0ODk3ZGQyMTIyYjEyOGVlZjA3IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKZWxsZSBMaWNodCA8amxpY2h0QGZzZmUub3JnPgpEYXRlOiBU aHUsIDcgU2VwIDIwMTcgMDA6NTI6NDkgKzAyMDAKU3ViamVjdDogW1BBVENIXSBMYXppbHkgcmVn aXN0ZXIgU0lHQ0hMRCBoYW5kZXIgb24gZmlyc3QgY2FsbCB0bwogJ2ZvcmsrZXhlYy1jb21tYW5k Jy4KCiogbW9kdWxlcy9zaGVwaGVyZC5zY20gKG1haW4pOiBNb3ZlIHVuY29uZGl0aW9uYWwgdG9w LWxldmVsIGNhbGwgdG8gJ3NpZ2FjdGlvbicgdG8uLi4KKiBtb2R1bGVzL3NoZXBoZXJkL3NlcnZp Y2Uuc2NtIChmb3JrK2V4ZWMtY29tbWFuZCk6IGhlcmUuIFVzZSBuZXcgdmFyaWFibGUuCiglc2ln Y2hsZC1oYW5kbGVyLWluc3RhbGxlZD8pOiBOZXcgdmFyaWFibGUuCi0tLQogbW9kdWxlcy9zaGVw aGVyZC5zY20gICAgICAgICB8IDMgLS0tCiBtb2R1bGVzL3NoZXBoZXJkL3NlcnZpY2Uuc2NtIHwg NyArKysrKysrCiAyIGZpbGVzIGNoYW5nZWQsIDcgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlvbnMo LSkKCmRpZmYgLS1naXQgYS9tb2R1bGVzL3NoZXBoZXJkLnNjbSBiL21vZHVsZXMvc2hlcGhlcmQu c2NtCmluZGV4IGY3YzE2OWQuLjdlYTY0OTMgMTAwNjQ0Ci0tLSBhL21vZHVsZXMvc2hlcGhlcmQu c2NtCisrKyBiL21vZHVsZXMvc2hlcGhlcmQuc2NtCkBAIC0xNDIsOSArMTQyLDYgQEAKICAgICA7 OyBTdGFydCB0aGUgJ3Jvb3QnIHNlcnZpY2UuCiAgICAgKHN0YXJ0IHJvb3Qtc2VydmljZSkKIAot ICAgIDs7IEluc3RhbGwgdGhlIFNJR0NITEQgaGFuZGxlci4KLSAgICAoc2lnYWN0aW9uIFNJR0NI TEQgcmVzcGF3bi1zZXJ2aWNlIFNBX05PQ0xEU1RPUCkKLQogICAgIDs7IFRoaXMgX211c3RfIHN1 Y2NlZWQuICAoV2UgY291bGQgYWxzbyBwdXQgdGhlIGBjYXRjaCcgYXJvdW5kCiAgICAgOzsgYG1h aW4nLCBidXQgaXQgaXMgb2Z0ZW4gdXNlZnVsIHRvIGdldCB0aGUgYmFja3RyYWNlLCBhbmQKICAg ICA7OyBgY2F1Z2h0LWVycm9yJyBkb2VzIG5vdCBkbyB0aGlzIHlldC4pCmRpZmYgLS1naXQgYS9t b2R1bGVzL3NoZXBoZXJkL3NlcnZpY2Uuc2NtIGIvbW9kdWxlcy9zaGVwaGVyZC9zZXJ2aWNlLnNj bQppbmRleCA3MmZiYzNkLi5iMmQ4YmM1IDEwMDY0NAotLS0gYS9tb2R1bGVzL3NoZXBoZXJkL3Nl cnZpY2Uuc2NtCisrKyBiL21vZHVsZXMvc2hlcGhlcmQvc2VydmljZS5zY20KQEAgLTEwMCw2ICsx MDAsOSBAQAogCiAgICAgICAgICAgICBjb25kaXRpb24tPnNleHApKQogCis7OyBLZWVwIHRyYWNr IG9mIGxhenkgaW5pdGlhbGl6YXRpb24gb2YgU0lHQ0hMRCBoYW5kbGVyCisoZGVmaW5lICVzaWdj aGxkLWhhbmRsZXItaW5zdGFsbGVkPyAjZikKKwogOzsgVHlwZSBvZiBzZXJ2aWNlIGFjdGlvbnMu CiAoZGVmaW5lLXJlY29yZC10eXBlIDxhY3Rpb24+CiAgIChtYWtlLWFjdGlvbiBuYW1lIHByb2Mg ZG9jKQpAQCAtNzg3LDYgKzc5MCwxMCBAQCBmYWxzZS4iCiAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgIChkZWZhdWx0LWVudmlyb25tZW50LXZhcmlhYmxlcykpKQogICAiU3Bhd24gYSBwcm9j ZXNzIHRoYXQgZXhlY3V0ZWQgQ09NTUFORCBhcyBwZXIgJ2V4ZWMtY29tbWFuZCcsIGFuZCByZXR1 cm4KIGl0cyBQSUQuIgorICA7OyBJbnN0YWxsIHRoZSBTSUdDSExEIGhhbmRsZXIgaWYgdGhpcyBp cyB0aGUgZmlyc3QgZm9yaytleGVjLWNvbW1hbmQgY2FsbAorICAodW5sZXNzICVzaWdjaGxkLWhh bmRsZXItaW5zdGFsbGVkPworICAgIChzaWdhY3Rpb24gU0lHQ0hMRCByZXNwYXduLXNlcnZpY2Ug U0FfTk9DTERTVE9QKQorICAgIChzZXQhICVzaWdjaGxkLWhhbmRsZXItaW5zdGFsbGVkPyAjdCkp CiAgIChsZXQgKChwaWQgKHByaW1pdGl2ZS1mb3JrKSkpCiAgICAgKGlmICh6ZXJvPyBwaWQpCiAg ICAgICAgIChleGVjLWNvbW1hbmQgY29tbWFuZAotLSAKMi4xNC4xCgo= --001a114dba3ee0e75405588d416f--