Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll
- From
Kim Gybels <kgybels@infogroep.be>
- Date
- Apr 15, 2018, 17:08 UTC
- Message-ID
- <20180415170859.GA30197@infogroep.be>
- In-Reply-To
- <nycvar.QRO.7.76.6.1804131433250.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
On (13/04/18 14:36), Johannes Schindelin wrote:
> > The poll provided in compat/poll.c is not interrupted by receiving > > SIGCHLD. Use a timeout for cleaning up dead children in a timely manner. > > Maybe say "When using this poll emulation, use a timeout ..."?
I will rewrite the commit message when I reroll the patch. Calling the poll "uninterruptible" might be wrong as well, although the poll doesn't return with EINTR when a child process terminates, it might still be interruptible in other ways. On a related note, the handler for SIGCHLD is simply not called in Git-for-Windows' daemon.
Show 13 quoted lines
> > diff --git a/daemon.c b/daemon.c
> > index fe833ea7de..6dc95c1b2f 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1147,6 +1147,7 @@ static int service_loop(struct socketlist *socklist)
> > {
> > struct pollfd *pfd;
> > int i;
> > + int poll_timeout = -1;
>
> Just reuse the line above:
>
> int poll_timeout = -1, i;Sure.
Show 21 quoted lines
> > @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist *socklist)
> > int i;
> >
> > check_dead_children();
> > -
> > - if (poll(pfd, socklist->nr, -1) < 0) {
> > +#ifdef NO_POLL
> > + poll_timeout = live_children ? 100 : -1;
> > +#endif
> > + int ret = poll(pfd, socklist->nr, poll_timeout);
> > + if (ret == 0) {
> > + continue;
> > + } else if (ret < 0) {
>
> I would find it a bit easier on the eyes if this did not use curlies, and
> dropped the unnecessary `else` (`continue` will take care of that):
>
> if (!ret)
> continue;
> if (ret < 0)
> [...]Funny, that's how I would normally write it, if I wasn't so focused on trying to follow the coding quidelines. While I'm at it, I will also fix that sneaky double space after the if.
Is it ok to add the timeout for all platforms using the poll emulation, since I only tested for Windows?
Best regards, Kim