[PATCH] remote-curl: don't hang when a server dies before any output

[PATCH] remote-curl: don't hang when a server dies before any output

[David Turner]

In the event that a HTTP server closes the connection after giving a
200 but before giving any packets, we don't want to hang forever
waiting for a response that will never come.  Instead, we should die
immediately.

One case where this happens is when attempting to fetch a dangling
object by SHA.

Still to do: it would be good to give a better error message
than "fatal: The remote end hung up unexpectedly".

Signed-off-by: David Turner <dturner@twosigma.com>
---

Note: if you run t5551 before applying the code patch, the second new
test will hang forever.

FWIW, I also saw this kind of hang at Twitter from time to time, but I
was never able to reliably reproduce it there, and thus never able to
fix it.  I suspect that a bad load balancer might have been killing
connections just after the headers, but this is pure speculation.

I am sorry that this patch does not provide a more useful error
message.  For us, it is important to fix the hangs (so that we can
retry on another server, for instance), but less important to be
user-friendly (since we were only seeing these with automated
processes).  I hope that someone who actually understands the http
code, and has some time, could help improve this aspect of the code.

If not, then at least we won't have inexplicable hangs.

 remote-curl.c               |  8 ++++++++
 t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/remote-curl.c b/remote-curl.c
index f14c41f..ee44236 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -400,6 +400,7 @@ struct rpc_state {
 	size_t pos;
 	int in;
 	int out;
+	int any_written;
 	struct strbuf result;
 	unsigned gzip_request : 1;
 	unsigned initial_buffer : 1;
@@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 {
 	size_t size = eltsize * nmemb;
 	struct rpc_state *rpc = buffer_;
+	if (size)
+		rpc->any_written = 1;
 	write_or_die(rpc->in, ptr, size);
 	return size;
 }
@@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
+
+	rpc->any_written = 0;
 	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request) {
 		credential_fill(&http_auth);
@@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
 	if (err != HTTP_OK)
 		err = -1;
 
+	if (!rpc->any_written)
+		err = -1;
+
 	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b27..43665ab 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
 	test_line_count = 2 posts
 '
 
+test_expect_success 'test allowreachablesha1inwant' '
+	test_when_finished "rm -rf test_reachable.git" &&
+	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+	git init --bare test_reachable.git &&
+	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+	git -C test_reachable.git fetch origin "$master_sha"
+'
+
+test_expect_success 'test allowreachablesha1inwant with unreachable' '
+	test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
+
+	#create unreachable sha
+	echo content >file2 &&
+	git add file2 &&
+	git commit -m two &&
+	git push public HEAD:refs/heads/doomed &&
+	git push public :refs/heads/doomed &&
+
+	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+	git init --bare test_reachable.git &&
+	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+'
+
 test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	(
 		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
-- 
2.8.0.rc4.22.g8ae061a

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Wed, Nov 09, 2016 at 05:18:30PM -0500, David Turner wrote:

> In the event that a HTTP server closes the connection after giving a
> 200 but before giving any packets, we don't want to hang forever
> waiting for a response that will never come.  Instead, we should die
> immediately.

I agree we don't want to hang forever, but this leaves open the
question: what is hanging?

My guess is that fetch-pack is waiting for more data from the server,
and remote-curl is waiting for fetch-pack to tell us what to send for
the next request. Neither will make forward progress because they are
effectively waiting on each other.

Which means this is likely a special case of malformed input from the
server. A server which likewise sends a partial response could end up in
the same deadlock, I would think (e.g., a half-finished pktline, or a
pktline but no trailing flush).

That doesn't make it wrong to fix this specific case (especially if it's
a common one), but I wonder if we could do better.

The root of the issue is that only fetch-pack understands the protocol,
and remote-curl is blindly proxying the data. But only remote-curl knows
that the HTTP request has ended, and it doesn't relay that information
to fetch-pack. So I can think of two solutions:

  1. Some way of remote-curl communicating the EOF to fetch-pack. It
     can't just close the descriptor, since we need to pass more data
     over it for the followup requests. You'd need something
     out-of-band, or to frame the HTTP data inside another layer of
     pktlines, both of which are kind of gross.

  2. Have remote-curl understand enough of the protocol that it can
     abort rather than hang.

     I think that's effectively the approach of your patch, but for one
     specific case. But could we, for example, make sure that everything
     we proxy is a complete set of pktlines and ends with a flush? And
     if not, then we hang up on fetch-pack.

     I _think_ that would work, because even the pack is always encased
     in pktlines for smart-http.

> @@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
>  	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>  
> +
> +	rpc->any_written = 0;

Extra blank line here?

> @@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
>  	if (err != HTTP_OK)
>  		err = -1;
>  
> +	if (!rpc->any_written)
> +		err = -1;
> +

I wondered if there were any cases where it was normal for the server to
return zero bytes. Possibly the ref advertisement is one, but this is
_just_ handling post_rpc(), so that's OK. And I think by definition
every response has to at least return a flush packet, or we would make
no forward progress (i.e., the exact case you are dealing with here).

-Peff

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:

>   2. Have remote-curl understand enough of the protocol that it can
>      abort rather than hang.
> 
>      I think that's effectively the approach of your patch, but for one
>      specific case. But could we, for example, make sure that everything
>      we proxy is a complete set of pktlines and ends with a flush? And
>      if not, then we hang up on fetch-pack.
> 
>      I _think_ that would work, because even the pack is always encased
>      in pktlines for smart-http.

So something like this. It turned out to be a lot uglier than I had
hoped because we get fed the data from curl in odd-sized chunks, so we
need a state machine.

But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the
same way.

---
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c..605357d77 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -403,6 +403,18 @@ struct rpc_state {
 	struct strbuf result;
 	unsigned gzip_request : 1;
 	unsigned initial_buffer : 1;
+
+	enum {
+		RPC_PKTLINE_ERROR, /* bogus hex chars in length */
+		RPC_PKTLINE_INITIAL, /* no packets received yet */
+		RPC_PKTLINE_1, /* got one hex char */
+		RPC_PKTLINE_2, /* got two hex chars */
+		RPC_PKTLINE_3, /* got three hex chars */
+		RPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */
+		RPC_PKTLINE_END_OF_PACKET, /* last packet completed */
+		RPC_PKTLINE_FLUSH, /* last packet was flush */
+	} pktline_state;
+	size_t pktline_len;
 };
 
 static size_t rpc_out(void *ptr, size_t eltsize,
@@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+static void update_pktline_state(struct rpc_state *rpc,
+				 const char *buf, size_t len)
+{
+#define READ_ONE_HEX(shift) do { \
+	int val = hexval(buf[0]); \
+	if (val < 0) { \
+		warning("error on %d", *buf); \
+		rpc->pktline_state = RPC_PKTLINE_ERROR; \
+		return; \
+	} \
+	rpc->pktline_len |= val << shift; \
+	buf++; \
+	len--; \
+} while(0)
+
+	while (len > 0) {
+		switch (rpc->pktline_state) {
+		case RPC_PKTLINE_ERROR:
+			/* previous error; there is no recovery */
+			return;
+
+		/* We can start a new pktline at any of these states */
+		case RPC_PKTLINE_INITIAL:
+		case RPC_PKTLINE_FLUSH:
+		case RPC_PKTLINE_END_OF_PACKET:
+			rpc->pktline_len = 0;
+			READ_ONE_HEX(12);
+			rpc->pktline_state = RPC_PKTLINE_1;
+			break;
+
+		case RPC_PKTLINE_1:
+			READ_ONE_HEX(8);
+			rpc->pktline_state = RPC_PKTLINE_2;
+			break;
+
+		case RPC_PKTLINE_2:
+			READ_ONE_HEX(4);
+			rpc->pktline_state = RPC_PKTLINE_3;
+			break;
+
+		case RPC_PKTLINE_3:
+			READ_ONE_HEX(0);
+			if (rpc->pktline_len) {
+				rpc->pktline_state = RPC_PKTLINE_DATA;
+				rpc->pktline_len -= 4;
+			} else
+				rpc->pktline_state = RPC_PKTLINE_FLUSH;
+			break;
+
+		case RPC_PKTLINE_DATA:
+			if (len < rpc->pktline_len) {
+				rpc->pktline_len -= len;
+				len = 0;
+			} else {
+				buf += rpc->pktline_len;
+				len -= rpc->pktline_len;
+				rpc->pktline_len = 0;
+				rpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;
+			}
+			break;
+		}
+	}
+#undef READ_ONE_HEX
+}
+
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
 	struct rpc_state *rpc = buffer_;
+	update_pktline_state(rpc, ptr, size);
 	write_or_die(rpc->in, ptr, size);
 	return size;
 }
