Fix potential hang in https handshake.

> From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001
> From: Stefan Zager <szager@google.com>
> Date: Thu, 18 Oct 2012 14:03:59 -0700
> Subject: [PATCH] Fix potential hang in https handshake.
>
> 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>
> ---

>  http.c |   40 ++++++++++++++++++++++++++--------------
>  1 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/http.c b/http.c
> index df9bb71..a6f66c0 100644
> --- a/http.c
> +++ b/http.c
> @@ -602,35 +602,47 @@ void run_active_slot(struct active_request_slot *slot)
>  	int max_fd;
>  	struct timeval select_timeout;
>  	int finished = 0;
> +	long curl_timeout;
>  
>  	slot->finished = &finished;
>  	while (!finished) {
>  		step_active_slots();
>  
>  		if (slot->in_use) {
> +			max_fd = -1;
> +			FD_ZERO(&readfds);
> +			FD_ZERO(&writefds);
> +			FD_ZERO(&excfds);
> +			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> +
>  #if LIBCURL_VERSION_NUM >= 0x070f04
> -			long curl_timeout;
> -			curl_multi_timeout(curlm, &curl_timeout);
> -			if (curl_timeout == 0) {
> -				continue;
> -			} else if (curl_timeout == -1) {
> +			/* 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
> +			*/
> +			if (max_fd == -1) {
>  				select_timeout.tv_sec  = 0;
>  				select_timeout.tv_usec = 50000;
>  			} else {
> -				select_timeout.tv_sec  =  curl_timeout / 1000;
> -				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
> +				curl_timeout = 0;
> +				curl_multi_timeout(curlm, &curl_timeout);
> +				if (curl_timeout == 0) {
> +					continue;
> +				} else if (curl_timeout == -1) {
> +					select_timeout.tv_sec  = 0;
> +					select_timeout.tv_usec = 50000;
> +				} else {
> +					select_timeout.tv_sec  =  curl_timeout / 1000;
> +					select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
> +				}
>  			}
>  #else
>  			select_timeout.tv_sec  = 0;
>  			select_timeout.tv_usec = 50000;
>  #endif
> -
> -			max_fd = -1;
> -			FD_ZERO(&readfds);
> -			FD_ZERO(&writefds);
> -			FD_ZERO(&excfds);
> -			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> -
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}

> > 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);

> On Thu, Oct 18, 2012 at 03:59:41PM -0700, Junio C Hamano wrote:
>
>> > 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...

> > IOW, it seems like we are _already_ following the advice referenced in
> > curl's manpage. Is there some case I am missing? Confused...
> 
> The issue with the current code is sometimes when libcurl is opening a
> CONNECT style connection through an HTTP proxy it returns a crazy high
> timeout (>240 seconds) and no fds. In this case Git waits forever.
> Stefan observed that using a timeout of 50 ms in this situation to
> poll libcurl is better, as it figures out a lot more quickly that it
> is connected to the proxy and can issue the request.

>
> On Fri, Oct 19, 2012 at 07:10:46AM -0700, Shawn O. Pearce wrote:
>
> > > IOW, it seems like we are _already_ following the advice referenced in
> > > curl's manpage. Is there some case I am missing? Confused...
> >
> > The issue with the current code is sometimes when libcurl is opening a
> > CONNECT style connection through an HTTP proxy it returns a crazy high
> > timeout (>240 seconds) and no fds. In this case Git waits forever.
> > Stefan observed that using a timeout of 50 ms in this situation to
> > poll libcurl is better, as it figures out a lot more quickly that it
> > is connected to the proxy and can issue the request.
>
> Ah. That sounds like a bug in curl to me. But either way, if we want to
> work around it, wouldn't the right thing be to override curl's timeout
> in that instance? Like:
>
> diff --git a/http.c b/http.c
> index df9bb71..cd07cdf 100644
> --- a/http.c
> +++ b/http.c
> @@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)
>                         FD_ZERO(&excfds);
>                         curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
>
> +                       /*
> +                        * Sometimes curl will give a really long timeout for a
> +                        * CONNECT when there are no fds to read, but we can
> +                        * get better results by running curl_multi_perform
> +                        * more frequently.
> +                        */
> +                       if (maxfd < 0 &&
> +                           (select_timeout.tv_sec > 0 ||
> +                            select_timeout.tv_usec > 50000)) {
> +                               select_timeout.tv_sec = 0;
> +                               select_timeout.tv_usec = 50000;
> +                       }
> +
>                         select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>                 }
>         }
>
> -Peff

> > diff --git a/http.c b/http.c
> > index df9bb71..cd07cdf 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)
> >                         FD_ZERO(&excfds);
> >                         curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> >
> > +                       /*
> > +                        * Sometimes curl will give a really long timeout for a
> > +                        * CONNECT when there are no fds to read, but we can
> > +                        * get better results by running curl_multi_perform
> > +                        * more frequently.
> > +                        */
> > +                       if (maxfd < 0 &&
> > +                           (select_timeout.tv_sec > 0 ||
> > +                            select_timeout.tv_usec > 50000)) {
> > +                               select_timeout.tv_sec = 0;
> > +                               select_timeout.tv_usec = 50000;
> > +                       }
> > +
> >                         select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
> >                 }
> >         }
> >
> I have no objection to this; any one else?

> On Fri, Oct 19, 2012 at 07:10:46AM -0700, Shawn O. Pearce wrote:
>
>> > IOW, it seems like we are _already_ following the advice referenced in
>> > curl's manpage. Is there some case I am missing? Confused...
>> 
>> The issue with the current code is sometimes when libcurl is opening a
>> CONNECT style connection through an HTTP proxy it returns a crazy high
>> timeout (>240 seconds) and no fds. In this case Git waits forever.
>> Stefan observed that using a timeout of 50 ms in this situation to
>> poll libcurl is better, as it figures out a lot more quickly that it
>> is connected to the proxy and can issue the request.
>
> Ah. That sounds like a bug in curl to me. But either way, if we want to
> work around it, wouldn't the right thing be to override curl's timeout
> in that instance? Like:

>
> diff --git a/http.c b/http.c
> index df9bb71..cd07cdf 100644
> --- a/http.c
> +++ b/http.c
> @@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)
>  			FD_ZERO(&excfds);
>  			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
>  
> +			/*
> +			 * Sometimes curl will give a really long timeout for a
> +			 * CONNECT when there are no fds to read, but we can
> +			 * get better results by running curl_multi_perform
> +			 * more frequently.
> +			 */
> +			if (maxfd < 0 &&
> +			    (select_timeout.tv_sec > 0 ||
> +			     select_timeout.tv_usec > 50000)) {
> +				select_timeout.tv_sec = 0;
> +				select_timeout.tv_usec = 50000;
> +			}
> +
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}
>
> -Peff

> On Oct 19, 2012 7:11 AM, "Shawn Pearce" <spearce@spearce.org> wrote:
>>
>> The issue with the current code is sometimes when libcurl is opening a
>> CONNECT style connection through an HTTP proxy it returns a crazy high
>> timeout (>240 seconds) and no fds. In this case Git waits forever.
>> Stefan observed that using a timeout of 50 ms in this situation to
>> poll libcurl is better, as it figures out a lot more quickly that it
>> is connected to the proxy and can issue the request.
>
> Correct.  Anecdotally, the zero-file-descriptor situation happens only once
> per process invocation, so the risk of passing a too-long timeout to
> select() is small.