Fix potential hang in https handshake.

Fix potential hang in https handshake.

[szager]

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

Re: Fix potential hang in https handshake.

[Junio C Hamano]

szager@google.com writes:

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

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



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

Re: Fix potential hang in https handshake.

[Jeff King]

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

-Peff

Re: Fix potential hang in https handshake.

[Shawn Pearce]

On Fri, Oct 19, 2012 at 3:36 AM, Jeff King <peff@peff.net> wrote:
> 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...

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.

Re: Fix potential hang in https handshake.

[Junio C Hamano]

Stefan Zager <szager@google.com> writes:

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

Thanks.

Re: Fix potential hang in https handshake.

[Daniel Stenberg]

On Fri, 19 Oct 2012, Shawn Pearce 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.

Is this repeatable with a recent libcurl? It certainly sounds like a bug to 
me, and I might be interested in giving a try at tracking it down...

-- 

  / daniel.haxx.se

Re: Fix potential hang in https handshake.

[Jeff King]

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

Re: Fix potential hang in https handshake.

[Stefan Zager]

On Fri, Oct 19, 2012 at 1:27 PM, Jeff King <peff@peff.net> wrote:
>
> 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

I have no objection to this; any one else?

Stefan

Re: Fix potential hang in https handshake.

[Jeff King]

On Fri, Oct 19, 2012 at 01:37:06PM -0700, Stefan Zager wrote:

> > 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?

If you wouldn't mind, I was hoping you could flesh out the comment a bit
more with real details of when this happens (and/or put them in the
commit message). If this is indeed a bug to be worked around, it will be
a huge help to somebody reading this code in a year who can confirm that
modern curl does not need it anymore.

-Peff

Re: Fix potential hang in https handshake.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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:

Yeah, that sounds like a more targetted workaround (read: better).

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