From: Jelle Licht <jlicht@fsfe.org>
To: "Ludovic Courtès" <ludo@gnu.org>
Cc: 27553@debbugs.gnu.org
Subject: [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork
Date: Thu, 7 Sep 2017 00:56:41 +0200 [thread overview]
Message-ID: <CAPsKtf+GHDagd9yLTYkSuYUts3tSvr8aE1m1XQ2xidZrQzQMVA@mail.gmail.com> (raw)
In-Reply-To: <CAPsKtfJoxtOQ0rTjT8MHNx=WwVO52+SfCB1iaGQ0LH55_FWAwA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 4285 bytes --]
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@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@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 #1.2: Type: text/html, Size: 5878 bytes --]
[-- Attachment #2: 0001-Lazily-register-SIGCHLD-hander-on-first-call-to-fork.patch --]
[-- Type: text/x-patch, Size: 1979 bytes --]
From db942182224dfc0accad94897dd2122b128eef07 Mon Sep 17 00:00:00 2001
From: Jelle Licht <jlicht@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.
---
modules/shepherd.scm | 3 ---
modules/shepherd/service.scm | 7 +++++++
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index f7c169d..7ea6493 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -142,9 +142,6 @@
;; Start the 'root' service.
(start root-service)
- ;; Install the SIGCHLD handler.
- (sigaction SIGCHLD respawn-service SA_NOCLDSTOP)
-
;; This _must_ succeed. (We could also put the `catch' around
;; `main', but it is often useful to get the backtrace, and
;; `caught-error' does not do this yet.)
diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index 72fbc3d..b2d8bc5 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -100,6 +100,9 @@
condition->sexp))
+;; Keep track of lazy initialization of SIGCHLD handler
+(define %sigchld-handler-installed? #f)
+
;; Type of service actions.
(define-record-type <action>
(make-action name proc doc)
@@ -787,6 +790,10 @@ false."
(default-environment-variables)))
"Spawn a process that executed COMMAND as per 'exec-command', and return
its PID."
+ ;; Install the SIGCHLD handler if this is the first fork+exec-command call
+ (unless %sigchld-handler-installed?
+ (sigaction SIGCHLD respawn-service SA_NOCLDSTOP)
+ (set! %sigchld-handler-installed? #t))
(let ((pid (primitive-fork)))
(if (zero? pid)
(exec-command command
--
2.14.1
next prev parent reply other threads:[~2017-09-06 22:57 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-02 1:11 [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork Jelle Licht
2017-07-12 21:34 ` Ludovic Courtès
2017-07-14 12:19 ` Jelle Licht
2017-07-17 8:33 ` Ludovic Courtès
2017-07-27 14:32 ` Jelle Licht
2017-09-06 22:56 ` Jelle Licht [this message]
2017-09-07 14:49 ` bug#27553: " Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPsKtf+GHDagd9yLTYkSuYUts3tSvr8aE1m1XQ2xidZrQzQMVA@mail.gmail.com \
--to=jlicht@fsfe.org \
--cc=27553@debbugs.gnu.org \
--cc=ludo@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.