Web lists-archives.com

Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll




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