{"thread":{"id":"44510","subject":"[PATCH v3] remote-curl: don't hang when a server dies before any output","startedAt":"2016-11-18T20:31:01Z","lastAt":"2016-11-18T21:05:48Z","messageCount":2,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"306164","messageId":"1479501049-15458-1-git-send-email-dturner@twosigma.com","threadId":"44510","inReplyTo":null,"subject":"[PATCH v3] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-11-18T20:30:49Z","receivedAt":"2016-11-18T20:31:01Z","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. In this case, the server dies before sending any data.\nPrior to this patch, fetch-pack would wait for data from the server,\nand remote-curl would wait for fetch-pack, causing a deadlock.\n\nDespite this patch, there is other possible malformed input that could\ncause the same deadlock (e.g. a half-finished pktline, or a pktline but\nno trailing flush). There are a few possible solutions to this:\n\n1. Allowing remote-curl to tell fetch-pack about the EOF (so that\nfetch-pack could know that no more data is coming until it says\nsomething else). This is tricky because an out-of-band signal would\nbe required, or the http response would have to be re-framed inside\nanother layer of pkt-line or something.\n\n2. Make remote-curl understand some of the protocol. It turns out\nthat in addition to understanding pkt-line, it would need to watch for\nack/nak. This is somewhat fragile, as information about the protocol\nwould end up in two places. Also, pkt-lines which are already at the\nlength limit would need special handling.\n\nBoth of these solutions would require a fair amount of work, whereas\nthis hack is easy and solves at least some of the problem.\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 \nSigned-off-by: Junio C Hamano \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":"306165","messageId":"xmqqd1hsmrbw.fsf@gitster.mtv.corp.google.com","threadId":"44510","inReplyTo":"1479501049-15458-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v3] remote-curl: don't hang when a server dies before any output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-18T21:05:38Z","receivedAt":"2016-11-18T21:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\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>\n> One case where this happens is when attempting to fetch a dangling\n> object by SHA. In this case, the server dies before sending any data.\n> Prior to this patch, fetch-pack would wait for data from the server,\n> and remote-curl would wait for fetch-pack, causing a deadlock.\n> ...\n> Still to do: it would be good to give a better error message\n> than \"fatal: The remote end hung up unexpectedly\".\n>\n> Signed-off-by: David Turner \n> Signed-off-by: Junio C Hamano \n> ---\n\nThanks.\n"}]}