{"thread":{"id":"28827","subject":"[PATCHv2] Improve use of select in http backend","startedAt":"2011-11-02T20:21:26Z","lastAt":"2011-11-04T18:02:30Z","messageCount":19,"participants":["Mika Fischer","Jeff King","Junio C Hamano","Daniel Stenberg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"178717","messageId":"1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":null,"subject":"[PATCHv2] Improve use of select in http backend","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-02T20:21:26Z","receivedAt":"2011-11-02T20:21:26Z","isPatch":false,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"I've split my previous patch into two, since the two parts actually do\ndifferent things.\n\nThe first patch uses curl_multi_fdset to use the file descriptors of the http\nconnections in the call to select, in effect waking up immediately when new\ndata arrives.\n\nThe second patch uses curl_multi_timeout (if available) to get a better\nrecommendation for how long to sleep from curl instead of always sleeping\nfor 50ms.\n"},{"id":"178718","messageId":"1320265288-12647-2-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":"1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de","subject":"[PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-02T20:21:27Z","receivedAt":"2011-11-02T20:21:27Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Instead of sleeping unconditionally for a 50ms, when no data can be read\nfrom the http connection(s), use curl_multi_fdset to obtain the actual\nfile descriptors of the open connections and use them in the select call.\nThis way, the 50ms sleep is interrupted when new data arrives.\n---\n http.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a4bc770..ae92318 100644\n--- a/http.c\n+++ b/http.c\n@@ -664,14 +664,14 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t}\n \n \t\tif (slot->in_use && !data_received) {\n-\t\t\tmax_fd = 0;\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 \t\t\tselect_timeout.tv_sec = 0;\n \t\t\tselect_timeout.tv_usec = 50000;\n-\t\t\tselect(max_fd, &readfds, &writefds,\n-\t\t\t       &excfds, &select_timeout);\n+\t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n #else\n-- \n1.7.8.rc0.33.g09c6f.dirty\n"},{"id":"178719","messageId":"1320265288-12647-3-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":"1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de","subject":"[PATCH 2/2] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-02T20:21:28Z","receivedAt":"2011-11-02T20:21:28Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Recent versions of curl can suggest a period of time the library user\nshould sleep and try again, when curl is blocked on reading or writing\n(or connecting). Use this timeout instead of always sleeping for 50ms.\n---\n http.c |   22 ++++++++++++++++++++--\n 1 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex ae92318..e91a2ab 100644\n--- a/http.c\n+++ b/http.c\n@@ -649,6 +649,9 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n+#if LIBCURL_VERSION_NUM >= 0x070f04\n+\tlong curl_timeout;\n+#endif\n \tint finished = 0;\n \n \tslot->finished = &finished;\n@@ -664,13 +667,28 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t}\n \n \t\tif (slot->in_use && !data_received) {\n+#if LIBCURL_VERSION_NUM >= 0x070f04\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\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}\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-\t\t\tselect_timeout.tv_sec = 0;\n-\t\t\tselect_timeout.tv_usec = 50000;\n+\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n-- \n1.7.8.rc0.33.g09c6f.dirty\n"},{"id":"178720","messageId":"20111102203221.GB5628@sigill.intra.peff.net","threadId":"28827","inReplyTo":"1320265288-12647-2-git-send-email-mika.fischer@zoopnet.de","subject":"Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-02T20:32:21Z","receivedAt":"2011-11-02T20:32:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2011 at 09:21:27PM +0100, Mika Fischer wrote:\n\n> Instead of sleeping unconditionally for a 50ms, when no data can be read\n> from the http connection(s), use curl_multi_fdset to obtain the actual\n> file descriptors of the open connections and use them in the select call.\n> This way, the 50ms sleep is interrupted when new data arrives.\n> ---\n>  http.c |    6 +++---\n>  1 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index a4bc770..ae92318 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -664,14 +664,14 @@ void run_active_slot(struct active_request_slot *slot)\n>  \t\t}\n>  \n>  \t\tif (slot->in_use && !data_received) {\n> -\t\t\tmax_fd = 0;\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>  \t\t\tselect_timeout.tv_sec = 0;\n>  \t\t\tselect_timeout.tv_usec = 50000;\n> -\t\t\tselect(max_fd, &readfds, &writefds,\n> -\t\t\t       &excfds, &select_timeout);\n> +\t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n>  \t\t}\n>  \t}\n\nDo we still need to care about data_received?\n\nMy understanding was that the code was originally trying to do:\n\n  1. Call curl, maybe get some data.\n\n  2. If we got data, then ask curl against immediately for some data.\n\n  3. Otherwise, sleep 50ms and then ask curl again.\n\nBut now that we are actually selecting on the proper descriptors, it\nshould now be safe to just do:\n\n  1. Call curl, maybe get some data.\n\n  2. Call select, which will wake immediately if curl is going to get\n     data.\n\nAt least that's my reading. I am working on unrelated patches that clean\nup the handling of data_received, but if it could go away altogether,\nthat would be even simpler.\n\n-Peff\n"},{"id":"178721","messageId":"20111102203543.GC5628@sigill.intra.peff.net","threadId":"28827","inReplyTo":"20111102203221.GB5628@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-02T20:35:43Z","receivedAt":"2011-11-02T20:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2011 at 04:32:21PM -0400, Jeff King wrote:\n\n> At least that's my reading. I am working on unrelated patches that clean\n> up the handling of data_received, but if it could go away altogether,\n> that would be even simpler.\n\nThat patch, btw, looks like this:\n\n-- >8 --\nSubject: [PATCH] http: remove \"local\" member from slot struct\n\nThe curl-multi http code does something like this:\n\n  while (!finished) {\n\t  try_to_read_from_slots();\n\t  if (!data_received)\n\t\t  wait_for_50_ms();\n  }\n\nWhich is horrible enough in itself, because of the\nhard-coded 50ms wait.\n\nBut there's some additional complexity: the method for\nfinding whether we received data is to actually run ftell()\non the file we are writing to to see if it got anything. So\nthe \"local\" member of the slot struct contains the FILE\npointer. Except that sometimes we don't have a file, because\nwe're writing to a strbuf. In that case, since curl calls\nour custom callback, we just increment the data_received\nflag when curl gives data to our callback.\n\nLet's do the same thing for the write-to-file case as we do\nfor the write-to-strbuf case: use a thin wrapper callback\nand increment the received flag. This makes both methods\nconsistent with each other, and saves us from managing the\n\"local\" struct member at all, reducing the code size.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c |   23 +++++++----------------\n http.h |    1 -\n 2 files changed, 7 insertions(+), 17 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a4bc770..99386ef 100644\n--- a/http.c\n+++ b/http.c\n@@ -93,6 +93,12 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+size_t fwrite_file(char *ptr, size_t eltsize, size_t nmemb, void *handle)\n+{\n+\tdata_received++;\n+\treturn fwrite(ptr, eltsize, nmemb, handle);\n+}\n+\n size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -538,7 +544,6 @@ struct active_request_slot *get_active_slot(void)\n \n \tactive_requests++;\n \tslot->in_use = 1;\n-\tslot->local = NULL;\n \tslot->results = NULL;\n \tslot->finished = NULL;\n \tslot->callback_data = NULL;\n@@ -642,8 +647,6 @@ void step_active_slots(void)\n void run_active_slot(struct active_request_slot *slot)\n {\n #ifdef USE_CURL_MULTI\n-\tlong last_pos = 0;\n-\tlong current_pos;\n \tfd_set readfds;\n \tfd_set writefds;\n \tfd_set excfds;\n@@ -656,13 +659,6 @@ void run_active_slot(struct active_request_slot *slot)\n \t\tdata_received = 0;\n \t\tstep_active_slots();\n \n-\t\tif (!data_received && slot->local != NULL) {\n-\t\t\tcurrent_pos = ftell(slot->local);\n-\t\t\tif (current_pos > last_pos)\n-\t\t\t\tdata_received++;\n-\t\t\tlast_pos = current_pos;\n-\t\t}\n-\n \t\tif (slot->in_use && !data_received) {\n \t\t\tmax_fd = 0;\n \t\t\tFD_ZERO(&readfds);\n@@ -818,13 +814,12 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\tif (target == HTTP_REQUEST_FILE) {\n \t\t\tlong posn = ftell(result);\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n-\t\t\t\t\t fwrite);\n+\t\t\t\t\t fwrite_file);\n \t\t\tif (posn > 0) {\n \t\t\t\tstrbuf_addf(&buf, \"Range: bytes=%ld-\", posn);\n \t\t\t\theaders = curl_slist_append(headers, buf.buf);\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n-\t\t\tslot->local = result;\n \t\t} else\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n \t\t\t\t\t fwrite_buffer);\n@@ -871,7 +866,6 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\tret = HTTP_START_FAILED;\n \t}\n \n-\tslot->local = NULL;\n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n \n@@ -1066,7 +1060,6 @@ void release_http_pack_request(struct http_pack_request *preq)\n \tif (preq->packfile != NULL) {\n \t\tfclose(preq->packfile);\n \t\tpreq->packfile = NULL;\n-\t\tpreq->slot->local = NULL;\n \t}\n \tif (preq->range_header != NULL) {\n \t\tcurl_slist_free_all(preq->range_header);\n@@ -1088,7 +1081,6 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n-\tpreq->slot->local = NULL;\n \n \tlst = preq->lst;\n \twhile (*lst != p)\n@@ -1157,7 +1149,6 @@ struct http_pack_request *new_http_pack_request(\n \t}\n \n \tpreq->slot = get_active_slot();\n-\tpreq->slot->local = preq->packfile;\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);\ndiff --git a/http.h b/http.h\nindex 3c332a9..7429381 100644\n--- a/http.h\n+++ b/http.h\n@@ -49,7 +49,6 @@ struct slot_results {\n \n struct active_request_slot {\n \tCURL *curl;\n-\tFILE *local;\n \tint in_use;\n \tCURLcode curl_result;\n \tlong http_code;\n-- \n1.7.7.rc3.12.g571d67\n"},{"id":"178724","messageId":"7vehxqi5bt.fsf@alter.siamese.dyndns.org","threadId":"28827","inReplyTo":"20111102203543.GC5628@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-02T21:26:30Z","receivedAt":"2011-11-02T21:26:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Nov 02, 2011 at 04:32:21PM -0400, Jeff King wrote:\n>\n>> At least that's my reading. I am working on unrelated patches that clean\n>> up the handling of data_received, but if it could go away altogether,\n>> that would be even simpler.\n>\n> That patch, btw, looks like this:\n>\n> -- >8 --\n> Subject: [PATCH] http: remove \"local\" member from slot struct\n>\n> The curl-multi http code does something like this:\n>\n>   while (!finished) {\n> \t  try_to_read_from_slots();\n> \t  if (!data_received)\n> \t\t  wait_for_50_ms();\n>   }\n>\n> ...\n> Let's do the same thing for the write-to-file case as we do\n> for the write-to-strbuf case: use a thin wrapper callback\n> and increment the received flag. This makes both methods\n> consistent with each other, and saves us from managing the\n> \"local\" struct member at all, reducing the code size.\n\nLooks very sensible.\n"},{"id":"178730","messageId":"CAOs=hR+QqUpYuth8Uvi2o7pm1LO8ogO2pN7nrMchYj96Cutmww@mail.gmail.com","threadId":"28827","inReplyTo":"20111102203221.GB5628@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-02T22:22:48Z","receivedAt":"2011-11-02T22:22:48Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"On Wed, Nov 2, 2011 at 21:32, Jeff King <peff@peff.net> wrote:\n> Do we still need to care about data_received?\n>\n> My understanding was that the code was originally trying to do:\n>\n>  1. Call curl, maybe get some data.\n>\n>  2. If we got data, then ask curl against immediately for some data.\n>\n>  3. Otherwise, sleep 50ms and then ask curl again.\n\nYes, that's exactly what it did.\n\n> But now that we are actually selecting on the proper descriptors, it\n> should now be safe to just do:\n>\n>  1. Call curl, maybe get some data.\n>\n>  2. Call select, which will wake immediately if curl is going to get\n>     data.\n\nThe only problem I can see is that curl_multi_fdset is not guaranteed\nto return any fds. So in theory it could be possible that we don't get\nfds, but we're actually reading stuff. In this case things would get\nslow, because we would sleep for 50ms after every read...\n\nHowever, I don't know if this is a case that actually comes up in the\nreal world. Maybe Daniel has some advice on this.\n\nBest,\n Mika\n"},{"id":"178731","messageId":"alpine.DEB.2.00.1111022336270.7774@tvnag.unkk.fr","threadId":"28827","inReplyTo":"CAOs=hR+QqUpYuth8Uvi2o7pm1LO8ogO2pN7nrMchYj96Cutmww@mail.gmail.com","subject":"Re: [PATCH 1/2] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2011-11-02T22:40:49Z","receivedAt":"2011-11-02T22:40:49Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Wed, 2 Nov 2011, Mika Fischer wrote:\n\n> The only problem I can see is that curl_multi_fdset is not guaranteed to \n> return any fds. So in theory it could be possible that we don't get fds, but \n> we're actually reading stuff. In this case things would get slow, because we \n> would sleep for 50ms after every read...\n>\n> However, I don't know if this is a case that actually comes up in the real \n> world. Maybe Daniel has some advice on this.\n\nIt doesn't really happen so it should be safe.\n\nThe case where no fds are returned is when libcurl cannot return a socket to \nwait for during name resolving (if your particular libcurl is built to use \nsuch a resolver backend - libcurl has several different ones). And during name \nresolving there won't be any data to read for the libcurl-app anyway.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"178803","messageId":"7v7h3ger3t.fsf@alter.siamese.dyndns.org","threadId":"28827","inReplyTo":"1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de","subject":"Re: [PATCHv2] Improve use of select in http backend","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-03T23:14:14Z","receivedAt":"2011-11-03T23:14:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I re-read this thread once again, and my understanding of the current\nsituation on these two patch series is the following. Please correct me if\nI am wrong.\n\n * This is not a regression fix, this is not a correctness fix, but it is\n   a performance improvement;\n\n * Jeff gave an idea for improvement around the use of (rather, not having\n   to use) data_received; and\n\n * Mika understood Jeff's suggestion, but was hesitant due to one\n   potential issue around curl_multi_fdset() and asked Daniel's opinion,\n   to which Daniel responded that the worrysome situation would not\n   happen.\n\nIt appears to me that the next step is for Mika to decide either (1) we go\nahead with the original patch and leave the improvement for later, or (2)\nupdate the patch as Jeff suggested and we review it again.\n\nI can go either way, but whichever way you choose, I would want to see the\npatches properly signed-off.\n\nThanks.\n"},{"id":"178822","messageId":"1320416367-28843-1-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":"1320265288-12647-1-git-send-email-mika.fischer@zoopnet.de","subject":"[PATCH v3 0/3] Improve use of select in http backend","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-04T14:19:24Z","receivedAt":"2011-11-04T14:19:24Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Changes since v2:\n- Properly signed off\n- Incorporated Jeff's suggestion of getting rid of data_received\n  altogether\n\nAnd yes, this is just a performance improvement, not a bugfix.\n\nMika Fischer (3):\n  http.c: Use curl_multi_fdset to select on curl fds instead of just\n    sleeping\n  http.c: Use timeout suggested by curl instead of fixed 50ms timeout\n  http.c: Rely on select instead of tracking whether data was received\n\n http.c |   42 +++++++++++++++++++++++-------------------\n http.h |    1 -\n 2 files changed, 23 insertions(+), 20 deletions(-)\n\n-- \n1.7.8.rc0.35.gd9f16.dirty\n"},{"id":"178821","messageId":"1320416367-28843-2-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":"1320416367-28843-1-git-send-email-mika.fischer@zoopnet.de","subject":"[PATCH v3 1/3] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-04T14:19:25Z","receivedAt":"2011-11-04T14:19:25Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Instead of sleeping unconditionally for a 50ms, when no data can be read\nfrom the http connection(s), use curl_multi_fdset to obtain the actual\nfile descriptors of the open connections and use them in the select call.\nThis way, the 50ms sleep is interrupted when new data arrives.\n\nSigned-off-by: Mika Fischer <mika.fischer@zoopnet.de>\n---\n http.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a4bc770..ae92318 100644\n--- a/http.c\n+++ b/http.c\n@@ -664,14 +664,14 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t}\n \n \t\tif (slot->in_use && !data_received) {\n-\t\t\tmax_fd = 0;\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 \t\t\tselect_timeout.tv_sec = 0;\n \t\t\tselect_timeout.tv_usec = 50000;\n-\t\t\tselect(max_fd, &readfds, &writefds,\n-\t\t\t       &excfds, &select_timeout);\n+\t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n #else\n-- \n1.7.8.rc0.35.gd9f16.dirty\n"},{"id":"178823","messageId":"1320416367-28843-3-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":"1320416367-28843-1-git-send-email-mika.fischer@zoopnet.de","subject":"[PATCH v3 2/3] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-04T14:19:26Z","receivedAt":"2011-11-04T14:19:26Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Recent versions of curl can suggest a period of time the library user\nshould sleep and try again, when curl is blocked on reading or writing\n(or connecting). Use this timeout instead of always sleeping for 50ms.\n\nSigned-off-by: Mika Fischer <mika.fischer@zoopnet.de>\n---\n http.c |   22 ++++++++++++++++++++--\n 1 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex ae92318..e91a2ab 100644\n--- a/http.c\n+++ b/http.c\n@@ -649,6 +649,9 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n+#if LIBCURL_VERSION_NUM >= 0x070f04\n+\tlong curl_timeout;\n+#endif\n \tint finished = 0;\n \n \tslot->finished = &finished;\n@@ -664,13 +667,28 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t}\n \n \t\tif (slot->in_use && !data_received) {\n+#if LIBCURL_VERSION_NUM >= 0x070f04\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\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}\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-\t\t\tselect_timeout.tv_sec = 0;\n-\t\t\tselect_timeout.tv_usec = 50000;\n+\n \t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n-- \n1.7.8.rc0.35.gd9f16.dirty\n"},{"id":"178824","messageId":"1320416367-28843-4-git-send-email-mika.fischer@zoopnet.de","threadId":"28827","inReplyTo":"1320416367-28843-1-git-send-email-mika.fischer@zoopnet.de","subject":"[PATCH v3 3/3] http.c: Rely on select instead of tracking whether data was received","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-04T14:19:27Z","receivedAt":"2011-11-04T14:19:27Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Since now select is used with the file descriptors of the http connections,\ntracking whether data was received recently (and trying to read more in\nthat case) is no longer necessary. Instead, always call select and rely on\nit to return as soon as new data can be read.\n\nSigned-off-by: Mika Fischer <mika.fischer@zoopnet.de>\n---\n http.c |   16 +---------------\n http.h |    1 -\n 2 files changed, 1 insertions(+), 16 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex e91a2ab..3c6a00b 100644\n--- a/http.c\n+++ b/http.c\n@@ -4,7 +4,6 @@\n #include \"run-command.h\"\n #include \"url.h\"\n \n-int data_received;\n int active_requests;\n int http_is_verbose;\n size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n@@ -99,13 +98,11 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n \tstruct strbuf *buffer = buffer_;\n \n \tstrbuf_add(buffer, ptr, size);\n-\tdata_received++;\n \treturn size;\n }\n \n size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)\n {\n-\tdata_received++;\n \treturn eltsize * nmemb;\n }\n \n@@ -642,8 +639,6 @@ void step_active_slots(void)\n void run_active_slot(struct active_request_slot *slot)\n {\n #ifdef USE_CURL_MULTI\n-\tlong last_pos = 0;\n-\tlong current_pos;\n \tfd_set readfds;\n \tfd_set writefds;\n \tfd_set excfds;\n@@ -656,17 +651,9 @@ void run_active_slot(struct active_request_slot *slot)\n \n \tslot->finished = &finished;\n \twhile (!finished) {\n-\t\tdata_received = 0;\n \t\tstep_active_slots();\n \n-\t\tif (!data_received && slot->local != NULL) {\n-\t\t\tcurrent_pos = ftell(slot->local);\n-\t\t\tif (current_pos > last_pos)\n-\t\t\t\tdata_received++;\n-\t\t\tlast_pos = current_pos;\n-\t\t}\n-\n-\t\tif (slot->in_use && !data_received) {\n+\t\tif (slot->in_use) {\n #if LIBCURL_VERSION_NUM >= 0x070f04\n \t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n \t\t\tif (curl_timeout == 0) {\n@@ -1232,7 +1219,6 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,\n \t\tgit_SHA1_Update(&freq->c, expn,\n \t\t\t\tsizeof(expn) - freq->stream.avail_out);\n \t} while (freq->stream.avail_in && freq->zret == Z_OK);\n-\tdata_received++;\n \treturn size;\n }\n \ndiff --git a/http.h b/http.h\nindex 3c332a9..71bdf58 100644\n--- a/http.h\n+++ b/http.h\n@@ -89,7 +89,6 @@ extern void step_active_slots(void);\n extern void http_init(struct remote *remote, const char *url);\n extern void http_cleanup(void);\n \n-extern int data_received;\n extern int active_requests;\n extern int http_is_verbose;\n extern size_t http_post_buffer;\n-- \n1.7.8.rc0.35.gd9f16.dirty\n"},{"id":"178839","messageId":"7vehxndd4q.fsf@alter.siamese.dyndns.org","threadId":"28827","inReplyTo":"1320416367-28843-3-git-send-email-mika.fischer@zoopnet.de","subject":"Re: [PATCH v3 2/3] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-04T17:13:41Z","receivedAt":"2011-11-04T17:13:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mika Fischer <mika.fischer@zoopnet.de> writes:\n\n> Recent versions of curl can suggest a period of time the library user\n> should sleep and try again, when curl is blocked on reading or writing\n> (or connecting). Use this timeout instead of always sleeping for 50ms.\n>\n> Signed-off-by: Mika Fischer <mika.fischer@zoopnet.de>\n\nThanks.\n\nI'm inclined to squash in the following to narrow the scope of\ncurl_timeout, though.\n\ndiff --git a/http.c b/http.c\nindex 5cb0fb6..924be52 100644\n--- a/http.c\n+++ b/http.c\n@@ -636,9 +636,6 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n-#if LIBCURL_VERSION_NUM >= 0x070f04\n-\tlong curl_timeout;\n-#endif\n \tint finished = 0;\n \n \tslot->finished = &finished;\n@@ -655,6 +652,7 @@ void run_active_slot(struct active_request_slot *slot)\n \n \t\tif (slot->in_use && !data_received) {\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"},{"id":"178841","messageId":"CAOs=hRKxc9SdE_HTnfs+WdnxZEY6yF9MBV_K1FX2=7B7xtj7-w@mail.gmail.com","threadId":"28827","inReplyTo":"7vehxndd4q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/3] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-04T17:47:44Z","receivedAt":"2011-11-04T17:47:44Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"On Fri, Nov 4, 2011 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:\n> I'm inclined to squash in the following to narrow the scope of\n> curl_timeout, though.\n>\n> diff --git a/http.c b/http.c\n> index 5cb0fb6..924be52 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -636,9 +636,6 @@ void run_active_slot(struct active_request_slot *slot)\n>        fd_set excfds;\n>        int max_fd;\n>        struct timeval select_timeout;\n> -#if LIBCURL_VERSION_NUM >= 0x070f04\n> -       long curl_timeout;\n> -#endif\n>        int finished = 0;\n>\n>        slot->finished = &finished;\n> @@ -655,6 +652,7 @@ void run_active_slot(struct active_request_slot *slot)\n>\n>                if (slot->in_use && !data_received) {\n>  #if LIBCURL_VERSION_NUM >= 0x070f04\n> +                       long curl_timeout;\n>                        curl_multi_timeout(curlm, &curl_timeout);\n>                        if (curl_timeout == 0) {\n>                                continue;\n\nAh yes, that's good. I would have done it this way in C++, but I\nwasn't sure whether C99 is OK for git.\n"},{"id":"178842","messageId":"20111104175127.GA26118@sigill.intra.peff.net","threadId":"28827","inReplyTo":"CAOs=hRKxc9SdE_HTnfs+WdnxZEY6yF9MBV_K1FX2=7B7xtj7-w@mail.gmail.com","subject":"Re: [PATCH v3 2/3] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-04T17:51:27Z","receivedAt":"2011-11-04T17:51:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 04, 2011 at 06:47:44PM +0100, Mika Fischer wrote:\n\n> >                if (slot->in_use && !data_received) {\n> >  #if LIBCURL_VERSION_NUM >= 0x070f04\n> > +                       long curl_timeout;\n> >                        curl_multi_timeout(curlm, &curl_timeout);\n> >                        if (curl_timeout == 0) {\n> >                                continue;\n> \n> Ah yes, that's good. I would have done it this way in C++, but I\n> wasn't sure whether C99 is OK for git.\n\nC99 is not OK. But this is not C99, as the conditional opens a new\nblock.\n\n-Peff\n"},{"id":"178843","messageId":"20111104175333.GB26118@sigill.intra.peff.net","threadId":"28827","inReplyTo":"1320416367-28843-1-git-send-email-mika.fischer@zoopnet.de","subject":"Re: [PATCH v3 0/3] Improve use of select in http backend","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-04T17:53:33Z","receivedAt":"2011-11-04T17:53:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 04, 2011 at 03:19:24PM +0100, Mika Fischer wrote:\n\n> Mika Fischer (3):\n>   http.c: Use curl_multi_fdset to select on curl fds instead of just\n>     sleeping\n>   http.c: Use timeout suggested by curl instead of fixed 50ms timeout\n>   http.c: Rely on select instead of tracking whether data was received\n\nAll three patches look good to me. Your 3/3 does most of the cleanup\nfrom the other patch I posted, but we can also do this on top:\n\n-- >8 --\nSubject: [PATCH 4/3] http: drop \"local\" member from request struct\n\nThis is a FILE pointer in the case that we are sending our\noutput to a file. We originally used it to run ftell() to\ndetermine whether data had been written to our file during\nour last call to curl. However, as of the last patch, we no\nlonger care about that flag anymore. All uses of this struct\nmember are now just book-keeping that can go away.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c |    6 ------\n http.h |    1 -\n 2 files changed, 0 insertions(+), 7 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 3c6a00b..cfa9b07 100644\n--- a/http.c\n+++ b/http.c\n@@ -535,7 +535,6 @@ struct active_request_slot *get_active_slot(void)\n \n \tactive_requests++;\n \tslot->in_use = 1;\n-\tslot->local = NULL;\n \tslot->results = NULL;\n \tslot->finished = NULL;\n \tslot->callback_data = NULL;\n@@ -829,7 +828,6 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\t\t\theaders = curl_slist_append(headers, buf.buf);\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n-\t\t\tslot->local = result;\n \t\t} else\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n \t\t\t\t\t fwrite_buffer);\n@@ -876,7 +874,6 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\tret = HTTP_START_FAILED;\n \t}\n \n-\tslot->local = NULL;\n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n \n@@ -1071,7 +1068,6 @@ void release_http_pack_request(struct http_pack_request *preq)\n \tif (preq->packfile != NULL) {\n \t\tfclose(preq->packfile);\n \t\tpreq->packfile = NULL;\n-\t\tpreq->slot->local = NULL;\n \t}\n \tif (preq->range_header != NULL) {\n \t\tcurl_slist_free_all(preq->range_header);\n@@ -1093,7 +1089,6 @@ int finish_http_pack_request(struct http_pack_request *preq)\n \n \tfclose(preq->packfile);\n \tpreq->packfile = NULL;\n-\tpreq->slot->local = NULL;\n \n \tlst = preq->lst;\n \twhile (*lst != p)\n@@ -1162,7 +1157,6 @@ struct http_pack_request *new_http_pack_request(\n \t}\n \n \tpreq->slot = get_active_slot();\n-\tpreq->slot->local = preq->packfile;\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);\ndiff --git a/http.h b/http.h\nindex 71bdf58..ee16069 100644\n--- a/http.h\n+++ b/http.h\n@@ -49,7 +49,6 @@ struct slot_results {\n \n struct active_request_slot {\n \tCURL *curl;\n-\tFILE *local;\n \tint in_use;\n \tCURLcode curl_result;\n \tlong http_code;\n-- \n1.7.7.2.4.gfd7b9\n"},{"id":"178844","messageId":"CAOs=hRL1vUFRwAGihi3GBmOrRV5mSXzrbx=T94UuHXW6g47JLw@mail.gmail.com","threadId":"28827","inReplyTo":"20111104175127.GA26118@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-04T17:55:31Z","receivedAt":"2011-11-04T17:55:31Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"On Fri, Nov 4, 2011 at 18:51, Jeff King <peff@peff.net> wrote:\n>> Ah yes, that's good. I would have done it this way in C++, but I\n>> wasn't sure whether C99 is OK for git.\n>\n> C99 is not OK. But this is not C99, as the conditional opens a new\n> block.\n\nOh I see, thanks for clarifying!\n"},{"id":"178846","messageId":"7v62izdavd.fsf@alter.siamese.dyndns.org","threadId":"28827","inReplyTo":"CAOs=hRKxc9SdE_HTnfs+WdnxZEY6yF9MBV_K1FX2=7B7xtj7-w@mail.gmail.com","subject":"Re: [PATCH v3 2/3] http.c: Use timeout suggested by curl instead of fixed 50ms timeout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-04T18:02:30Z","receivedAt":"2011-11-04T18:02:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mika Fischer <mika.fischer@zoopnet.de> writes:\n\n>>                if (slot->in_use && !data_received) {\n>>  #if LIBCURL_VERSION_NUM >= 0x070f04\n>> +                       long curl_timeout;\n>>                        curl_multi_timeout(curlm, &curl_timeout);\n>>                        if (curl_timeout == 0) {\n>>                                continue;\n>\n> Ah yes, that's good. I would have done it this way in C++, but I\n> wasn't sure whether C99 is OK for git.\n\nI do not see anything C99 here.\n"}]}