@@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
+	rpc->pktline_state = RPC_PKTLINE_INITIAL;
+
 	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request) {
 		credential_fill(&http_auth);
@@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)
 	if (err != HTTP_OK)
 		err = -1;
 
+	if (rpc->pktline_state != RPC_PKTLINE_FLUSH) {
+		error("invalid or truncated response from http server");
+		err = -1;
+	}
+
 	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So something like this. It turned out to be a lot uglier than I had
> hoped because we get fed the data from curl in odd-sized chunks, so we
> need a state machine.

It is unfortunate that we have to snoop the protocol like this to
infer an error, but I do not think we can do better than that
approach.  FWIW, I did not find the logic in update_pktline_state()
you wrote ugly at all.

Having to assume that the end of each round from the other end must
be a FLUSH does feel somewhat ugly and brittle, though.

> diff --git a/remote-curl.c b/remote-curl.c
> index f14c41f4c..605357d77 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -403,6 +403,18 @@ struct rpc_state {
>  	struct strbuf result;
>  	unsigned gzip_request : 1;
>  	unsigned initial_buffer : 1;
> +
> +	enum {
> +		RPC_PKTLINE_ERROR, /* bogus hex chars in length */
> +		RPC_PKTLINE_INITIAL, /* no packets received yet */
> +		RPC_PKTLINE_1, /* got one hex char */
> +		RPC_PKTLINE_2, /* got two hex chars */
> +		RPC_PKTLINE_3, /* got three hex chars */
> +		RPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */
> +		RPC_PKTLINE_END_OF_PACKET, /* last packet completed */
> +		RPC_PKTLINE_FLUSH, /* last packet was flush */
> +	} pktline_state;
> +	size_t pktline_len;
>  };
>  
>  static size_t rpc_out(void *ptr, size_t eltsize,
> @@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
>  }
>  #endif
>  
> +static void update_pktline_state(struct rpc_state *rpc,
> +				 const char *buf, size_t len)
> +{
> +#define READ_ONE_HEX(shift) do { \
> +	int val = hexval(buf[0]); \
> +	if (val < 0) { \
> +		warning("error on %d", *buf); \
> +		rpc->pktline_state = RPC_PKTLINE_ERROR; \
> +		return; \
> +	} \
> +	rpc->pktline_len |= val << shift; \
> +	buf++; \
> +	len--; \
> +} while(0)
> +
> +	while (len > 0) {
> +		switch (rpc->pktline_state) {
> +		case RPC_PKTLINE_ERROR:
> +			/* previous error; there is no recovery */
> +			return;
> +
> +		/* We can start a new pktline at any of these states */
> +		case RPC_PKTLINE_INITIAL:
> +		case RPC_PKTLINE_FLUSH:
> +		case RPC_PKTLINE_END_OF_PACKET:
> +			rpc->pktline_len = 0;
> +			READ_ONE_HEX(12);
> +			rpc->pktline_state = RPC_PKTLINE_1;
> +			break;
> +
> +		case RPC_PKTLINE_1:
> +			READ_ONE_HEX(8);
> +			rpc->pktline_state = RPC_PKTLINE_2;
> +			break;
> +
> +		case RPC_PKTLINE_2:
> +			READ_ONE_HEX(4);
> +			rpc->pktline_state = RPC_PKTLINE_3;
> +			break;
> +
> +		case RPC_PKTLINE_3:
> +			READ_ONE_HEX(0);
> +			if (rpc->pktline_len) {
> +				rpc->pktline_state = RPC_PKTLINE_DATA;
> +				rpc->pktline_len -= 4;
> +			} else
> +				rpc->pktline_state = RPC_PKTLINE_FLUSH;
> +			break;
> +
> +		case RPC_PKTLINE_DATA:
> +			if (len < rpc->pktline_len) {
> +				rpc->pktline_len -= len;
> +				len = 0;
> +			} else {
> +				buf += rpc->pktline_len;
> +				len -= rpc->pktline_len;
> +				rpc->pktline_len = 0;
> +				rpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;
> +			}
> +			break;
> +		}
> +	}
> +#undef READ_ONE_HEX
> +}
> +
>  static size_t rpc_in(char *ptr, size_t eltsize,
>  		size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
>  	struct rpc_state *rpc = buffer_;
> +	update_pktline_state(rpc, ptr, size);
>  	write_or_die(rpc->in, ptr, size);
>  	return size;
>  }
> @@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
>  	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>  
> +	rpc->pktline_state = RPC_PKTLINE_INITIAL;
> +
>  	err = run_slot(slot, NULL);
>  	if (err == HTTP_REAUTH && !large_request) {
>  		credential_fill(&http_auth);
> @@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)
>  	if (err != HTTP_OK)
>  		err = -1;
>  
> +	if (rpc->pktline_state != RPC_PKTLINE_FLUSH) {
> +		error("invalid or truncated response from http server");
> +		err = -1;
> +	}
> +
>  	curl_slist_free_all(headers);
>  	free(gzip_body);
>  	return err;

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Mon, Nov 14, 2016 at 01:19:49PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So something like this. It turned out to be a lot uglier than I had
> > hoped because we get fed the data from curl in odd-sized chunks, so we
> > need a state machine.
> 
> It is unfortunate that we have to snoop the protocol like this to
> infer an error, but I do not think we can do better than that
> approach.  FWIW, I did not find the logic in update_pktline_state()
> you wrote ugly at all.
> 
> Having to assume that the end of each round from the other end must
> be a FLUSH does feel somewhat ugly and brittle, though.

