{"thread":{"id":"31860","subject":"Fix potential hang in https handshake.","startedAt":"2012-10-18T21:35:26Z","lastAt":"2012-10-19T20:40:46Z","messageCount":10,"participants":["szager@google.com","Junio C Hamano","Jeff King","Shawn Pearce","Daniel Stenberg","Stefan Zager"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"201530","messageId":"5080761e.IEDySUuQzlCwL6fM%szager@google.com","threadId":"31860","inReplyTo":null,"subject":"Fix potential hang in https handshake.","fromName":"","fromEmail":"szager@google.com","sentAt":"2012-10-18T21:35:26Z","receivedAt":"2012-10-18T21:35:26Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":">From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001\nFrom: Stefan Zager \nDate: Thu, 18 Oct 2012 14:03:59 -0700\nSubject: [PATCH] Fix potential hang in https handshake.\n\nIt will sometimes happen that curl_multi_fdset() doesn't\nreturn any file descriptors. In that case, it's recommended\nthat the application sleep for a short time before running\ncurl_multi_perform() again.\n\nhttp://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n\nSigned-off-by: Stefan Zager \n---\n http.c | 40 ++++++++++++++++++++++++++--------------\n 1 files changed, 26 insertions(+), 14 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex df9bb71..a6f66c0 100644\n--- a/http.c\n+++ b/http.c\n@@ -602,35 +602,47 @@ void run_active_slot(struct active_request_slot *slot)\n \tint max_fd;\n \tstruct timeval select_timeout;\n \tint finished = 0;\n+\tlong curl_timeout;\n \n \tslot->finished = &finished;\n \twhile (!finished) {\n \t\tstep_active_slots();\n \n \t\tif (slot->in_use) {\n+\t\t\tmax_fd = -1;\n+\t\t\tFD_ZERO(&readfds);\n+\t\t\tFD_ZERO(&writefds);\n+\t\t\tFD_ZERO(&excfds);\n+\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n+\n #if LIBCURL_VERSION_NUM >= 0x070f04\n-\t\t\tlong curl_timeout;\n-\t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n-\t\t\tif (curl_timeout == 0) {\n-\t\t\t\tcontinue;\n-\t\t\t} else if (curl_timeout == -1) {\n+\t\t\t/* It will sometimes happen that curl_multi_fdset() doesn't\n+\t\t\t return any file descriptors. In that case, it's recommended\n+\t\t\t that the application sleep for a short time before running\n+\t\t\t curl_multi_perform() again.\n+\n+\t\t\t http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n+\t\t\t*/\n+\t\t\tif (max_fd == -1) {\n \t\t\t\tselect_timeout.tv_sec = 0;\n \t\t\t\tselect_timeout.tv_usec = 50000;\n \t\t\t} else {\n-\t\t\t\tselect_timeout.tv_sec = curl_timeout / 1000;\n-\t\t\t\tselect_timeout.tv_usec = (curl_timeout % 1000) * 1000;\n+\t\t\t\tcurl_timeout = 0;\n+\t\t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n+\t\t\t\tif (curl_timeout == 0) {\n+\t\t\t\t\tcontinue;\n+\t\t\t\t} else if (curl_timeout == -1) {\n+\t\t\t\t\tselect_timeout.tv_sec = 0;\n+\t\t\t\t\tselect_timeout.tv_usec = 50000;\n+\t\t\t\t} else {\n+\t\t\t\t\tselect_timeout.tv_sec = curl_timeout / 1000;\n+\t\t\t\t\tselect_timeout.tv_usec = (curl_timeout % 1000) * 1000;\n+\t\t\t\t}\n \t\t\t}\n #else\n \t\t\tselect_timeout.tv_sec = 0;\n \t\t\tselect_timeout.tv_usec = 50000;\n #endif\n-\n-\t\t\tmax_fd = -1;\n-\t\t\tFD_ZERO(&readfds);\n-\t\t\tFD_ZERO(&writefds);\n-\t\t\tFD_ZERO(&excfds);\n-\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n-\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n-- \n1.7.7.3\n"},{"id":"201535","messageId":"7vd30fl736.fsf@alter.siamese.dyndns.org","threadId":"31860","inReplyTo":"5080761e.IEDySUuQzlCwL6fM%szager@google.com","subject":"Re: Fix potential hang in https handshake.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-18T22:59:41Z","receivedAt":"2012-10-18T22:59:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"szager@google.com writes:\n\n> From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001\n> From: Stefan Zager \n> Date: Thu, 18 Oct 2012 14:03:59 -0700\n> Subject: [PATCH] Fix potential hang in https handshake.\n>\n> It will sometimes happen that curl_multi_fdset() doesn't\n> return any file descriptors. In that case, it's recommended\n> that the application sleep for a short time before running\n> curl_multi_perform() again.\n>\n> http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n>\n> Signed-off-by: Stefan Zager \n> ---\n\nThanks. Would it be a better idea to \"patch up\" in problematic\ncase, instead of making this logic too deeply nested, like this\ninstead, I have to wonder...\n\n\n\t... all the existing code above unchanged ...\n\tcurl_multi_fdset(..., &max_fd);\n+\tif (max_fd < 0) { \n+\t\t/* nothing actionable??? */\n+\t\tselect_timeout.tv_sec = 0;\n+\t\tselect_timeout.tv_usec = 50000;\n+\t}\n\n\tselect(max_fd+1, ..., &select_timeout);\n\n\n\n> http.c | 40 ++++++++++++++++++++++++++--------------\n> 1 files changed, 26 insertions(+), 14 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index df9bb71..a6f66c0 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -602,35 +602,47 @@ void run_active_slot(struct active_request_slot *slot)\n> \tint max_fd;\n> \tstruct timeval select_timeout;\n> \tint finished = 0;\n> +\tlong curl_timeout;\n> \n> \tslot->finished = &finished;\n> \twhile (!finished) {\n> \t\tstep_active_slots();\n> \n> \t\tif (slot->in_use) {\n> +\t\t\tmax_fd = -1;\n> +\t\t\tFD_ZERO(&readfds);\n> +\t\t\tFD_ZERO(&writefds);\n> +\t\t\tFD_ZERO(&excfds);\n> +\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n> +\n> #if LIBCURL_VERSION_NUM >= 0x070f04\n> -\t\t\tlong curl_timeout;\n> -\t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n> -\t\t\tif (curl_timeout == 0) {\n> -\t\t\t\tcontinue;\n> -\t\t\t} else if (curl_timeout == -1) {\n> +\t\t\t/* It will sometimes happen that curl_multi_fdset() doesn't\n> +\t\t\t return any file descriptors. In that case, it's recommended\n> +\t\t\t that the application sleep for a short time before running\n> +\t\t\t curl_multi_perform() again.\n> +\n> +\t\t\t http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n> +\t\t\t*/\n> +\t\t\tif (max_fd == -1) {\n> \t\t\t\tselect_timeout.tv_sec = 0;\n> \t\t\t\tselect_timeout.tv_usec = 50000;\n> \t\t\t} else {\n> -\t\t\t\tselect_timeout.tv_sec = curl_timeout / 1000;\n> -\t\t\t\tselect_timeout.tv_usec = (curl_timeout % 1000) * 1000;\n> +\t\t\t\tcurl_timeout = 0;\n> +\t\t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n> +\t\t\t\tif (curl_timeout == 0) {\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t} else if (curl_timeout == -1) {\n> +\t\t\t\t\tselect_timeout.tv_sec = 0;\n> +\t\t\t\t\tselect_timeout.tv_usec = 50000;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tselect_timeout.tv_sec = curl_timeout / 1000;\n> +\t\t\t\t\tselect_timeout.tv_usec = (curl_timeout % 1000) * 1000;\n> +\t\t\t\t}\n> \t\t\t}\n> #else\n> \t\t\tselect_timeout.tv_sec = 0;\n> \t\t\tselect_timeout.tv_usec = 50000;\n> #endif\n> -\n> -\t\t\tmax_fd = -1;\n> -\t\t\tFD_ZERO(&readfds);\n> -\t\t\tFD_ZERO(&writefds);\n> -\t\t\tFD_ZERO(&excfds);\n> -\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n> -\n> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> \t\t}\n> \t}\n"},{"id":"201548","messageId":"20121019103627.GA29366@sigill.intra.peff.net","threadId":"31860","inReplyTo":"7vd30fl736.fsf@alter.siamese.dyndns.org","subject":"Re: Fix potential hang in https handshake.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-19T10:36:28Z","receivedAt":"2012-10-19T10:36:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 18, 2012 at 03:59:41PM -0700, Junio C Hamano wrote:\n\n> > It will sometimes happen that curl_multi_fdset() doesn't\n> > return any file descriptors. In that case, it's recommended\n> > that the application sleep for a short time before running\n> > curl_multi_perform() again.\n> >\n> > http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n> >\n> > Signed-off-by: Stefan Zager \n> > ---\n> \n> Thanks. Would it be a better idea to \"patch up\" in problematic\n> case, instead of making this logic too deeply nested, like this\n> instead, I have to wonder...\n> \n> \n> \t... all the existing code above unchanged ...\n> \tcurl_multi_fdset(..., &max_fd);\n> +\tif (max_fd < 0) { \n> +\t\t/* nothing actionable??? */\n> +\t\tselect_timeout.tv_sec = 0;\n> +\t\tselect_timeout.tv_usec = 50000;\n> +\t}\n> \n> \tselect(max_fd+1, ..., &select_timeout);\n\nBut wouldn't that override a potentially shorter timeout that curl gave\nus via curl_multi_timeout, making us unnecessarily slow to hand control\nback to curl?\n\nThe current logic is:\n\n - if curl says there is something to do now (timeout == 0), do it\n immediately\n\n - if curl gives us a timeout, use it with select\n\n - otherwise, feed 50ms to selection\n\nIt should not matter what we get from curl_multi_fdset. If there are\nfds, great, we will feed them to select with the timeout, and we may\nbreak out early if there is work to do. If not, then we are already\ndoing this wait.\n\nIOW, it seems like we are _already_ following the advice referenced in\ncurl's manpage. Is there some case I am missing? Confused...\n\n-Peff\n"},{"id":"201555","messageId":"CAJo=hJvWV0WPN5rCYK-JxfaEPWp7syUM1H0w4=Eb27=50+pXjg@mail.gmail.com","threadId":"31860","inReplyTo":"20121019103627.GA29366@sigill.intra.peff.net","subject":"Re: Fix potential hang in https handshake.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-10-19T14:10:46Z","receivedAt":"2012-10-19T14:10:46Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Oct 19, 2012 at 3:36 AM, Jeff King wrote:\n> On Thu, Oct 18, 2012 at 03:59:41PM -0700, Junio C Hamano wrote:\n>\n>> > It will sometimes happen that curl_multi_fdset() doesn't\n>> > return any file descriptors. In that case, it's recommended\n>> > that the application sleep for a short time before running\n>> > curl_multi_perform() again.\n>> >\n>> > http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n>> >\n>> > Signed-off-by: Stefan Zager \n>> > ---\n>>\n>> Thanks. Would it be a better idea to \"patch up\" in problematic\n>> case, instead of making this logic too deeply nested, like this\n>> instead, I have to wonder...\n>>\n>>\n>> ... all the existing code above unchanged ...\n>> curl_multi_fdset(..., &max_fd);\n>> + if (max_fd < 0) {\n>> + /* nothing actionable??? */\n>> + select_timeout.tv_sec = 0;\n>> + select_timeout.tv_usec = 50000;\n>> + }\n>>\n>> select(max_fd+1, ..., &select_timeout);\n>\n> But wouldn't that override a potentially shorter timeout that curl gave\n> us via curl_multi_timeout, making us unnecessarily slow to hand control\n> back to curl?\n>\n> The current logic is:\n>\n> - if curl says there is something to do now (timeout == 0), do it\n> immediately\n>\n> - if curl gives us a timeout, use it with select\n>\n> - otherwise, feed 50ms to selection\n>\n> It should not matter what we get from curl_multi_fdset. If there are\n> fds, great, we will feed them to select with the timeout, and we may\n> break out early if there is work to do. If not, then we are already\n> doing this wait.\n>\n> IOW, it seems like we are _already_ following the advice referenced in\n> curl's manpage. Is there some case I am missing? Confused...\n\nThe issue with the current code is sometimes when libcurl is opening a\nCONNECT style connection through an HTTP proxy it returns a crazy high\ntimeout (>240 seconds) and no fds. In this case Git waits forever.\nStefan observed that using a timeout of 50 ms in this situation to\npoll libcurl is better, as it figures out a lot more quickly that it\nis connected to the proxy and can issue the request.\n"},{"id":"201558","messageId":"7vpq4ejsxz.fsf@alter.siamese.dyndns.org","threadId":"31860","inReplyTo":"CAHOQ7J9W8FdKqzqbuDqj4bcFyN02kUigWtbL_xCen-PYWF9LUg@mail.gmail.com","subject":"Re: Fix potential hang in https handshake.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-19T17:02:48Z","receivedAt":"2012-10-19T17:02:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Zager writes:\n\n> On Oct 19, 2012 7:11 AM, \"Shawn Pearce\" wrote:\n>>\n>> The issue with the current code is sometimes when libcurl is opening a\n>> CONNECT style connection through an HTTP proxy it returns a crazy high\n>> timeout (>240 seconds) and no fds. In this case Git waits forever.\n>> Stefan observed that using a timeout of 50 ms in this situation to\n>> poll libcurl is better, as it figures out a lot more quickly that it\n>> is connected to the proxy and can issue the request.\n>\n> Correct. Anecdotally, the zero-file-descriptor situation happens only once\n> per process invocation, so the risk of passing a too-long timeout to\n> select() is small.\n\nThanks.\n"},{"id":"201560","messageId":"alpine.DEB.2.00.1210191907020.17638@tvnag.unkk.fr","threadId":"31860","inReplyTo":"CAJo=hJvWV0WPN5rCYK-JxfaEPWp7syUM1H0w4=Eb27=50+pXjg@mail.gmail.com","subject":"Re: Fix potential hang in https handshake.","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2012-10-19T17:08:02Z","receivedAt":"2012-10-19T17:08:02Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Fri, 19 Oct 2012, Shawn Pearce wrote:\n\n> The issue with the current code is sometimes when libcurl is opening a \n> CONNECT style connection through an HTTP proxy it returns a crazy high \n> timeout (>240 seconds) and no fds. In this case Git waits forever.\n\nIs this repeatable with a recent libcurl? It certainly sounds like a bug to \nme, and I might be interested in giving a try at tracking it down...\n\n-- \n\n / daniel.haxx.se\n"},{"id":"201566","messageId":"20121019202723.GA24184@sigill.intra.peff.net","threadId":"31860","inReplyTo":"CAJo=hJvWV0WPN5rCYK-JxfaEPWp7syUM1H0w4=Eb27=50+pXjg@mail.gmail.com","subject":"Re: Fix potential hang in https handshake.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-19T20:27:23Z","receivedAt":"2012-10-19T20:27:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 19, 2012 at 07:10:46AM -0700, Shawn O. Pearce wrote:\n\n> > IOW, it seems like we are _already_ following the advice referenced in\n> > curl's manpage. Is there some case I am missing? Confused...\n> \n> The issue with the current code is sometimes when libcurl is opening a\n> CONNECT style connection through an HTTP proxy it returns a crazy high\n> timeout (>240 seconds) and no fds. In this case Git waits forever.\n> Stefan observed that using a timeout of 50 ms in this situation to\n> poll libcurl is better, as it figures out a lot more quickly that it\n> is connected to the proxy and can issue the request.\n\nAh. That sounds like a bug in curl to me. But either way, if we want to\nwork around it, wouldn't the right thing be to override curl's timeout\nin that instance? Like:\n\ndiff --git a/http.c b/http.c\nindex df9bb71..cd07cdf 100644\n--- a/http.c\n+++ b/http.c\n@@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t\tFD_ZERO(&excfds);\n \t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n \n+\t\t\t/*\n+\t\t\t * Sometimes curl will give a really long timeout for a\n+\t\t\t * CONNECT when there are no fds to read, but we can\n+\t\t\t * get better results by running curl_multi_perform\n+\t\t\t * more frequently.\n+\t\t\t */\n+\t\t\tif (maxfd < 0 &&\n+\t\t\t (select_timeout.tv_sec > 0 ||\n+\t\t\t select_timeout.tv_usec > 50000)) {\n+\t\t\t\tselect_timeout.tv_sec = 0;\n+\t\t\t\tselect_timeout.tv_usec = 50000;\n+\t\t\t}\n+\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n\n-Peff\n"},{"id":"201567","messageId":"CAHOQ7J8D-8++vgMh=c0rcTtAKrhWUCQx2nSd_spBzFe=QdXwBw@mail.gmail.com","threadId":"31860","inReplyTo":"20121019202723.GA24184@sigill.intra.peff.net","subject":"Re: Fix potential hang in https handshake.","fromName":"Stefan Zager","fromEmail":"szager@google.com","sentAt":"2012-10-19T20:37:06Z","receivedAt":"2012-10-19T20:37:06Z","isPatch":false,"sender":{"key":"szager@google.com","avatar":null},"body":"On Fri, Oct 19, 2012 at 1:27 PM, Jeff King wrote:\n>\n> On Fri, Oct 19, 2012 at 07:10:46AM -0700, Shawn O. Pearce wrote:\n>\n> > > IOW, it seems like we are _already_ following the advice referenced in\n> > > curl's manpage. Is there some case I am missing? Confused...\n> >\n> > The issue with the current code is sometimes when libcurl is opening a\n> > CONNECT style connection through an HTTP proxy it returns a crazy high\n> > timeout (>240 seconds) and no fds. In this case Git waits forever.\n> > Stefan observed that using a timeout of 50 ms in this situation to\n> > poll libcurl is better, as it figures out a lot more quickly that it\n> > is connected to the proxy and can issue the request.\n>\n> Ah. That sounds like a bug in curl to me. But either way, if we want to\n> work around it, wouldn't the right thing be to override curl's timeout\n> in that instance? Like:\n>\n> diff --git a/http.c b/http.c\n> index df9bb71..cd07cdf 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)\n> FD_ZERO(&excfds);\n> curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n>\n> + /*\n> + * Sometimes curl will give a really long timeout for a\n> + * CONNECT when there are no fds to read, but we can\n> + * get better results by running curl_multi_perform\n> + * more frequently.\n> + */\n> + if (maxfd < 0 &&\n> + (select_timeout.tv_sec > 0 ||\n> + select_timeout.tv_usec > 50000)) {\n> + select_timeout.tv_sec = 0;\n> + select_timeout.tv_usec = 50000;\n> + }\n> +\n> select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> }\n> }\n>\n> -Peff\n\nI have no objection to this; any one else?\n\nStefan\n"},{"id":"201568","messageId":"20121019204035.GA24448@sigill.intra.peff.net","threadId":"31860","inReplyTo":"CAHOQ7J8D-8++vgMh=c0rcTtAKrhWUCQx2nSd_spBzFe=QdXwBw@mail.gmail.com","subject":"Re: Fix potential hang in https handshake.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-19T20:40:35Z","receivedAt":"2012-10-19T20:40:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 19, 2012 at 01:37:06PM -0700, Stefan Zager wrote:\n\n> > diff --git a/http.c b/http.c\n> > index df9bb71..cd07cdf 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)\n> > FD_ZERO(&excfds);\n> > curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n> >\n> > + /*\n> > + * Sometimes curl will give a really long timeout for a\n> > + * CONNECT when there are no fds to read, but we can\n> > + * get better results by running curl_multi_perform\n> > + * more frequently.\n> > + */\n> > + if (maxfd < 0 &&\n> > + (select_timeout.tv_sec > 0 ||\n> > + select_timeout.tv_usec > 50000)) {\n> > + select_timeout.tv_sec = 0;\n> > + select_timeout.tv_usec = 50000;\n> > + }\n> > +\n> > select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> > }\n> > }\n> >\n> I have no objection to this; any one else?\n\nIf you wouldn't mind, I was hoping you could flesh out the comment a bit\nmore with real details of when this happens (and/or put them in the\ncommit message). If this is indeed a bug to be worked around, it will be\na huge help to somebody reading this code in a year who can confirm that\nmodern curl does not need it anymore.\n\n-Peff\n"},{"id":"201569","messageId":"7vr4oui4a9.fsf@alter.siamese.dyndns.org","threadId":"31860","inReplyTo":"20121019202723.GA24184@sigill.intra.peff.net","subject":"Re: Fix potential hang in https handshake.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-19T20:40:46Z","receivedAt":"2012-10-19T20:40:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 19, 2012 at 07:10:46AM -0700, Shawn O. Pearce wrote:\n>\n>> > IOW, it seems like we are _already_ following the advice referenced in\n>> > curl's manpage. Is there some case I am missing? Confused...\n>> \n>> The issue with the current code is sometimes when libcurl is opening a\n>> CONNECT style connection through an HTTP proxy it returns a crazy high\n>> timeout (>240 seconds) and no fds. In this case Git waits forever.\n>> Stefan observed that using a timeout of 50 ms in this situation to\n>> poll libcurl is better, as it figures out a lot more quickly that it\n>> is connected to the proxy and can issue the request.\n>\n> Ah. That sounds like a bug in curl to me. But either way, if we want to\n> work around it, wouldn't the right thing be to override curl's timeout\n> in that instance? Like:\n\nYeah, that sounds like a more targetted workaround (read: better).\n\n>\n> diff --git a/http.c b/http.c\n> index df9bb71..cd07cdf 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -631,6 +631,19 @@ void run_active_slot(struct active_request_slot *slot)\n> \t\t\tFD_ZERO(&excfds);\n> \t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n> \n> +\t\t\t/*\n> +\t\t\t * Sometimes curl will give a really long timeout for a\n> +\t\t\t * CONNECT when there are no fds to read, but we can\n> +\t\t\t * get better results by running curl_multi_perform\n> +\t\t\t * more frequently.\n> +\t\t\t */\n> +\t\t\tif (maxfd < 0 &&\n> +\t\t\t (select_timeout.tv_sec > 0 ||\n> +\t\t\t select_timeout.tv_usec > 50000)) {\n> +\t\t\t\tselect_timeout.tv_sec = 0;\n> +\t\t\t\tselect_timeout.tv_usec = 50000;\n> +\t\t\t}\n> +\n> \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n> \t\t}\n> \t}\n>\n> -Peff\n"}]}