Re: Fix potential hang in https handshake.

From: Jeff King <peff@peff.net>
Date: Oct 19, 2012, 10:36 UTC
Message-ID: <20121019103627.GA29366@sigill.intra.peff.net>
In-Reply-To: <7vd30fl736.fsf@alter.siamese.dyndns.org>

On Thu, Oct 18, 2012 at 03:59:41PM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> > It will sometimes happen that curl_multi_fdset() doesn't
> > return any file descriptors.  In that case, it's recommended
> > that the application sleep for a short time before running
> > curl_multi_perform() again.
> >
> > http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
> >
> > Signed-off-by: Stefan Zager <szager@google.com>
> > ---
> 
> Thanks.  Would it be a better idea to "patch up" in problematic
> case, instead of making this logic too deeply nested, like this
> instead, I have to wonder...
> 
> 
> 	... all the existing code above unchanged ...
> 	curl_multi_fdset(..., &max_fd);
> +	if (max_fd < 0) {    
> +		/* nothing actionable??? */
> +		select_timeout.tv_sec = 0;
> +		select_timeout.tv_usec = 50000;
> +	}
> 
> 	select(max_fd+1, ..., &select_timeout);

But wouldn't that override a potentially shorter timeout that curl gave us via curl_multi_timeout, making us unnecessarily slow to hand control back to curl?

The current logic is:

  - if curl says there is something to do now (timeout == 0), do it
    immediately

  - if curl gives us a timeout, use it with select

  - otherwise, feed 50ms to selection

It should not matter what we get from curl_multi_fdset. If there are fds, great, we will feed them to select with the timeout, and we may break out early if there is work to do. If not, then we are already doing this wait.

IOW, it seems like we are _already_ following the advice referenced in curl's manpage. Is there some case I am missing? Confused...

-Peff