Yeah, I agree. The other option is to signal to fetch-pack that we saw
EOF on the server request, and then we do not have to snoop; it knows
where in the protocol state it is at.

But doing that is a little tricky. We could send our own flush packet,
but that doesn't quite work. When fetch-pack sees a flush it does not
know if it is the server's flush and it should wait for our flush, or if
the server hung up prematurely and the flush is from us. :)

The "elegant" solution is to just wrap the server's data in another set
of pktlines. So the server flush becomes "00080000" (8 bytes including
our pktline, and then 4 bytes for the flush packet). But wrapping each
pktline gets a little hairy. What happens when the server sends us a
pktline of LARGE_PACKET_MAX? We can't wrap that without sending our own
pktline that's LARGE_PACKET_MAX+4.

One solution is that pktlines 0001-0003 are unused and impossible for
data. So we could send the server pktlines as-is, and use pktline 0001
as our special signal for "end of http request". A server shouldn't ever
send that, and if they did (perhaps to try something malicious), it
would just cause fetch-pack to think they prematurely ended the
conversation.

Hmm. I suppose that doesn't quite work, though. One of the problems is
that the server sends a partial pktline, in which case our special flush
packet would get gobbled up as data for that broken pktline.

So you really do need framing or some other out-of-band communication,
or resolve yourself to snooping in remote-curl.

