{"thread":{"id":"35888","subject":"[BUG?] git http connection reuse","startedAt":"2014-02-16T04:05:38Z","lastAt":"2014-02-18T10:34:20Z","messageCount":11,"participants":["Jeff King","Kyle J. McKay","Daniel Stenberg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"234878","messageId":"20140216040538.GA4754@sigill.intra.peff.net","threadId":"35888","inReplyTo":null,"subject":"[BUG?] git http connection reuse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-16T04:05:38Z","receivedAt":"2014-02-16T04:05:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I've noticed that git does not reuse http connections when fetching, and\nI'm trying to figure out why. It seems related to our use of two curl\nfeatures:\n\n 1. If we use the curl_multi interface (even though we are doing the\n requests sequentially), we do not get reuse.\n\n 2. If we set CURLOPT_HTTPAUTH to CURLAUTH_ANY, we do not get reuse.\n\nIt's fairly easy to replicate with the patch below. Running:\n\n rm -rf test &&\n GIT_CURL_VERBOSE=1 git clone https://github.com/peff/test 2>&1 |\n grep -i connect\n\nproduces this with stock git:\n\n * Connected to github.com (192.30.252.131) port 443 (#0)\n * SSL connection using ECDHE-RSA-AES128-GCM-SHA256\n * Connection #0 to host github.com left intact\n * Connected to github.com (192.30.252.131) port 443 (#1)\n * SSL connection using ECDHE-RSA-AES128-GCM-SHA256\n * Connection #1 to host github.com left intact\n\nand this with the patched git:\n\n * Connected to github.com (192.30.252.131) port 443 (#0)\n * SSL connection using ECDHE-RSA-AES128-GCM-SHA256\n * Connection #0 to host github.com left intact\n * Re-using existing connection! (#0) with host github.com\n * Connected to github.com (192.30.252.131) port 443 (#0)\n * Connection #0 to host github.com left intact\n\nThe patch itself just turns off our use of these features (and is\nobviously just for experimentation, not inclusion):\n\ndiff --git a/http.c b/http.c\nindex 70eaa26..8e642fa 100644\n--- a/http.c\n+++ b/http.c\n@@ -313,9 +313,6 @@ static CURL *get_curl_handle(void)\n #if LIBCURL_VERSION_NUM >= 0x070907\n \tcurl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);\n #endif\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n-#endif\n \n \tif (http_proactive_auth)\n \t\tinit_curl_http_auth(result);\ndiff --git a/http.h b/http.h\nindex cd37d58..6506347 100644\n--- a/http.h\n+++ b/http.h\n@@ -18,11 +18,6 @@\n */\n #undef USE_CURL_MULTI\n \n-#if LIBCURL_VERSION_NUM >= 0x071000\n-#define USE_CURL_MULTI\n-#define DEFAULT_MAX_REQUESTS 5\n-#endif\n-\n #if LIBCURL_VERSION_NUM < 0x070704\n #define curl_global_cleanup() do { /* nothing */ } while (0)\n #endif\n\nI'm trying to figure out whether these options do not support connection\nreuse for some reason, whether we are just invoking curl in a weird or\nwrong way, or whether it's a curl bug. The curl_multi code is a bit more\ncomplex, but here's a very simple program that demonstrates the issue\nwith CURLAUTH_ANY:\n\n-- >8 --\n#include \n#include \n\nint main(int argc, char **argv)\n{\n CURL *curl = curl_easy_init();\n\n curl_easy_setopt(curl, CURLOPT_URL, argv[1]);\n curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n if (argv[2])\n curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n curl_easy_perform(curl);\n\n curl_easy_setopt(curl, CURLOPT_URL, argv[1]);\n curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n curl_easy_perform(curl);\n return 0;\n}\n-- 8< --\n\nRunning it as \"./a.out http://example.com\" reuses the connection.\nRunning \"./a.out http://example.com break\" will set CURLOPT_HTTPAUTH,\nand cause curl to make two connections. Are we using curl wrong? Do we\nneed to be touching CURLOPT_HTTPAUTH in between the requests (doing so\ndoes not seem to help)?\n\nMy curl version is 7.35.0, if that makes a difference.\n\n-Peff\n"},{"id":"234880","messageId":"377DF3F4-1F9B-4CB0-A9BF-658E561F4349@gmail.com","threadId":"35888","inReplyTo":"20140216040538.GA4754@sigill.intra.peff.net","subject":"Re: [BUG?] git http connection reuse","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-02-16T07:16:41Z","receivedAt":"2014-02-16T07:16:41Z","isPatch":false,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 15, 2014, at 20:05, Jeff King wrote:\n> I've noticed that git does not reuse http connections when fetching, \n> and\n> I'm trying to figure out why. It seems related to our use of two curl\n> features:\n>\n> 1. If we use the curl_multi interface (even though we are doing the\n> requests sequentially), we do not get reuse.\n\nTake a look at the curl_multi_setopt page [1]. I think these explain \nthe curl_multi issue:\n\n> CURLMOPT_PIPELINING\n>\n> Pass a long set to 1 to enable or 0 to disable. Enabling pipelining \n> on a multi handle will make it attempt to perform HTTP Pipelining as \n> far as possible for transfers using this handle. This means that if \n> you add a second request that can use an already existing \n> connection, the second request will be \"piped\" on the same \n> connection rather than being executed in parallel. (Added in 7.16.0)\n>\n>\n> CURLMOPT_MAX_TOTAL_CONNECTIONS\n>\n> Pass a long. The set number will be used as the maximum amount of \n> simultaneously open connections in total. For each new session, \n> libcurl will open a new connection up to the limit set by \n> CURLMOPT_MAX_TOTAL_CONNECTIONS. When the limit is reached, the \n> sessions will be pending until there are available connections. If \n> CURLMOPT_PIPELINING is 1, libcurl will try to pipeline if the host \n> is capable of it.\n>\n> The default value is 0, which means that there is no limit. However, \n> for backwards compatibility, setting it to 0 when \n> CURLMOPT_PIPELINING is 1 will not be treated as unlimited. Instead \n> it will open only 1 connection and try to pipeline on it.\n>\n> (Added in 7.30.0)\n\nIf pipelining is off (the default) and total connections is not 1 it \nsounds to me from the description above that the requests will be \nexecuted on separate connections until the maximum number of \nconnections is in use and then there might be some reuse. That might \nnot be what's actually going on with multi, but that's my guess and I \nthink setting CURLMOPT_PIPELINING to to 1 and then also setting \nCURLMOPT_MAX_TOTAL_CONNECTIONS to a non-zero value might be what you \nwant.\n\n> 2. If we set CURLOPT_HTTPAUTH to CURLAUTH_ANY, we do not get reuse.\n[snip]\n> My curl version is 7.35.0, if that makes a difference.\n\nAnd that is explained by this from the CHANGES file:\n\n> Daniel Stenberg (7 Jan 2014)\n> - ConnectionExists: fix NTLM check for new connection\n>\n> When the requested authentication bitmask includes NTLM, we cannot\n> re-use a connection for another username/password as we then risk\n> re-using NTLM (connection-based auth).\n>\n> This has the unfortunate downside that if you include NTLM as a \n> possible\n> auth, you cannot re-use connections for other usernames/passwords \n> even\n> if NTLM doesn't end up the auth type used.\n>\n> Reported-by: Paras S\n> Patched-by: Paras S\n> Bug: http://curl.haxx.se/mail/lib-2014-01/0046.html\n\nLooks like you're just lucky as that above change first appears in \n7.35.0. But it seems there are some patches for older versions so \nthey might be affected as well [2].\n\nAdjusting your test program to leave the first connection set to \nCURLAUTH_ANY, but then setting the second one to CURLAUTH_ANY with the \ntwo NTLM bits turned off (CURLAUTH_NTLM and CURLAUTH_NTLM_WB) results \nin the connection being reused for me. :)\n\nWith the older cURL versions I already had installed, the second \nconnection is always reused. I didn't see the non-reuse with your \nsample code until I fetched 7.35.0.\n\n--Kyle\n\n[1] http://curl.haxx.se/libcurl/c/curl_multi_setopt.html\n[2] http://curl.haxx.se/docs/adv_20140129.html\n"},{"id":"234886","messageId":"alpine.DEB.2.00.1402161307160.10435@tvnag.unkk.fr","threadId":"35888","inReplyTo":"377DF3F4-1F9B-4CB0-A9BF-658E561F4349@gmail.com","subject":"Re: [BUG?] git http connection reuse","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2014-02-16T12:18:52Z","receivedAt":"2014-02-16T12:18:52Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Sat, 15 Feb 2014, Kyle J. McKay wrote:\n\n> If pipelining is off (the default) and total connections is not 1 it sounds \n> to me from the description above that the requests will be executed on \n> separate connections until the maximum number of connections is in use and \n> then there might be some reuse.\n\nNot exactly. When about to do a request, libcurl will always try to find an \nexisting idle but open connection in its connection pool to re-use. With the \nmulti interface you can of course easily start N requests at once to the same \nhost and then they'll only re-use connections to the extent there are \nconnections to pick, otherwise it'll create new connections.\n\n>> Daniel Stenberg (7 Jan 2014)\n>> - ConnectionExists: fix NTLM check for new connection\n\n> Looks like you're just lucky as that above change first appears in 7.35.0. \n> But it seems there are some patches for older versions so they might be \n> affected as well [2].\n\nRight, the problem is there to make sure that a NTLM-auth connection with \ndifferent credentials aren't re-used. NTLM with its connection-oriented \nauthentication breaks the traditional HTTP paradigms and before this change \nthere was a risk that libcurl would wrongly re-use a NTLM connection that was \ndone with different credentials!\n\nI suspect we introduced a regression here with that fix. I'll dig into this.\n\n-- \n\n / daniel.haxx.se\n"},{"id":"234887","messageId":"alpine.DEB.2.00.1402161431230.10435@tvnag.unkk.fr","threadId":"35888","inReplyTo":"alpine.DEB.2.00.1402161307160.10435@tvnag.unkk.fr","subject":"Re: [BUG?] git http connection reuse","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2014-02-16T13:33:06Z","receivedAt":"2014-02-16T13:33:06Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Sun, 16 Feb 2014, Daniel Stenberg wrote:\n\n> Right, the problem is there to make sure that a NTLM-auth connection with \n> different credentials aren't re-used. NTLM with its connection-oriented \n> authentication breaks the traditional HTTP paradigms and before this change \n> there was a risk that libcurl would wrongly re-use a NTLM connection that \n> was done with different credentials!\n>\n> I suspect we introduced a regression here with that fix. I'll dig into this.\n\nThanks for pointing this out!\n\nI could repeat this problem with a curl test case so I wrote up two, and then \nI made a fix to curl:\n\n https://github.com/bagder/curl/commit/d765099813f5\n\n-- \n\n / daniel.haxx.se\n"},{"id":"234935","messageId":"20140217234711.GA20499@sigill.intra.peff.net","threadId":"35888","inReplyTo":"alpine.DEB.2.00.1402161431230.10435@tvnag.unkk.fr","subject":"Re: [BUG?] git http connection reuse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-17T23:47:11Z","receivedAt":"2014-02-17T23:47:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 16, 2014 at 02:33:06PM +0100, Daniel Stenberg wrote:\n\n> On Sun, 16 Feb 2014, Daniel Stenberg wrote:\n> \n> >Right, the problem is there to make sure that a NTLM-auth\n> >connection with different credentials aren't re-used. NTLM with its\n> >connection-oriented authentication breaks the traditional HTTP\n> >paradigms and before this change there was a risk that libcurl\n> >would wrongly re-use a NTLM connection that was done with different\n> >credentials!\n> >\n> >I suspect we introduced a regression here with that fix. I'll dig into this.\n> \n> Thanks for pointing this out!\n> \n> I could repeat this problem with a curl test case so I wrote up two,\n> and then I made a fix to curl:\n> \n> https://github.com/bagder/curl/commit/d765099813f5\n\nThanks for the quick turnaround. I've confirmed that your patch fixes\nboth my limited test case and Git itself (when using just the curl_easy\ninterface).\n\n-Peff\n"},{"id":"234937","messageId":"20140217235625.GB20499@sigill.intra.peff.net","threadId":"35888","inReplyTo":"alpine.DEB.2.00.1402161307160.10435@tvnag.unkk.fr","subject":"Re: [BUG?] git http connection reuse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-17T23:56:26Z","receivedAt":"2014-02-17T23:56:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 16, 2014 at 01:18:52PM +0100, Daniel Stenberg wrote:\n\n> On Sat, 15 Feb 2014, Kyle J. McKay wrote:\n> \n> >If pipelining is off (the default) and total connections is not 1\n> >it sounds to me from the description above that the requests will\n> >be executed on separate connections until the maximum number of\n> >connections is in use and then there might be some reuse.\n> \n> Not exactly. When about to do a request, libcurl will always try to\n> find an existing idle but open connection in its connection pool to\n> re-use. With the multi interface you can of course easily start N\n> requests at once to the same host and then they'll only re-use\n> connections to the extent there are connections to pick, otherwise\n> it'll create new connections.\n\nRight; I'd expect multiple connections for parallel requests, but in\nthis case we are completing the first and removing the handle before\nstarting the second. Digging further, I was able to reproduce the\nbehavior with a simple program:\n\n-- >8 --\n#include \n#include \n#include \n\nint main(int argc, char **argv)\n{\n CURL *curl = curl_easy_init();\n CURLM *multi = curl_multi_init();\n int want_multi = atoi(argv[2]);\n int i;\n\n curl_easy_setopt(curl, CURLOPT_URL, argv[1]);\n curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n\n for (i = 0; i < 2; i++) {\n if (i < want_multi) {\n int nr;\n\n curl_multi_add_handle(multi, curl);\n do {\n curl_multi_perform(multi, &nr);\n } while (nr);\n curl_multi_remove_handle(multi, curl);\n }\n else\n curl_easy_perform(curl);\n }\n\n return 0;\n}\n-- 8< --\n\nThe program just requests the same URL twice using the same curl handle,\noptionally using the multi interface for zero, one, or both of the\nrequests. For \"0\" and \"2\" (i.e., either both curl_easy or both\ncurl_multi), the connection is reused. But for \"1\" (in which we use\nmulti for the first request, but easy for the second), we do not reuse\nthe connection.\n\nThe manpage for curl_multi_add_handle does say:\n\n When an easy handle has been added to a multi stack, you can not\n and you must not use curl_easy_perform(3) on that handle!\n\nDoes that apply to the handle after it has finished its transaction and\nbeen removed from the multi object (in which case git is Doing It\nWrong)? Is the program above failing to take some step to finalize the\nfirst request, so that the second one knows its connection can be\nreused? Or should curl be handling this?\n\n-Peff\n"},{"id":"234941","messageId":"alpine.DEB.2.00.1402180806070.15465@tvnag.unkk.fr","threadId":"35888","inReplyTo":"20140217235625.GB20499@sigill.intra.peff.net","subject":"Re: [BUG?] git http connection reuse","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2014-02-18T07:13:16Z","receivedAt":"2014-02-18T07:13:16Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Mon, 17 Feb 2014, Jeff King wrote:\n\n> Right; I'd expect multiple connections for parallel requests, but in this \n> case we are completing the first and removing the handle before starting the \n> second. Digging further, I was able to reproduce the behavior with a simple \n> program:\n\nYeah, given your description I had no problems to repeat it either. Turns out \nwe had no decent test case that checked for this so in our eagerness to fix a \nsecurity problem involving \"over-reuse\" we broke this simpler reuse case. Two \nsteps forward, one step backward... :-/\n\n> The manpage for curl_multi_add_handle does say:\n>\n> When an easy handle has been added to a multi stack, you can not\n> and you must not use curl_easy_perform(3) on that handle!\n>\n> Does that apply to the handle after it has finished its transaction and been \n> removed from the multi object (in which case git is Doing It Wrong)?\n\nNo it doesn't. The man page should probably be clarified to express that \nslightly better. It just means that _while_ a handle is added to a multi \nhandle it cannot be used with curl_easy_perform().\n\nSo yes, you can remove indeed it from the handle and then do \ncurl_easy_perform(). It works just fine!\n\nSeveral internal caches are kept in the multi handle when that's used though, \nso when getting the easy handle out after having used it with the multi \ninterface and then using it with the easy interface may cause libcurl to do a \nlittle more work than if you would be able to re-add it to the same multi \nhandle do to the operation with that...\n\n-- \n\n / daniel.haxx.se\n"},{"id":"234944","messageId":"20140218075534.GB29804@sigill.intra.peff.net","threadId":"35888","inReplyTo":"alpine.DEB.2.00.1402180806070.15465@tvnag.unkk.fr","subject":"Re: [BUG?] git http connection reuse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-18T07:55:34Z","receivedAt":"2014-02-18T07:55:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 18, 2014 at 08:13:16AM +0100, Daniel Stenberg wrote:\n\n> >Right; I'd expect multiple connections for parallel requests, but\n> >in this case we are completing the first and removing the handle\n> >before starting the second. Digging further, I was able to\n> >reproduce the behavior with a simple program:\n> \n> Yeah, given your description I had no problems to repeat it either.\n> Turns out we had no decent test case that checked for this so in our\n> eagerness to fix a security problem involving \"over-reuse\" we broke\n> this simpler reuse case. Two steps forward, one step backward... :-/\n\nYou are talking about the NTLM fix here, right?\n\nI think there is still an unrelated issue with curl_multi preventing\nconnection reuse, but I'm not sure from what you say below...\n\n> >Does that apply to the handle after it has finished its transaction\n> >and been removed from the multi object (in which case git is Doing\n> >It Wrong)?\n> \n> No it doesn't. The man page should probably be clarified to express\n> that slightly better. It just means that _while_ a handle is added to\n> a multi handle it cannot be used with curl_easy_perform().\n\nThanks for the clarification. That was my guess from reading it, but\ngiven that it wasn't working as expected, I wondered if we were\nviolating the rules. I saw the documentation fix you just pushed; it\nlooks reasonable to me.\n\n> Several internal caches are kept in the multi handle when that's used\n> though, so when getting the easy handle out after having used it with\n> the multi interface and then using it with the easy interface may\n> cause libcurl to do a little more work than if you would be able to\n> re-add it to the same multi handle do to the operation with that...\n\nI'm not clear whether you mean by this that it is _expected_ in my test\nprogram for curl not to reuse the connection. Or that curl may simply\nhave to do a little more work, and it is still a bug that the connection\nis not reused.\n\nWe can certainly teach git to use the multi interface, even when doing a\nsingle blocking request.\n\n-Peff\n"},{"id":"234945","messageId":"alpine.DEB.2.00.1402180916530.15465@tvnag.unkk.fr","threadId":"35888","inReplyTo":"20140218075534.GB29804@sigill.intra.peff.net","subject":"Re: [BUG?] git http connection reuse","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2014-02-18T08:20:46Z","receivedAt":"2014-02-18T08:20:46Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 18 Feb 2014, Jeff King wrote:\n\n> I think there is still an unrelated issue with curl_multi preventing \n> connection reuse, but I'm not sure from what you say below...\n\n> I'm not clear whether you mean by this that it is _expected_ in my test \n> program for curl not to reuse the connection. Or that curl may simply have \n> to do a little more work, and it is still a bug that the connection is not \n> reused.\n\nArgh, sorry. I thought were still referring to the previous problem. I can \nindeed repeat the problem you talk about with your test code. Thanks! I'll get \nback to you.\n\n-- \n\n / daniel.haxx.se\n"},{"id":"234952","messageId":"alpine.DEB.2.00.1402181001540.15465@tvnag.unkk.fr","threadId":"35888","inReplyTo":"20140218075534.GB29804@sigill.intra.peff.net","subject":"Re: [BUG?] git http connection reuse","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2014-02-18T09:09:29Z","receivedAt":"2014-02-18T09:09:29Z","isPatch":false,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 18 Feb 2014, Jeff King wrote:\n\n> I'm not clear whether you mean by this that it is _expected_ in my test \n> program for curl not to reuse the connection. Or that curl may simply have \n> to do a little more work, and it is still a bug that the connection is not \n> reused.\n\nOkey, I checked this closer now and this is the full explanation to what \nhappens. It seems to work as intended:\n\nIt's all about where the connection cache is held by libcurl. When you create \na multi handle, it will create a connection cache that will automatically be \nshared by all easy handles that are added to it.\n\nIf you create an easy handle and make a curl_easy_perform() on that, it will \ncreate its own connection cache and keep it associated with this easy handle.\n\nWhen first using an easy handle within a multi handle it will use the shared \nconnection cache in there as long as it is in that multi handle family, but as \nsoon as you remove it from there it will be detached from that connection \ncache.\n\nThen, when doing a fresh request with easy_perform using the handle that was \ndetached from the multi handle, it will create and use its own private cache \nas it can't re-use the previous connection that is cached within the multi \nhandle.\n\n> We can certainly teach git to use the multi interface, even when doing a \n> single blocking request.\n\nFor connection re-use purposes, that may make a lot of sense.\n\n-- \n\n / daniel.haxx.se\n"},{"id":"234960","messageId":"20140218103420.GA2848@sigill.intra.peff.net","threadId":"35888","inReplyTo":"alpine.DEB.2.00.1402181001540.15465@tvnag.unkk.fr","subject":"http: never use curl_easy_perform","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-18T10:34:20Z","receivedAt":"2014-02-18T10:34:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 18, 2014 at 10:09:29AM +0100, Daniel Stenberg wrote:\n\n> Okey, I checked this closer now and this is the full explanation to\n> what happens. It seems to work as intended:\n\nThanks, your explanation makes perfect sense.\n\nI think we should apply the patch below for git to consistently use the\nmulti interface. With this (and the recent patch for the NTLM issue), I\ncan do a whole smart-http clone over a single connection. This doesn't\nmake a huge difference for github.com, because the ssl session cache\neliminates most of the repeated work, but for servers which do not\nimplement ssl session caching, it may be more noticeable.\n\n-- >8 --\nSubject: http: never use curl_easy_perform\n\nWe currently don't reuse http connections when fetching via\nthe smart-http protocol. This is bad because the TCP\nhandshake introduces latency, and especially because SSL\nconnection setup may be non-trivial.\n\nWe can fix it by consistently using curl's \"multi\"\ninterface. The reason is rather complicated:\n\nOur http code has two ways of being used: queuing many\n\"slots\" to be fetched in parallel, or fetching a single\nrequest in a blocking manner. The parallel code is built on\ncurl's \"multi\" interface. Most of the single-request code\nuses http_request, which is built on top of the parallel\ncode (we just feed it one slot, and wait until it finishes).\n\nHowever, one could also accomplish the single-request scheme\nby avoiding curl's multi interface entirely and just using\ncurl_easy_perform. This is simpler, and is used by post_rpc\nin the smart-http protocol.\n\nIt does work to use the same curl handle in both contexts,\nas long as it is not at the same time. However, internally\ncurl may not share all of the cached resources between both\ncontexts. In particular, a connection formed using the\n\"multi\" code will go into a reuse pool connected to the\n\"multi\" object. Further requests using the \"easy\" interface\nwill not be able to reuse that connection.\n\nThe smart http protocol does ref discovery via http_request,\nwhich uses the \"multi\" interface, and then follows up with\nthe \"easy\" interface for its rpc calls. As a result, we make\ntwo HTTP connections rather than reusing a single one.\n\nWe could teach the ref discovery to use the \"easy\"\ninterface. But it is only once we have done this discovery\nthat we know whether the protocol will be smart or dumb. If\nit is dumb, then our further requests, which want to fetch\nobjects in parallel, will not be able to reuse the same\nconnection.\n\nInstead, this patch switches post_rpc to build on the\nparallel interface, which means that we use it consistently\neverywhere. It's a little more complicated to use, but since\nwe have the infrastructure already, it doesn't add any code;\nwe can just factor out the relevant bits from http_request.\n\nSigned-off-by: Jeff King \n---\n http.c | 24 +++++++++++++++---------\n http.h | 9 +++++++++\n remote-curl.c | 5 +----\n 3 files changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 70eaa26..1212c58 100644\n--- a/http.c\n+++ b/http.c\n@@ -880,6 +880,20 @@ int handle_curl_result(struct slot_results *results)\n \t}\n }\n \n+int run_one_slot(struct active_request_slot *slot,\n+\t\t struct slot_results *results)\n+{\n+\tslot->results = results;\n+\tif (!start_active_slot(slot)) {\n+\t\tsnprintf(curl_errorstr, sizeof(curl_errorstr),\n+\t\t\t \"failed to start HTTP request\");\n+\t\treturn HTTP_START_FAILED;\n+\t}\n+\n+\trun_active_slot(slot);\n+\treturn handle_curl_result(results);\n+}\n+\n static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)\n {\n \tchar *ptr;\n@@ -907,7 +921,6 @@ static int http_request(const char *url,\n \tint ret;\n \n \tslot = get_active_slot();\n-\tslot->results = &results;\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \n \tif (result == NULL) {\n@@ -942,14 +955,7 @@ static int http_request(const char *url,\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n \n-\tif (start_active_slot(slot)) {\n-\t\trun_active_slot(slot);\n-\t\tret = handle_curl_result(&results);\n-\t} else {\n-\t\tsnprintf(curl_errorstr, sizeof(curl_errorstr),\n-\t\t\t \"failed to start HTTP request\");\n-\t\tret = HTTP_START_FAILED;\n-\t}\n+\tret = run_one_slot(slot, &results);\n \n \tif (options && options->content_type)\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,\ndiff --git a/http.h b/http.h\nindex cd37d58..a828884 100644\n--- a/http.h\n+++ b/http.h\n@@ -90,6 +90,15 @@ extern void finish_active_slot(struct active_request_slot *slot);\n extern void finish_all_active_slots(void);\n extern int handle_curl_result(struct slot_results *results);\n \n+/*\n+ * This will run one slot to completion in a blocking manner, similar to how\n+ * curl_easy_perform would work (but we don't want to use that, because\n+ * we do not want to intermingle calls to curl_multi and curl_easy).\n+ *\n+ */\n+int run_one_slot(struct active_request_slot *slot,\n+\t\t struct slot_results *results);\n+\n #ifdef USE_CURL_MULTI\n extern void fill_active_slots(void);\n extern void add_fill_function(void *data, int (*fill)(void *));\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 10cb011..52c2d96 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -423,11 +423,8 @@ static int run_slot(struct active_request_slot *slot,\n \tif (!results)\n \t\tresults = &results_buf;\n \n-\tslot->results = results;\n-\tslot->curl_result = curl_easy_perform(slot->curl);\n-\tfinish_active_slot(slot);\n+\terr = run_one_slot(slot, results);\n \n-\terr = handle_curl_result(results);\n \tif (err != HTTP_OK && err != HTTP_REAUTH) {\n \t\terror(\"RPC failed; result=%d, HTTP code = %ld\",\n \t\t results->curl_result, results->http_code);\n-- \n1.8.5.2.500.g8060133\n"}]}