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.
> > 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.
> > @@ -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