-Peff

RE: [PATCH] remote-curl: don't hang when a server dies before any output

[David Turner]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, November 14, 2016 2:41 PM
> To: David Turner
> Cc: git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
> 
> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
> 
> >   2. Have remote-curl understand enough of the protocol that it can
> >      abort rather than hang.
> >
> >      I think that's effectively the approach of your patch, but for one
> >      specific case. But could we, for example, make sure that everything
> >      we proxy is a complete set of pktlines and ends with a flush? And
> >      if not, then we hang up on fetch-pack.
> >
> >      I _think_ that would work, because even the pack is always encased
> >      in pktlines for smart-http.
> 
> So something like this. It turned out to be a lot uglier than I had hoped
> because we get fed the data from curl in odd-sized chunks, so we need a
> state machine.
> 
> But it does seem to work. At least it doesn't seem to break anything in
> the test suite, and it fixes the new tests you added. I'd worry that
> there's some obscure case where the response isn't packetized in the same
> way.

Overall, 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.

> +#define READ_ONE_HEX(shift) do { \
> +	int val = hexval(buf[0]); \
> +	if (val < 0) { \
> +		warning("error on %d", *buf); \
> +		rpc->pktline_state = RPC_PKTLINE_ERROR; \
> +		return; \
> +	} \
> +	rpc->pktline_len |= val << shift; \

Nit: parenthesize shift here, since it is a parameter to a macro.

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Mon, Nov 14, 2016 at 11:25:30PM +0000, David Turner wrote:

> > But it does seem to work. At least it doesn't seem to break anything
> > in the test suite, and it fixes the new tests you added. I'd worry
> > that there's some obscure case where the response isn't packetized
> > in the same way.
> 
> Overall, 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.

