* [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork @ 2017-07-02 1:11 Jelle Licht 2017-07-12 21:34 ` Ludovic Courtès 0 siblings, 1 reply; 7+ messages in thread From: Jelle Licht @ 2017-07-02 1:11 UTC (permalink / raw) To: 27553 [-- Attachment #1.1: Type: text/plain, Size: 897 bytes --] Hi all, 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. 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. Regards, Jelle [-- Attachment #1.2: Type: text/html, Size: 1104 bytes --] [-- Attachment #2: 0001-Register-SIGCHLD-handler-after-primitive-fork.patch --] [-- Type: text/x-patch, Size: 1294 bytes --] From 2603a1f4201668c13f992dbde3dd46be4dc2c31d Mon Sep 17 00:00:00 2001 From: Jelle Licht <jlicht@fsfe.org> Date: Sun, 2 Jul 2017 02:58:36 +0200 Subject: [PATCH] Register SIGCHLD handler after primitive-fork. * modules/shepherd.scm (main): Move call to 'sigaction' so daemonized process can handle SIGCHLD signal. --- modules/shepherd.scm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/shepherd.scm b/modules/shepherd.scm index f7c169d..dfae7d6 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.) @@ -164,6 +161,9 @@ (apply format #f (gettext (cadr args)) (caddr args)) (quit 1)))) + ;; Install the SIGCHLD handler. + (sigaction SIGCHLD respawn-service SA_NOCLDSTOP) + (when (provided? 'threads) ;; XXX: This terrible hack allows us to make sure that signal handlers ;; get a chance to run in a timely fashion. Without it, after an EINTR, -- 2.13.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork 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 0 siblings, 1 reply; 7+ messages in thread From: Ludovic Courtès @ 2017-07-12 21:34 UTC (permalink / raw) To: Jelle Licht; +Cc: 27553 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 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.) 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? Ludo’. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork 2017-07-12 21:34 ` Ludovic Courtès @ 2017-07-14 12:19 ` Jelle Licht 2017-07-17 8:33 ` Ludovic Courtès 0 siblings, 1 reply; 7+ messages in thread From: Jelle Licht @ 2017-07-14 12:19 UTC (permalink / raw) To: Ludovic Courtès; +Cc: 27553 [-- Attachment #1: Type: text/plain, Size: 2108 bytes --] Hi Ludo, 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]? > > > 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. > > 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. Regards, Jelle [1]: https://www.gnu.org/software/shepherd/ [2]: https://www.gnu.org/software/guix/about/#contact [-- Attachment #2: Type: text/html, Size: 3194 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork 2017-07-14 12:19 ` Jelle Licht @ 2017-07-17 8:33 ` Ludovic Courtès 2017-07-27 14:32 ` Jelle Licht 0 siblings, 1 reply; 7+ messages in thread From: Ludovic Courtès @ 2017-07-17 8:33 UTC (permalink / raw) To: Jelle Licht; +Cc: 27553 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’. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork 2017-07-17 8:33 ` Ludovic Courtès @ 2017-07-27 14:32 ` Jelle Licht 2017-09-06 22:56 ` Jelle Licht 0 siblings, 1 reply; 7+ messages in thread From: Jelle Licht @ 2017-07-27 14:32 UTC (permalink / raw) To: Ludovic Courtès; +Cc: 27553 [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug#27553] [PATCH shepherd] Register SIGCHLD handler after primitive fork 2017-07-27 14:32 ` Jelle Licht @ 2017-09-06 22:56 ` Jelle Licht 2017-09-07 14:49 ` bug#27553: " Ludovic Courtès 0 siblings, 1 reply; 7+ messages in thread From: Jelle Licht @ 2017-09-06 22:56 UTC (permalink / raw) To: Ludovic Courtès; +Cc: 27553 [-- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* bug#27553: [PATCH shepherd] Register SIGCHLD handler after primitive fork 2017-09-06 22:56 ` Jelle Licht @ 2017-09-07 14:49 ` Ludovic Courtès 0 siblings, 0 replies; 7+ messages in thread From: Ludovic Courtès @ 2017-09-07 14:49 UTC (permalink / raw) To: Jelle Licht; +Cc: 27553-done Heya! Jelle Licht <jlicht@fsfe.org> skribis: > 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 :-). Indeed, thanks for the reminder. > 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. LGTM, applied! Ludo’. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-07 14:50 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2017-09-07 14:49 ` bug#27553: " Ludovic Courtès
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/guix.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).