{"thread":{"id":"44449","subject":"[PATCH] remote-curl: don't hang when a server dies before any output","startedAt":"2016-11-09T22:18:43Z","lastAt":"2016-11-18T18:28:47Z","messageCount":18,"participants":["David Turner","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305646","messageId":"1478729910-26232-1-git-send-email-dturner@twosigma.com","threadId":"44449","inReplyTo":null,"subject":"[PATCH] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-11-09T22:18:30Z","receivedAt":"2016-11-09T22:18:43Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"In the event that a HTTP server closes the connection after giving a\n200 but before giving any packets, we don't want to hang forever\nwaiting for a response that will never come.  Instead, we should die\nimmediately.\n\nOne case where this happens is when attempting to fetch a dangling\nobject by SHA.\n\nStill to do: it would be good to give a better error message\nthan \"fatal: The remote end hung up unexpectedly\".\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n\nNote: if you run t5551 before applying the code patch, the second new\ntest will hang forever.\n\nFWIW, I also saw this kind of hang at Twitter from time to time, but I\nwas never able to reliably reproduce it there, and thus never able to\nfix it.  I suspect that a bad load balancer might have been killing\nconnections just after the headers, but this is pure speculation.\n\nI am sorry that this patch does not provide a more useful error\nmessage.  For us, it is important to fix the hangs (so that we can\nretry on another server, for instance), but less important to be\nuser-friendly (since we were only seeing these with automated\nprocesses).  I hope that someone who actually understands the http\ncode, and has some time, could help improve this aspect of the code.\n\nIf not, then at least we won't have inexplicable hangs.\n\n remote-curl.c               |  8 ++++++++\n t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 38 insertions(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex f14c41f..ee44236 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -400,6 +400,7 @@ struct rpc_state {\n \tsize_t pos;\n \tint in;\n \tint out;\n+\tint any_written;\n \tstruct strbuf result;\n \tunsigned gzip_request : 1;\n \tunsigned initial_buffer : 1;\n@@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n {\n \tsize_t size = eltsize * nmemb;\n \tstruct rpc_state *rpc = buffer_;\n+\tif (size)\n+\t\trpc->any_written = 1;\n \twrite_or_die(rpc->in, ptr, size);\n \treturn size;\n }\n@@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n \n+\n+\trpc->any_written = 0;\n \terr = run_slot(slot, NULL);\n \tif (err == HTTP_REAUTH && !large_request) {\n \t\tcredential_fill(&http_auth);\n@@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)\n \tif (err != HTTP_OK)\n \t\terr = -1;\n \n+\tif (!rpc->any_written)\n+\t\terr = -1;\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 1ec5b27..43665ab 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '\n \ttest_line_count = 2 posts\n '\n \n+test_expect_success 'test allowreachablesha1inwant' '\n+\ttest_when_finished \"rm -rf test_reachable.git\" &&\n+\tserver=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tmaster_sha=$(git -C \"$server\" rev-parse refs/heads/master) &&\n+\tgit -C \"$server\" config uploadpack.allowreachablesha1inwant 1 &&\n+\n+\tgit init --bare test_reachable.git &&\n+\tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n+\tgit -C test_reachable.git fetch origin \"$master_sha\"\n+'\n+\n+test_expect_success 'test allowreachablesha1inwant with unreachable' '\n+\ttest_when_finished \"rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)\" &&\n+\n+\t#create unreachable sha\n+\techo content >file2 &&\n+\tgit add file2 &&\n+\tgit commit -m two &&\n+\tgit push public HEAD:refs/heads/doomed &&\n+\tgit push public :refs/heads/doomed &&\n+\n+\tserver=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tmaster_sha=$(git -C \"$server\" rev-parse refs/heads/master) &&\n+\tgit -C \"$server\" config uploadpack.allowreachablesha1inwant 1 &&\n+\n+\tgit init --bare test_reachable.git &&\n+\tgit -C test_reachable.git remote add origin \"$HTTPD_URL/smart/repo.git\" &&\n+\ttest_must_fail git -C test_reachable.git fetch origin \"$(git rev-parse HEAD)\"\n+'\n+\n test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '\n \t(\n \t\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"305898","messageId":"20161114182431.e7jjnq422c4xobdb@sigill.intra.peff.net","threadId":"44449","inReplyTo":"1478729910-26232-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-14T18:24:31Z","receivedAt":"2016-11-14T18:24:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 09, 2016 at 05:18:30PM -0500, David Turner wrote:\n\n> In the event that a HTTP server closes the connection after giving a\n> 200 but before giving any packets, we don't want to hang forever\n> waiting for a response that will never come.  Instead, we should die\n> immediately.\n\nI agree we don't want to hang forever, but this leaves open the\nquestion: what is hanging?\n\nMy guess is that fetch-pack is waiting for more data from the server,\nand remote-curl is waiting for fetch-pack to tell us what to send for\nthe next request. Neither will make forward progress because they are\neffectively waiting on each other.\n\nWhich means this is likely a special case of malformed input from the\nserver. A server which likewise sends a partial response could end up in\nthe same deadlock, I would think (e.g., a half-finished pktline, or a\npktline but no trailing flush).\n\nThat doesn't make it wrong to fix this specific case (especially if it's\na common one), but I wonder if we could do better.\n\nThe root of the issue is that only fetch-pack understands the protocol,\nand remote-curl is blindly proxying the data. But only remote-curl knows\nthat the HTTP request has ended, and it doesn't relay that information\nto fetch-pack. So I can think of two solutions:\n\n  1. Some way of remote-curl communicating the EOF to fetch-pack. It\n     can't just close the descriptor, since we need to pass more data\n     over it for the followup requests. You'd need something\n     out-of-band, or to frame the HTTP data inside another layer of\n     pktlines, both of which are kind of gross.\n\n  2. Have remote-curl understand enough of the protocol that it can\n     abort rather than hang.\n\n     I think that's effectively the approach of your patch, but for one\n     specific case. But could we, for example, make sure that everything\n     we proxy is a complete set of pktlines and ends with a flush? And\n     if not, then we hang up on fetch-pack.\n\n     I _think_ that would work, because even the pack is always encased\n     in pktlines for smart-http.\n\n> @@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n>  \n> +\n> +\trpc->any_written = 0;\n\nExtra blank line here?\n\n> @@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tif (err != HTTP_OK)\n>  \t\terr = -1;\n>  \n> +\tif (!rpc->any_written)\n> +\t\terr = -1;\n> +\n\nI wondered if there were any cases where it was normal for the server to\nreturn zero bytes. Possibly the ref advertisement is one, but this is\n_just_ handling post_rpc(), so that's OK. And I think by definition\nevery response has to at least return a flush packet, or we would make\nno forward progress (i.e., the exact case you are dealing with here).\n\n-Peff\n"},{"id":"305912","messageId":"20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net","threadId":"44449","inReplyTo":"20161114182431.e7jjnq422c4xobdb@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-14T19:40:49Z","receivedAt":"2016-11-14T19:40:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:\n\n>   2. Have remote-curl understand enough of the protocol that it can\n>      abort rather than hang.\n> \n>      I think that's effectively the approach of your patch, but for one\n>      specific case. But could we, for example, make sure that everything\n>      we proxy is a complete set of pktlines and ends with a flush? And\n>      if not, then we hang up on fetch-pack.\n> \n>      I _think_ that would work, because even the pack is always encased\n>      in pktlines for smart-http.\n\nSo something like this. It turned out to be a lot uglier than I had\nhoped because we get fed the data from curl in odd-sized chunks, so we\nneed a state machine.\n\nBut it does seem to work. At least it doesn't seem to break anything in\nthe test suite, and it fixes the new tests you added. I'd worry that\nthere's some obscure case where the response isn't packetized in the\nsame way.\n\n---\ndiff --git a/remote-curl.c b/remote-curl.c\nindex f14c41f4c..605357d77 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -403,6 +403,18 @@ struct rpc_state {\n \tstruct strbuf result;\n \tunsigned gzip_request : 1;\n \tunsigned initial_buffer : 1;\n+\n+\tenum {\n+\t\tRPC_PKTLINE_ERROR, /* bogus hex chars in length */\n+\t\tRPC_PKTLINE_INITIAL, /* no packets received yet */\n+\t\tRPC_PKTLINE_1, /* got one hex char */\n+\t\tRPC_PKTLINE_2, /* got two hex chars */\n+\t\tRPC_PKTLINE_3, /* got three hex chars */\n+\t\tRPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */\n+\t\tRPC_PKTLINE_END_OF_PACKET, /* last packet completed */\n+\t\tRPC_PKTLINE_FLUSH, /* last packet was flush */\n+\t} pktline_state;\n+\tsize_t pktline_len;\n };\n \n static size_t rpc_out(void *ptr, size_t eltsize,\n@@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+static void update_pktline_state(struct rpc_state *rpc,\n+\t\t\t\t const char *buf, size_t len)\n+{\n+#define READ_ONE_HEX(shift) do { \\\n+\tint val = hexval(buf[0]); \\\n+\tif (val < 0) { \\\n+\t\twarning(\"error on %d\", *buf); \\\n+\t\trpc->pktline_state = RPC_PKTLINE_ERROR; \\\n+\t\treturn; \\\n+\t} \\\n+\trpc->pktline_len |= val << shift; \\\n+\tbuf++; \\\n+\tlen--; \\\n+} while(0)\n+\n+\twhile (len > 0) {\n+\t\tswitch (rpc->pktline_state) {\n+\t\tcase RPC_PKTLINE_ERROR:\n+\t\t\t/* previous error; there is no recovery */\n+\t\t\treturn;\n+\n+\t\t/* We can start a new pktline at any of these states */\n+\t\tcase RPC_PKTLINE_INITIAL:\n+\t\tcase RPC_PKTLINE_FLUSH:\n+\t\tcase RPC_PKTLINE_END_OF_PACKET:\n+\t\t\trpc->pktline_len = 0;\n+\t\t\tREAD_ONE_HEX(12);\n+\t\t\trpc->pktline_state = RPC_PKTLINE_1;\n+\t\t\tbreak;\n+\n+\t\tcase RPC_PKTLINE_1:\n+\t\t\tREAD_ONE_HEX(8);\n+\t\t\trpc->pktline_state = RPC_PKTLINE_2;\n+\t\t\tbreak;\n+\n+\t\tcase RPC_PKTLINE_2:\n+\t\t\tREAD_ONE_HEX(4);\n+\t\t\trpc->pktline_state = RPC_PKTLINE_3;\n+\t\t\tbreak;\n+\n+\t\tcase RPC_PKTLINE_3:\n+\t\t\tREAD_ONE_HEX(0);\n+\t\t\tif (rpc->pktline_len) {\n+\t\t\t\trpc->pktline_state = RPC_PKTLINE_DATA;\n+\t\t\t\trpc->pktline_len -= 4;\n+\t\t\t} else\n+\t\t\t\trpc->pktline_state = RPC_PKTLINE_FLUSH;\n+\t\t\tbreak;\n+\n+\t\tcase RPC_PKTLINE_DATA:\n+\t\t\tif (len < rpc->pktline_len) {\n+\t\t\t\trpc->pktline_len -= len;\n+\t\t\t\tlen = 0;\n+\t\t\t} else {\n+\t\t\t\tbuf += rpc->pktline_len;\n+\t\t\t\tlen -= rpc->pktline_len;\n+\t\t\t\trpc->pktline_len = 0;\n+\t\t\t\trpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;\n+\t\t\t}\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+#undef READ_ONE_HEX\n+}\n+\n static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n \tstruct rpc_state *rpc = buffer_;\n+\tupdate_pktline_state(rpc, ptr, size);\n \twrite_or_die(rpc->in, ptr, size);\n \treturn size;\n }\n@@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n \n+\trpc->pktline_state = RPC_PKTLINE_INITIAL;\n+\n \terr = run_slot(slot, NULL);\n \tif (err == HTTP_REAUTH && !large_request) {\n \t\tcredential_fill(&http_auth);\n@@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)\n \tif (err != HTTP_OK)\n \t\terr = -1;\n \n+\tif (rpc->pktline_state != RPC_PKTLINE_FLUSH) {\n+\t\terror(\"invalid or truncated response from http server\");\n+\t\terr = -1;\n+\t}\n+\n \tcurl_slist_free_all(headers);\n \tfree(gzip_body);\n \treturn err;\n"},{"id":"305920","messageId":"xmqqpolxwyh6.fsf@gitster.mtv.corp.google.com","threadId":"44449","inReplyTo":"20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-14T21:19:49Z","receivedAt":"2016-11-14T21:19:56Z","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> So something like this. It turned out to be a lot uglier than I had\n> hoped because we get fed the data from curl in odd-sized chunks, so we\n> need a state machine.\n\nIt is unfortunate that we have to snoop the protocol like this to\ninfer an error, but I do not think we can do better than that\napproach.  FWIW, I did not find the logic in update_pktline_state()\nyou wrote ugly at all.\n\nHaving to assume that the end of each round from the other end must\nbe a FLUSH does feel somewhat ugly and brittle, though.\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index f14c41f4c..605357d77 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -403,6 +403,18 @@ struct rpc_state {\n>  \tstruct strbuf result;\n>  \tunsigned gzip_request : 1;\n>  \tunsigned initial_buffer : 1;\n> +\n> +\tenum {\n> +\t\tRPC_PKTLINE_ERROR, /* bogus hex chars in length */\n> +\t\tRPC_PKTLINE_INITIAL, /* no packets received yet */\n> +\t\tRPC_PKTLINE_1, /* got one hex char */\n> +\t\tRPC_PKTLINE_2, /* got two hex chars */\n> +\t\tRPC_PKTLINE_3, /* got three hex chars */\n> +\t\tRPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */\n> +\t\tRPC_PKTLINE_END_OF_PACKET, /* last packet completed */\n> +\t\tRPC_PKTLINE_FLUSH, /* last packet was flush */\n> +\t} pktline_state;\n> +\tsize_t pktline_len;\n>  };\n>  \n>  static size_t rpc_out(void *ptr, size_t eltsize,\n> @@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n>  }\n>  #endif\n>  \n> +static void update_pktline_state(struct rpc_state *rpc,\n> +\t\t\t\t const char *buf, size_t len)\n> +{\n> +#define READ_ONE_HEX(shift) do { \\\n> +\tint val = hexval(buf[0]); \\\n> +\tif (val < 0) { \\\n> +\t\twarning(\"error on %d\", *buf); \\\n> +\t\trpc->pktline_state = RPC_PKTLINE_ERROR; \\\n> +\t\treturn; \\\n> +\t} \\\n> +\trpc->pktline_len |= val << shift; \\\n> +\tbuf++; \\\n> +\tlen--; \\\n> +} while(0)\n> +\n> +\twhile (len > 0) {\n> +\t\tswitch (rpc->pktline_state) {\n> +\t\tcase RPC_PKTLINE_ERROR:\n> +\t\t\t/* previous error; there is no recovery */\n> +\t\t\treturn;\n> +\n> +\t\t/* We can start a new pktline at any of these states */\n> +\t\tcase RPC_PKTLINE_INITIAL:\n> +\t\tcase RPC_PKTLINE_FLUSH:\n> +\t\tcase RPC_PKTLINE_END_OF_PACKET:\n> +\t\t\trpc->pktline_len = 0;\n> +\t\t\tREAD_ONE_HEX(12);\n> +\t\t\trpc->pktline_state = RPC_PKTLINE_1;\n> +\t\t\tbreak;\n> +\n> +\t\tcase RPC_PKTLINE_1:\n> +\t\t\tREAD_ONE_HEX(8);\n> +\t\t\trpc->pktline_state = RPC_PKTLINE_2;\n> +\t\t\tbreak;\n> +\n> +\t\tcase RPC_PKTLINE_2:\n> +\t\t\tREAD_ONE_HEX(4);\n> +\t\t\trpc->pktline_state = RPC_PKTLINE_3;\n> +\t\t\tbreak;\n> +\n> +\t\tcase RPC_PKTLINE_3:\n> +\t\t\tREAD_ONE_HEX(0);\n> +\t\t\tif (rpc->pktline_len) {\n> +\t\t\t\trpc->pktline_state = RPC_PKTLINE_DATA;\n> +\t\t\t\trpc->pktline_len -= 4;\n> +\t\t\t} else\n> +\t\t\t\trpc->pktline_state = RPC_PKTLINE_FLUSH;\n> +\t\t\tbreak;\n> +\n> +\t\tcase RPC_PKTLINE_DATA:\n> +\t\t\tif (len < rpc->pktline_len) {\n> +\t\t\t\trpc->pktline_len -= len;\n> +\t\t\t\tlen = 0;\n> +\t\t\t} else {\n> +\t\t\t\tbuf += rpc->pktline_len;\n> +\t\t\t\tlen -= rpc->pktline_len;\n> +\t\t\t\trpc->pktline_len = 0;\n> +\t\t\t\trpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +#undef READ_ONE_HEX\n> +}\n> +\n>  static size_t rpc_in(char *ptr, size_t eltsize,\n>  \t\tsize_t nmemb, void *buffer_)\n>  {\n>  \tsize_t size = eltsize * nmemb;\n>  \tstruct rpc_state *rpc = buffer_;\n> +\tupdate_pktline_state(rpc, ptr, size);\n>  \twrite_or_die(rpc->in, ptr, size);\n>  \treturn size;\n>  }\n> @@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n>  \n> +\trpc->pktline_state = RPC_PKTLINE_INITIAL;\n> +\n>  \terr = run_slot(slot, NULL);\n>  \tif (err == HTTP_REAUTH && !large_request) {\n>  \t\tcredential_fill(&http_auth);\n> @@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tif (err != HTTP_OK)\n>  \t\terr = -1;\n>  \n> +\tif (rpc->pktline_state != RPC_PKTLINE_FLUSH) {\n> +\t\terror(\"invalid or truncated response from http server\");\n> +\t\terr = -1;\n> +\t}\n> +\n>  \tcurl_slist_free_all(headers);\n>  \tfree(gzip_body);\n>  \treturn err;\n"},{"id":"305922","messageId":"20161114213337.wipubwz5k22foyva@sigill.intra.peff.net","threadId":"44449","inReplyTo":"xmqqpolxwyh6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-14T21:33:37Z","receivedAt":"2016-11-14T21:33:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2016 at 01:19:49PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So something like this. It turned out to be a lot uglier than I had\n> > hoped because we get fed the data from curl in odd-sized chunks, so we\n> > need a state machine.\n> \n> It is unfortunate that we have to snoop the protocol like this to\n> infer an error, but I do not think we can do better than that\n> approach.  FWIW, I did not find the logic in update_pktline_state()\n> you wrote ugly at all.\n> \n> Having to assume that the end of each round from the other end must\n> be a FLUSH does feel somewhat ugly and brittle, though.\n\nYeah, I agree. The other option is to signal to fetch-pack that we saw\nEOF on the server request, and then we do not have to snoop; it knows\nwhere in the protocol state it is at.\n\nBut doing that is a little tricky. We could send our own flush packet,\nbut that doesn't quite work. When fetch-pack sees a flush it does not\nknow if it is the server's flush and it should wait for our flush, or if\nthe server hung up prematurely and the flush is from us. :)\n\nThe \"elegant\" solution is to just wrap the server's data in another set\nof pktlines. So the server flush becomes \"00080000\" (8 bytes including\nour pktline, and then 4 bytes for the flush packet). But wrapping each\npktline gets a little hairy. What happens when the server sends us a\npktline of LARGE_PACKET_MAX? We can't wrap that without sending our own\npktline that's LARGE_PACKET_MAX+4.\n\nOne solution is that pktlines 0001-0003 are unused and impossible for\ndata. So we could send the server pktlines as-is, and use pktline 0001\nas our special signal for \"end of http request\". A server shouldn't ever\nsend that, and if they did (perhaps to try something malicious), it\nwould just cause fetch-pack to think they prematurely ended the\nconversation.\n\nHmm. I suppose that doesn't quite work, though. One of the problems is\nthat the server sends a partial pktline, in which case our special flush\npacket would get gobbled up as data for that broken pktline.\n\nSo you really do need framing or some other out-of-band communication,\nor resolve yourself to snooping in remote-curl.\n\n-Peff\n"},{"id":"305925","messageId":"a57cc9c4a0a840baab5b8123fac9388b@exmbdft7.ad.twosigma.com","threadId":"44449","inReplyTo":"20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net","subject":"RE: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-14T23:25:30Z","receivedAt":"2016-11-14T23:25:36Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Monday, November 14, 2016 2:41 PM\n> To: David Turner\n> Cc: git@vger.kernel.org; spearce@spearce.org\n> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any\n> output\n> \n> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:\n> \n> >   2. Have remote-curl understand enough of the protocol that it can\n> >      abort rather than hang.\n> >\n> >      I think that's effectively the approach of your patch, but for one\n> >      specific case. But could we, for example, make sure that everything\n> >      we proxy is a complete set of pktlines and ends with a flush? And\n> >      if not, then we hang up on fetch-pack.\n> >\n> >      I _think_ that would work, because even the pack is always encased\n> >      in pktlines for smart-http.\n> \n> So something like this. It turned out to be a lot uglier than I had hoped\n> because we get fed the data from curl in odd-sized chunks, so we need a\n> state machine.\n> \n> But it does seem to work. At least it doesn't seem to break anything in\n> the test suite, and it fixes the new tests you added. I'd worry that\n> there's some obscure case where the response isn't packetized in the same\n> way.\n\nOverall, this looks good to me.  The state machine is pretty clean. I think I would have used a tiny buffer for the length field, and then I would have regretted it.  Your way looks nicer than my unwritten patch would have looked.\n\n> +#define READ_ONE_HEX(shift) do { \\\n> +\tint val = hexval(buf[0]); \\\n> +\tif (val < 0) { \\\n> +\t\twarning(\"error on %d\", *buf); \\\n> +\t\trpc->pktline_state = RPC_PKTLINE_ERROR; \\\n> +\t\treturn; \\\n> +\t} \\\n> +\trpc->pktline_len |= val << shift; \\\n\nNit: parenthesize shift here, since it is a parameter to a macro.\n\n"},{"id":"305926","messageId":"20161114234847.2nexsgedpg7zvrr5@sigill.intra.peff.net","threadId":"44449","inReplyTo":"a57cc9c4a0a840baab5b8123fac9388b@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-14T23:48:47Z","receivedAt":"2016-11-14T23:48:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2016 at 11:25:30PM +0000, David Turner wrote:\n\n> > But it does seem to work. At least it doesn't seem to break anything\n> > in the test suite, and it fixes the new tests you added. I'd worry\n> > that there's some obscure case where the response isn't packetized\n> > in the same way.\n> \n> Overall, this looks good to me.  The state machine is pretty clean. I\n> think I would have used a tiny buffer for the length field, and then I\n> would have regretted it.  Your way looks nicer than my unwritten patch\n> would have looked.\n\nHeh, I started it that way but you end up dealing with the same states\n(they're just implicit in your \"how big is my temp buffer\" field).\n\n> > +#define READ_ONE_HEX(shift) do { \\\n> > +\tint val = hexval(buf[0]); \\\n> > +\tif (val < 0) { \\\n> > +\t\twarning(\"error on %d\", *buf); \\\n> > +\t\trpc->pktline_state = RPC_PKTLINE_ERROR; \\\n> > +\t\treturn; \\\n> > +\t} \\\n> > +\trpc->pktline_len |= val << shift; \\\n> \n> Nit: parenthesize shift here, since it is a parameter to a macro.\n\nYeah, I'm often a bit slack on these one-off inside-a-function macros.\nBut it does not hurt to to be careful.\n\nI'll make that change and then try to wrap this up with a commit\nmessage. I plan to steal your tests, if that's OK.\n\n-Peff\n"},{"id":"305927","messageId":"20161115004426.unheihlmftlw6ex7@sigill.intra.peff.net","threadId":"44449","inReplyTo":"20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-15T00:44:26Z","receivedAt":"2016-11-15T00:44:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2016 at 02:40:49PM -0500, Jeff King wrote:\n\n> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:\n> \n> >   2. Have remote-curl understand enough of the protocol that it can\n> >      abort rather than hang.\n> > \n> >      I think that's effectively the approach of your patch, but for one\n> >      specific case. But could we, for example, make sure that everything\n> >      we proxy is a complete set of pktlines and ends with a flush? And\n> >      if not, then we hang up on fetch-pack.\n> > \n> >      I _think_ that would work, because even the pack is always encased\n> >      in pktlines for smart-http.\n> \n> So something like this. It turned out to be a lot uglier than I had\n> hoped because we get fed the data from curl in odd-sized chunks, so we\n> need a state machine.\n> \n> But it does seem to work. At least it doesn't seem to break anything in\n> the test suite, and it fixes the new tests you added. I'd worry that\n> there's some obscure case where the response isn't packetized in the\n> same way.\n\nActually, I take it back. I think it works for a single round of ref\nnegotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to\nfail t5551.\n\nI think I've probably made a mis-assumption on exactly when in the HTTP\nprotocol we will see a flush packet (and perhaps that is a sign that\nthis protocol-snooping approach is not a good one).\n\nI don't have time to dig more on this tonight, and I'll be traveling for\nthe rest of the week. So if anybody is interested, please feel free to\ndig into it.\n\n-Peff\n"},{"id":"305928","messageId":"xmqqa8d1v9lo.fsf@gitster.mtv.corp.google.com","threadId":"44449","inReplyTo":"20161115004426.unheihlmftlw6ex7@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-15T01:02:27Z","receivedAt":"2016-11-15T01:02:35Z","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> Actually, I take it back. I think it works for a single round of ref\n> negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to\n> fail t5551.\n>\n> I think I've probably made a mis-assumption on exactly when in the HTTP\n> protocol we will see a flush packet (and perhaps that is a sign that\n> this protocol-snooping approach is not a good one).\n\nHmph.  I think I tried David's original under GIT_TEST_LONG and saw\nit got stuck; could be the same issue, I guess.\n"},{"id":"305934","messageId":"20161115024043.iuescpzofmrwvfbf@sigill.intra.peff.net","threadId":"44449","inReplyTo":"20161115004426.unheihlmftlw6ex7@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-15T02:40:44Z","receivedAt":"2016-11-15T02:40:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2016 at 07:44:26PM -0500, Jeff King wrote:\n\n> > But it does seem to work. At least it doesn't seem to break anything in\n> > the test suite, and it fixes the new tests you added. I'd worry that\n> > there's some obscure case where the response isn't packetized in the\n> > same way.\n> \n> Actually, I take it back. I think it works for a single round of ref\n> negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to\n> fail t5551.\n> \n> I think I've probably made a mis-assumption on exactly when in the HTTP\n> protocol we will see a flush packet (and perhaps that is a sign that\n> this protocol-snooping approach is not a good one).\n\nYep, that is it. The server may end with an ACK or a NAK, not a flush\npacket. So going this route really does mean teaching remote-curl a lot\nmore about the protocol, which is pretty nasty. I think trying to\nsomehow signal the end-of-response to fetch-pack would be a more\nmaintainable approach.\n\n-Peff\n"},{"id":"305935","messageId":"20161115035844.e6ehuy7uigqinbnv@sigill.intra.peff.net","threadId":"44449","inReplyTo":"xmqqa8d1v9lo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-15T03:58:44Z","receivedAt":"2016-11-15T03:58:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2016 at 05:02:27PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Actually, I take it back. I think it works for a single round of ref\n> > negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to\n> > fail t5551.\n> >\n> > I think I've probably made a mis-assumption on exactly when in the HTTP\n> > protocol we will see a flush packet (and perhaps that is a sign that\n> > this protocol-snooping approach is not a good one).\n> \n> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw\n> it got stuck; could be the same issue, I guess.\n\nIt works OK here. I think it is just that the test is really slow (by\ndesign).\n\n-Peff\n"},{"id":"305959","messageId":"a651930afcc542fb80161a7a399aa0ce@exmbdft7.ad.twosigma.com","threadId":"44449","inReplyTo":"20161114234847.2nexsgedpg7zvrr5@sigill.intra.peff.net","subject":"RE: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-15T15:45:19Z","receivedAt":"2016-11-15T15:45:35Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n...\n> I'll make that change and then try to wrap this up with a commit message.\n> I plan to steal your tests, if that's OK.\n\nPlease do!\n\n"},{"id":"305968","messageId":"xmqqzil0tza6.fsf@gitster.mtv.corp.google.com","threadId":"44449","inReplyTo":"20161115035844.e6ehuy7uigqinbnv@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-15T17:42:57Z","receivedAt":"2016-11-15T17:43:23Z","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 Mon, Nov 14, 2016 at 05:02:27PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > Actually, I take it back. I think it works for a single round of ref\n>> > negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to\n>> > fail t5551.\n>> >\n>> > I think I've probably made a mis-assumption on exactly when in the HTTP\n>> > protocol we will see a flush packet (and perhaps that is a sign that\n>> > this protocol-snooping approach is not a good one).\n>> \n>> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw\n>> it got stuck; could be the same issue, I guess.\n>\n> It works OK here. I think it is just that the test is really slow (by\n> design).\n\nYeah, I think what I recalled was my old attempt to run the\nfollow-up \"any SHA-1\" patch without this one.\n"},{"id":"306143","messageId":"20161118170147.g7nbkxpyihwkk6fw@sigill.intra.peff.net","threadId":"44449","inReplyTo":"xmqqzil0tza6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-18T17:01:47Z","receivedAt":"2016-11-18T17:01:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 15, 2016 at 09:42:57AM -0800, Junio C Hamano wrote:\n\n> >> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw\n> >> it got stuck; could be the same issue, I guess.\n> >\n> > It works OK here. I think it is just that the test is really slow (by\n> > design).\n> \n> Yeah, I think what I recalled was my old attempt to run the\n> follow-up \"any SHA-1\" patch without this one.\n\nRight, that makes sense.\n\nSo I don't feel like we have a good patch for the general case yet, and\nI'm probably not going to get around to implementing it anytime soon. So\nI'd suggest taking David's original patch (to punt when the response is\nempty) in the meantime.\n\nIt doesn't fix all cases, but if fixes _a_ case, and probably one of the\nmost likely one in practice. I don't think it can cause any regressions.\nIt's a \"snooping\" solution like mine, but it makes many fewer\nassumptions about the protocol. We know that an empty response cannot\npossibly advance fetch-pack further because we won't have sent it any\nbytes. :)\n\nI do think the commit message could be improved based on the discussion\nhere, though (at the very least to describe the nature of the deadlock,\nand that we are choosing only one of the possible solutions, and why).\n\n-Peff\n"},{"id":"306144","messageId":"764305554fa74779ad5fb956aa2b658a@EXMBNJE7.ad.twosigma.com","threadId":"44449","inReplyTo":"20161118170147.g7nbkxpyihwkk6fw@sigill.intra.peff.net","subject":"RE: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-18T17:04:59Z","receivedAt":"2016-11-18T17:05:06Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Friday, November 18, 2016 12:02 PM\n> To: Junio C Hamano\n> Cc: David Turner; git@vger.kernel.org; spearce@spearce.org\n> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any\n> output\n> \n> On Tue, Nov 15, 2016 at 09:42:57AM -0800, Junio C Hamano wrote:\n> \n> > >> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw\n> > >> it got stuck; could be the same issue, I guess.\n> > >\n> > > It works OK here. I think it is just that the test is really slow\n> > > (by design).\n> >\n> > Yeah, I think what I recalled was my old attempt to run the follow-up\n> > \"any SHA-1\" patch without this one.\n> \n> Right, that makes sense.\n> \n> So I don't feel like we have a good patch for the general case yet, and\n> I'm probably not going to get around to implementing it anytime soon. \n\nI'm confused -- it sounds like your patch actually does work (that is, that Junio's failure was not caused by your patch but by the absence of our patches). And your patch handles more cases than mine.  So we should probably use it instead of mine.\n"},{"id":"306145","messageId":"20161118170855.22p3vhcocdxgmpy4@sigill.intra.peff.net","threadId":"44449","inReplyTo":"764305554fa74779ad5fb956aa2b658a@EXMBNJE7.ad.twosigma.com","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-18T17:08:55Z","receivedAt":"2016-11-18T17:09:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2016 at 05:04:59PM +0000, David Turner wrote:\n\n> > So I don't feel like we have a good patch for the general case yet,\n> > and I'm probably not going to get around to implementing it anytime\n> > soon. \n> \n> I'm confused -- it sounds like your patch actually does work (that is,\n> that Junio's failure was not caused by your patch but by the absence\n> of our patches). And your patch handles more cases than mine.  So we\n> should probably use it instead of mine.\n\nNo, mine passes the vanilla test suite, but fails with GIT_TEST_LONG.\nIf the want/have negotiation takes multiple rounds, the intermediate\nrounds don't end on a flush packet, and my patch causes remote-curl to\ncomplain that the response was truncated.\n\nI think you could fix it by teaching remote-curl that the final packet\nmust be a flush _or_ contain an ACK/NAK, but I didn't try it. That's\ngetting a bit invasive and brittle.\n\n-Peff\n"},{"id":"306147","messageId":"3404f9d7dd6c44bc9090cc1b556bdb9b@EXMBNJE7.ad.twosigma.com","threadId":"44449","inReplyTo":"20161118170855.22p3vhcocdxgmpy4@sigill.intra.peff.net","subject":"RE: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-18T17:48:48Z","receivedAt":"2016-11-18T17:49:09Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Friday, November 18, 2016 12:09 PM\n> To: David Turner\n> Cc: Junio C Hamano; git@vger.kernel.org; spearce@spearce.org\n> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any\n> output\n> \n> On Fri, Nov 18, 2016 at 05:04:59PM +0000, David Turner wrote:\n> \n> > > So I don't feel like we have a good patch for the general case yet,\n> > > and I'm probably not going to get around to implementing it anytime\n> > > soon.\n> >\n> > I'm confused -- it sounds like your patch actually does work (that is,\n> > that Junio's failure was not caused by your patch but by the absence\n> > of our patches). And your patch handles more cases than mine.  So we\n> > should probably use it instead of mine.\n> \n> No, mine passes the vanilla test suite, but fails with GIT_TEST_LONG.\n> If the want/have negotiation takes multiple rounds, the intermediate\n> rounds don't end on a flush packet, and my patch causes remote-curl to\n> complain that the response was truncated.\n> \n> I think you could fix it by teaching remote-curl that the final packet\n> must be a flush _or_ contain an ACK/NAK, but I didn't try it. That's\n> getting a bit invasive and brittle.\n\nOK, I'll re-roll mine with a better message.\n"},{"id":"306151","messageId":"xmqqlgwgmylk.fsf@gitster.mtv.corp.google.com","threadId":"44449","inReplyTo":"20161118170147.g7nbkxpyihwkk6fw@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: don't hang when a server dies before any output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-18T18:28:39Z","receivedAt":"2016-11-18T18:28:47Z","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> So I don't feel like we have a good patch for the general case yet, and\n> I'm probably not going to get around to implementing it anytime soon. So\n> I'd suggest taking David's original patch (to punt when the response is\n> empty) in the meantime.\n\nYup, we are on the same page; the above matches what I wrote in the\ndraft of the next issue of What's cooking report last night.\n\n> I do think the commit message could be improved based on the discussion\n> here, though (at the very least to describe the nature of the deadlock,\n> and that we are choosing only one of the possible solutions, and why).\n\nThanks.  That sounds sensible.\n\n\n"}]}