Heh, I started it that way but you end up dealing with the same states
(they're just implicit in your "how big is my temp buffer" field).

> > +#define READ_ONE_HEX(shift) do { \
> > +	int val = hexval(buf[0]); \
> > +	if (val < 0) { \
> > +		warning("error on %d", *buf); \
> > +		rpc->pktline_state = RPC_PKTLINE_ERROR; \
> > +		return; \
> > +	} \
> > +	rpc->pktline_len |= val << shift; \
> 
> Nit: parenthesize shift here, since it is a parameter to a macro.

Yeah, I'm often a bit slack on these one-off inside-a-function macros.
But it does not hurt to to be careful.

I'll make that change and then try to wrap this up with a commit
message. I plan to steal your tests, if that's OK.

-Peff

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Mon, Nov 14, 2016 at 02:40:49PM -0500, Jeff King wrote:

> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
> 
> >   2. Have remote-curl understand enough of the protocol that it can
> >      abort rather than hang.
> > 
> >      I think that's effectively the approach of your patch, but for one
> >      specific case. But could we, for example, make sure that everything
> >      we proxy is a complete set of pktlines and ends with a flush? And
> >      if not, then we hang up on fetch-pack.
> > 
> >      I _think_ that would work, because even the pack is always encased
> >      in pktlines for smart-http.
> 
> So something like this. It turned out to be a lot uglier than I had
> hoped because we get fed the data from curl in odd-sized chunks, so we
> need a state machine.
> 
> But it does seem to work. At least it doesn't seem to break anything in
> the test suite, and it fixes the new tests you added. I'd worry that
> there's some obscure case where the response isn't packetized in the
> same way.

Actually, I take it back. I think it works for a single round of ref
negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
fail t5551.

I think I've probably made a mis-assumption on exactly when in the HTTP
protocol we will see a flush packet (and perhaps that is a sign that
this protocol-snooping approach is not a good one).

I don't have time to dig more on this tonight, and I'll be traveling for
the rest of the week. So if anybody is interested, please feel free to
dig into it.

-Peff

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Actually, I take it back. I think it works for a single round of ref
> negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
> fail t5551.
>
> I think I've probably made a mis-assumption on exactly when in the HTTP
> protocol we will see a flush packet (and perhaps that is a sign that
> this protocol-snooping approach is not a good one).

Hmph.  I think I tried David's original under GIT_TEST_LONG and saw
it got stuck; could be the same issue, I guess.

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Mon, Nov 14, 2016 at 07:44:26PM -0500, Jeff King wrote:

> > But it does seem to work. At least it doesn't seem to break anything in
> > the test suite, and it fixes the new tests you added. I'd worry that
> > there's some obscure case where the response isn't packetized in the
> > same way.
> 
> Actually, I take it back. I think it works for a single round of ref
> negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
> fail t5551.
> 
> I think I've probably made a mis-assumption on exactly when in the HTTP
> protocol we will see a flush packet (and perhaps that is a sign that
> this protocol-snooping approach is not a good one).

Yep, that is it. The server may end with an ACK or a NAK, not a flush
packet. So going this route really does mean teaching remote-curl a lot
more about the protocol, which is pretty nasty. I think trying to
somehow signal the end-of-response to fetch-pack would be a more
maintainable approach.

-Peff

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Mon, Nov 14, 2016 at 05:02:27PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Actually, I take it back. I think it works for a single round of ref
> > negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
> > fail t5551.
> >
> > I think I've probably made a mis-assumption on exactly when in the HTTP
> > protocol we will see a flush packet (and perhaps that is a sign that
> > this protocol-snooping approach is not a good one).
> 
> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw
> it got stuck; could be the same issue, I guess.

It works OK here. I think it is just that the test is really slow (by
design).

-Peff

RE: [PATCH] remote-curl: don't hang when a server dies before any output

[David Turner]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
...
> I'll make that change and then try to wrap this up with a commit message.
> I plan to steal your tests, if that's OK.

Please do!

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Nov 14, 2016 at 05:02:27PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Actually, I take it back. I think it works for a single round of ref
>> > negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
>> > fail t5551.
>> >
>> > I think I've probably made a mis-assumption on exactly when in the HTTP
>> > protocol we will see a flush packet (and perhaps that is a sign that
>> > this protocol-snooping approach is not a good one).
>> 
>> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw
>> it got stuck; could be the same issue, I guess.
>
> It works OK here. I think it is just that the test is really slow (by
> design).

Yeah, I think what I recalled was my old attempt to run the
follow-up "any SHA-1" patch without this one.

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Tue, Nov 15, 2016 at 09:42:57AM -0800, Junio C Hamano wrote:

> >> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw
> >> it got stuck; could be the same issue, I guess.
> >
> > It works OK here. I think it is just that the test is really slow (by
> > design).
> 
> Yeah, I think what I recalled was my old attempt to run the
> follow-up "any SHA-1" patch without this one.

Right, that makes sense.

So I don't feel like we have a good patch for the general case yet, and
I'm probably not going to get around to implementing it anytime soon. So
I'd suggest taking David's original patch (to punt when the response is
empty) in the meantime.

It doesn't fix all cases, but if fixes _a_ case, and probably one of the
most likely one in practice. I don't think it can cause any regressions.
It's a "snooping" solution like mine, but it makes many fewer
assumptions about the protocol. We know that an empty response cannot
possibly advance fetch-pack further because we won't have sent it any
bytes. :)

