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

[Junio C Hamano <gitster@pobox.com>]

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;