Re: Fix potential hang in https handshake.
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 sza...@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 = 5;
+ }
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
On Fri, Oct 19, 2012 at 3:36 AM, Jeff King p...@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 sza...@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 = 5;
+ }
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
Stefan Zager sza...@google.com writes: On Oct 19, 2012 7:11 AM, Shawn Pearce spea...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
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 5)) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
+
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
}
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
On Fri, Oct 19, 2012 at 1:27 PM, Jeff King p...@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 5)) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
+
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
}
-Peff
I have no objection to this; any one else?
Stefan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
Jeff King p...@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 5)) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
+
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
}
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fix potential hang in https handshake (v3)
From 32e06128dbc97ceb0d060c88ec8db204fa51be5c Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@google.com
Date: Thu, 18 Oct 2012 16:23:53 -0700
Subject: [PATCH] Fix potential hang in https handshake.
It has been observed that curl_multi_timeout may return a very long
timeout value (e.g., 294 seconds and some usec) just before
curl_multi_fdset returns no file descriptors for reading. The
upshot is that select() will hang for a long time -- long enough for
an https handshake to be dropped. The observed behavior is that
the git command will hang at the terminal and never transfer any
data.
This patch is a workaround for a probable bug in libcurl. The bug
only seems to manifest around a very specific set of circumstances:
- curl version (from curl/curlver.h):
#define LIBCURL_VERSION_NUM 0x071307
- git-remote-https running on an ubuntu-lucid VM.
- Connecting through squid proxy running on another VM.
Interestingly, the problem doesn't manifest if a host connects
through squid proxy running on localhost; only if the proxy is on
a separate VM (not sure if the squid host needs to be on a separate
physical machine). That would seem to suggest that this issue
is timing-sensitive.
This patch is more or less in line with a recommendation in the
curl docs about how to behave when curl_multi_fdset doesn't return
and file descriptors:
http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
Signed-off-by: Stefan Zager sza...@google.com
---
http.c | 11 +++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/http.c b/http.c
index df9bb71..51eef02 100644
--- a/http.c
+++ b/http.c
@@ -631,6 +631,17 @@ void run_active_slot(struct active_request_slot *slot)
FD_ZERO(excfds);
curl_multi_fdset(curlm, readfds, writefds, excfds,
max_fd);
+ /* It can happen that curl_multi_timeout returns a
pathologically
+* long timeout when curl_multi_fdset returns no file
descriptors
+* to read. See commit message for more details.
+*/
+ if (max_fd 0
+ select_timeout.tv_sec 0 ||
+ select_timeout.tv_usec 5) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
+
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
}
--
1.7.7.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake (v3)
On Fri, Oct 19, 2012 at 02:04:20PM -0700, sza...@google.com wrote:
From 32e06128dbc97ceb0d060c88ec8db204fa51be5c Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@google.com
Date: Thu, 18 Oct 2012 16:23:53 -0700
Drop these lines.
Subject: [PATCH] Fix potential hang in https handshake.
And make this one your actual email subject.
It has been observed that curl_multi_timeout may return a very long
timeout value (e.g., 294 seconds and some usec) just before
curl_multi_fdset returns no file descriptors for reading. The
upshot is that select() will hang for a long time -- long enough for
an https handshake to be dropped. The observed behavior is that
the git command will hang at the terminal and never transfer any
data.
This patch is a workaround for a probable bug in libcurl. The bug
only seems to manifest around a very specific set of circumstances:
- curl version (from curl/curlver.h):
#define LIBCURL_VERSION_NUM 0x071307
- git-remote-https running on an ubuntu-lucid VM.
- Connecting through squid proxy running on another VM.
Interestingly, the problem doesn't manifest if a host connects
through squid proxy running on localhost; only if the proxy is on
a separate VM (not sure if the squid host needs to be on a separate
physical machine). That would seem to suggest that this issue
is timing-sensitive.
Thanks, that explanation makes much more sense.
diff --git a/http.c b/http.c
index df9bb71..51eef02 100644
--- a/http.c
+++ b/http.c
@@ -631,6 +631,17 @@ void run_active_slot(struct active_request_slot *slot)
FD_ZERO(excfds);
curl_multi_fdset(curlm, readfds, writefds, excfds,
max_fd);
+ /* It can happen that curl_multi_timeout returns a
pathologically
+ * long timeout when curl_multi_fdset returns no file
descriptors
+ * to read. See commit message for more details.
+ */
Minor nit, but our multi-line comment style is:
/*
* blah blah blah
*/
+ if (max_fd 0
+ select_timeout.tv_sec 0 ||
+ select_timeout.tv_usec 5) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
Should there be parentheses separating the || bit from the ?
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake (v3)
Jeff King p...@peff.net writes:
+if (max_fd 0
+select_timeout.tv_sec 0 ||
+select_timeout.tv_usec 5) {
+select_timeout.tv_sec = 0;
+select_timeout.tv_usec = 5;
+}
Should there be parentheses separating the || bit from the ?
Yeah, I think there should be. Thanks for sharp eyes.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix potential hang in https handshake.
It has been observed that curl_multi_timeout may return a very long
timeout value (e.g., 294 seconds and some usec) just before
curl_multi_fdset returns no file descriptors for reading. The
upshot is that select() will hang for a long time -- long enough for
an https handshake to be dropped. The observed behavior is that
the git command will hang at the terminal and never transfer any
data.
This patch is a workaround for a probable bug in libcurl. The bug
only seems to manifest around a very specific set of circumstances:
- curl version (from curl/curlver.h):
#define LIBCURL_VERSION_NUM 0x071307
- git-remote-https running on an ubuntu-lucid VM.
- Connecting through squid proxy running on another VM.
Interestingly, the problem doesn't manifest if a host connects
through squid proxy running on localhost; only if the proxy is on
a separate VM (not sure if the squid host needs to be on a separate
physical machine). That would seem to suggest that this issue
is timing-sensitive.
This patch is more or less in line with a recommendation in the
curl docs about how to behave when curl_multi_fdset doesn't return
and file descriptors:
http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
Signed-off-by: Stefan Zager sza...@google.com
---
http.c | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/http.c b/http.c
index df9bb71..b7e7ab4 100644
--- a/http.c
+++ b/http.c
@@ -631,6 +631,18 @@ void run_active_slot(struct active_request_slot *slot)
FD_ZERO(excfds);
curl_multi_fdset(curlm, readfds, writefds, excfds,
max_fd);
+ /*
+* It can happen that curl_multi_timeout returns a
pathologically
+* long timeout when curl_multi_fdset returns no file
descriptors
+* to read. See commit message for more details.
+*/
+ if (max_fd 0
+ (select_timeout.tv_sec 0 ||
+ select_timeout.tv_usec 5)) {
+ select_timeout.tv_sec = 0;
+select_timeout.tv_usec = 5;
+ }
+
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
}
--
1.7.7.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix potential hang in https handshake.
Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fix potential hang in https handshake.
From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@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 sza...@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 = 5;
} 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 = 5;
+ } 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 = 5;
#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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake.
sza...@google.com writes:
From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@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 sza...@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 = 5;
+ }
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 = 5;
} 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 = 5;
+ } 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 = 5;
#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);
}
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fix potential hang in https handshake (v2).
From aa77ab3dd5b98a5786ac158528f45355fc0ddbc3 Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@google.com
Date: Thu, 18 Oct 2012 16:23:53 -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 sza...@google.com
---
http.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/http.c b/http.c
index df9bb71..e8aba7f 100644
--- a/http.c
+++ b/http.c
@@ -630,6 +630,10 @@ void run_active_slot(struct active_request_slot *slot)
FD_ZERO(writefds);
FD_ZERO(excfds);
curl_multi_fdset(curlm, readfds, writefds, excfds,
max_fd);
+ if (max_fd 0) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
--
1.7.7.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake (v2).
sza...@google.com writes:
From aa77ab3dd5b98a5786ac158528f45355fc0ddbc3 Mon Sep 17 00:00:00 2001
From: Stefan Zager sza...@google.com
Date: Thu, 18 Oct 2012 16:23:53 -0700
Subject: [PATCH] Fix potential hang in https handshake.
Please don't do the above (as usual ;-)
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 sza...@google.com
---
The above sounds like the code is doing something against a
recommended practice, but is there a user-visible symptom due to
this?
We end up calling select() without any bit set in fds, so it would
become micro-sleep of select_timeout in such a case, but as far as I
can see, the existing code either
* does not select() and keeps polling step_active_slots() without
delay, when curl_timeout gives a 0 return value; or
* sets 50ms timeout or whatever negative value derived from
curl_timeout when the returned value is not 0 nor -1.
Is the symptom that select(), when given a negative timeout and no
fd to wake it, sleeps forever (or lng time, taking that negative
value as if it is a large unsigned long) or something?
Thanks.
http.c |4
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/http.c b/http.c
index df9bb71..e8aba7f 100644
--- a/http.c
+++ b/http.c
@@ -630,6 +630,10 @@ void run_active_slot(struct active_request_slot *slot)
FD_ZERO(writefds);
FD_ZERO(excfds);
curl_multi_fdset(curlm, readfds, writefds, excfds,
max_fd);
+ if (max_fd 0) {
+ select_timeout.tv_sec = 0;
+ select_timeout.tv_usec = 5;
+ }
select(max_fd+1, readfds, writefds, excfds,
select_timeout);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fix potential hang in https handshake (v2).
Junio C Hamano gits...@pobox.com writes: We end up calling select() without any bit set in fds, so it would become micro-sleep of select_timeout in such a case, but as far as I can see, the existing code either * does not select() and keeps polling step_active_slots() without delay, when curl_timeout gives a 0 return value; or * sets 50ms timeout or whatever negative value derived from curl_timeout when the returned value is not 0 nor -1. Is the symptom that select(), when given a negative timeout and no fd to wake it, sleeps forever (or lng time, taking that negative value as if it is a large unsigned long) or something? Addendum. What I am trying to get at are (1) three line description I can put in the release notes for this fix when it is merged to the maintenance track, and (2) a feel of how often this happens and how bad the damage is. The latter helps us judge if this do the normal thing, but if in a rare case where we do not find any fds, patch it up to proceed is a better approach over your original. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