I do think the commit message could be improved based on the discussion
here, though (at the very least to describe the nature of the deadlock,
and that we are choosing only one of the possible solutions, and why).

-Peff

RE: [PATCH] remote-curl: don't hang when a server dies before any output

[David Turner]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Friday, November 18, 2016 12:02 PM
> To: Junio C Hamano
> Cc: David Turner; git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
> 
> On Tue, Nov 15, 2016 at 09:42:57AM -0800, Junio C Hamano wrote:
> 
> > >> Hmph.  I think I tried David's original under GIT_TEST_LONG and saw
> > >> it got stuck; could be the same issue, I guess.
> > >
> > > It works OK here. I think it is just that the test is really slow
> > > (by design).
> >
> > Yeah, I think what I recalled was my old attempt to run the follow-up
> > "any SHA-1" patch without this one.
> 
> Right, that makes sense.
> 
> So I don't feel like we have a good patch for the general case yet, and
> I'm probably not going to get around to implementing it anytime soon. 

I'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.

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Jeff King]

On Fri, Nov 18, 2016 at 05:04:59PM +0000, David Turner wrote:

> > So I don't feel like we have a good patch for the general case yet,
> > and I'm probably not going to get around to implementing it anytime
> > soon. 
> 
> I'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.

No, mine passes the vanilla test suite, but fails with GIT_TEST_LONG.
If the want/have negotiation takes multiple rounds, the intermediate
rounds don't end on a flush packet, and my patch causes remote-curl to
complain that the response was truncated.

I think you could fix it by teaching remote-curl that the final packet
must be a flush _or_ contain an ACK/NAK, but I didn't try it. That's
getting a bit invasive and brittle.

-Peff

RE: [PATCH] remote-curl: don't hang when a server dies before any output

[David Turner]

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Friday, November 18, 2016 12:09 PM
> To: David Turner
> Cc: Junio C Hamano; git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
> 
> On Fri, Nov 18, 2016 at 05:04:59PM +0000, David Turner wrote:
> 
> > > So I don't feel like we have a good patch for the general case yet,
> > > and I'm probably not going to get around to implementing it anytime
> > > soon.
> >
> > I'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.
> 
> No, mine passes the vanilla test suite, but fails with GIT_TEST_LONG.
> If the want/have negotiation takes multiple rounds, the intermediate
> rounds don't end on a flush packet, and my patch causes remote-curl to
> complain that the response was truncated.
> 
> I think you could fix it by teaching remote-curl that the final packet
> must be a flush _or_ contain an ACK/NAK, but I didn't try it. That's
> getting a bit invasive and brittle.

OK, I'll re-roll mine with a better message.

Re: [PATCH] remote-curl: don't hang when a server dies before any output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I don't feel like we have a good patch for the general case yet, and
> I'm probably not going to get around to implementing it anytime soon. So
> I'd suggest taking David's original patch (to punt when the response is
> empty) in the meantime.

Yup, we are on the same page; the above matches what I wrote in the
draft of the next issue of What's cooking report last night.

> I do think the commit message could be improved based on the discussion
> here, though (at the very least to describe the nature of the deadlock,
> and that we are choosing only one of the possible solutions, and why).

Thanks.  That sounds sensible.