{"thread":{"id":"44509","subject":"[PATCH v2] remote-curl: don't hang when a server dies before any output","startedAt":"2016-11-18T17:59:03Z","lastAt":"2016-11-18T20:05:29Z","messageCount":2,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"306148","messageId":"1479491919-12592-1-git-send-email-dturner@twosigma.com","threadId":"44509","inReplyTo":null,"subject":"[PATCH v2] remote-curl: don't hang when a server dies before any output","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-11-18T17:58:39Z","receivedAt":"2016-11-18T17:59:03Z","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\nPrior to this patch, fetch-pack would wait for more data from the\nserver, and 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 <dturner@twosigma.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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":"306161","messageId":"xmqqh974mu56.fsf@gitster.mtv.corp.google.com","threadId":"44509","inReplyTo":"1479491919-12592-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH v2] remote-curl: don't hang when a server dies before any output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-18T20:04:53Z","receivedAt":"2016-11-18T20:05:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> 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.\n>\n> Prior to this patch, fetch-pack would wait for more data from the\n> server, and remote-curl would wait for fetch-pack, causing a deadlock.\n>\n> Despite this patch, there is other possible malformed input that could\n> cause the same deadlock (e.g. a half-finished pktline, or a pktline but\n> no trailing flush).  There are a few possible solutions to this:\n>\n> 1. Allowing remote-curl to tell fetch-pack about the EOF (so that\n> fetch-pack could know that no more data is coming until it says\n> something else).  This is tricky because an out-of-band signal would\n> be required, or the http response would have to be re-framed inside\n> another layer of pkt-line or something.\n>\n> 2. Make remote-curl understand some of the protocol.  It turns out\n> that in addition to understanding pkt-line, it would need to watch for\n> ack/nak.  This is somewhat fragile, as information about the protocol\n> would end up in two places.  Also, pkt-lines which are already at the\n> length limit would need special handling.\n>\n> Both of these solutions would require a fair amount of work, whereas\n> this hack is easy and solves at least some of the problem.\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\nIt seems to me that there is some gap/leap in the description\nbetween the second and third paragraph above (i.e. the client asks\nfor an object, the server tries to see if the object is reachable\nfrom the refs, and then what?  who waits for more data without\nasking?), but other than that, including the future direction, the\nproposed log message is very nicely described.\n\nThanks, will queue.\n"}]}