All of lore.kernel.org
 help / color / mirror / Atom feed
* Bug report: Git pull hang occasionally
@ 2016-12-21 19:47 Kai Zhang
  2016-12-21 20:59 ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Kai Zhang @ 2016-12-21 19:47 UTC (permalink / raw)
  To: git

Issue: Git pull hang occasionally, and when git pull start hanging, need manually "kill -9" to stop hanging

Environment:
Server side:
Git version: 2.11.0
OS: ubuntu 12.04
Nginx: 1.9.7.4
fcgiwrap: 1.1.0
Git repo: None bare, small size (less than 5 MB including .git folder), small file number (less than 100 files)

Nginx config for git:

    location ~* /git(\/.*) {
        root /var/git;
        fastcgi_buffers 256 8k;
        fastcgi_param SCRIPT_FILENAME   /usr/lib/git-core/git-http-backend;
        fastcgi_param GIT_HTTP_EXPORT_ALL       true;
        fastcgi_param GIT_PROJECT_ROOT          /var/git;
        fastcgi_param PATH_INFO                 $1;
        fastcgi_pass unix:/var/run/fcgiwrap.socket;
        include         /opt/openresty/nginx/conf/fastcgi_params;
    }

Client side:
Git version: 2.11.0
OS: ubuntu 12.04
All git operations go through http only

End to end work flow:
Keep committing small files to non-bare git repo on server side (twice per second), message will be sent to client side for every update, once client receives message, do git pull

Hanging frequency:
Around 4 times a day

Hanging command stack:
root     32640 23228  0 20:51 ?        00:00:00 git pull -v remote_name master --allow-unrelated-histories
root     32641 32640  0 20:51 ?        00:00:00 git fetch --update-head-ok -v remote_name master
root     32642 32641  0 20:51 ?        00:00:00 git-remote-http remote_name http://server:80/git/repo_name/.git
root     32651 32642  0 20:51 ?        00:00:00 git fetch-pack --stateless-rpc --stdin --lock-pack --thin --no-progress http://server:80/git/repo_name/.git/

Access log for hanging git pull:
10.1.0.10 - - [20/Dec/2016:20:38:10 +0000] "GET /git/repo_name/.git/info/refs?service=git-upload-pack HTTP/1.1" 200 363 "-" "git/2.11.0" "-"
10.1.0.10 - - [20/Dec/2016:20:38:10 +0000] "POST /git/repo_name/.git/git-upload-pack HTTP/1.1" 200 5 "-" "git/2.11.0" "-"

Error log for hanging git pull:
2016/12/20 20:38:10 [error] 9957#0: *687703 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "POST /git/repo_name/.git/git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"


Some observation:
1. When hanging happen, same repository could be cloned or pulled by another process on the same client.
2. After killing hanging git pull, during retry,  same repository can be sync up successfully.
3. Git pull has been executed twice per second. But hanging only happens around 4 times a day.
4. When "fatal: 'HEAD' is a symref but it is not?" happen for POST on server side, client side always start to hang. And when hanging happen on client side, this log for POST always appears. But, if  "fatal: 'HEAD' is a symref but it is not?" happen for GET request on server side, client side never hang. For example: 

2016/12/20 20:36:53 [error] 9954#0: *685174 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "GET /git/repo_name/.git/info/refs?service=git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"

will not trigger hanging on client side. And this log "fatal: 'HEAD' is a symref but it is not?" is happening very rare (less than 10 times a day).


It seems a error handling issue on client side. Any help or pointer on where to look will be appreciated.

Regards
Kai

PS. I am not subscribed to the mailing list, please keep me in Cc



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2016-12-21 19:47 Bug report: Git pull hang occasionally Kai Zhang
@ 2016-12-21 20:59 ` Junio C Hamano
  2016-12-21 21:10   ` Kai Zhang
  2016-12-21 21:32   ` Junio C Hamano
  0 siblings, 2 replies; 8+ messages in thread
From: Junio C Hamano @ 2016-12-21 20:59 UTC (permalink / raw)
  To: Kai Zhang; +Cc: git

Kai Zhang <kai@netskope.com> writes:

> 2016/12/20 20:38:10 [error] 9957#0: *687703 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "POST /git/repo_name/.git/git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"

(Not a solution)

In order to tell the client if HEAD is a symbolic ref and to what
underlying ref it points at if it is a symbolic ref, at the very
beginning of upload-pack, there is a call to head_ref_namespaced()
that uses find_symref().  find_symref() gets "HEAD" and a boolean
that says if it is a symbolic ref, but it does not get where the
symbolic ref points at, so it does resolve_ref_unsafe() to learn
that information.

Between the time head_ref_namespaced() checks the refs database and
finds that HEAD is a symbolic ref, and the time find_symref() calls
resolve_ref_unsafe() to find out where it leads to, if somebody else
updates HEAD, resolve_ref_unsafe() can give an unexpected result, as
all of these read-only operations are performed without any locking.

And the unexpected discrepancy is reported by find_symref() as
fatal.  The server side dies, and somehow that fact is lost between
the upload-pack process and the client and somebody in the middle
(e.g. fastcgi interface or nginx webserver on the server side, or
the remote-curl helper on the client side) keeps the "git fetch"
process waiting.

So there seem to be two issues.  

 - Because of the unlocked read, find_symref() can observe an
   inconsistent state.  Perhaps it should be updated not to die but
   to retry, expecting that transient inconsistency will go away.

 - A fatal error in upload-pack is not reported back to the client
   to cause it exit is an obvious one, and even if we find a way to
   make this fatal error in find_symref() not to trigger, fatal
   errors in other places in the code can trigger the same symptom.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2016-12-21 20:59 ` Junio C Hamano
@ 2016-12-21 21:10   ` Kai Zhang
  2016-12-21 21:32   ` Junio C Hamano
  1 sibling, 0 replies; 8+ messages in thread
From: Kai Zhang @ 2016-12-21 21:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Thank you for your insight and detailed explanation Junio.

I think what you said is what is happening in my environment. Both writing and reading are happening simultaneously. 


> On Dec 21, 2016, at 12:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Kai Zhang <kai@netskope.com> writes:
> 
>> 2016/12/20 20:38:10 [error] 9957#0: *687703 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "POST /git/repo_name/.git/git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"
> 
> (Not a solution)
> 
> In order to tell the client if HEAD is a symbolic ref and to what
> underlying ref it points at if it is a symbolic ref, at the very
> beginning of upload-pack, there is a call to head_ref_namespaced()
> that uses find_symref().  find_symref() gets "HEAD" and a boolean
> that says if it is a symbolic ref, but it does not get where the
> symbolic ref points at, so it does resolve_ref_unsafe() to learn
> that information.
> 
> Between the time head_ref_namespaced() checks the refs database and
> finds that HEAD is a symbolic ref, and the time find_symref() calls
> resolve_ref_unsafe() to find out where it leads to, if somebody else
> updates HEAD, resolve_ref_unsafe() can give an unexpected result, as
> all of these read-only operations are performed without any locking.
> 
> And the unexpected discrepancy is reported by find_symref() as
> fatal.  The server side dies, and somehow that fact is lost between
> the upload-pack process and the client and somebody in the middle
> (e.g. fastcgi interface or nginx webserver on the server side, or
> the remote-curl helper on the client side) keeps the "git fetch"
> process waiting.
> 
> So there seem to be two issues.  
> 
> - Because of the unlocked read, find_symref() can observe an
>   inconsistent state.  Perhaps it should be updated not to die but
>   to retry, expecting that transient inconsistency will go away.
> 
> - A fatal error in upload-pack is not reported back to the client
>   to cause it exit is an obvious one, and even if we find a way to
>   make this fatal error in find_symref() not to trigger, fatal
>   errors in other places in the code can trigger the same symptom.
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2016-12-21 20:59 ` Junio C Hamano
  2016-12-21 21:10   ` Kai Zhang
@ 2016-12-21 21:32   ` Junio C Hamano
  2016-12-21 22:31     ` Kai Zhang
  2017-01-12 18:24     ` Kai Zhang
  1 sibling, 2 replies; 8+ messages in thread
From: Junio C Hamano @ 2016-12-21 21:32 UTC (permalink / raw)
  To: Kai Zhang; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> And the unexpected discrepancy is reported by find_symref() as
> fatal.  The server side dies, and somehow that fact is lost between
> the upload-pack process and the client and somebody in the middle
> (e.g. fastcgi interface or nginx webserver on the server side, or
> the remote-curl helper on the client side) keeps the "git fetch"
> process waiting.
>
> So there seem to be two issues.  
>
>  - Because of the unlocked read, find_symref() can observe an
>    inconsistent state.  Perhaps it should be updated not to die but
>    to retry, expecting that transient inconsistency will go away.
>
>  - A fatal error in upload-pack is not reported back to the client
>    to cause it exit is an obvious one, and even if we find a way to
>    make this fatal error in find_symref() not to trigger, fatal
>    errors in other places in the code can trigger the same symptom.

I wonder if the latter is solved by recent patch 296b847c0d
("remote-curl: don't hang when a server dies before any output",
2016-11-18) on the client side.

-- >8 --
From: David Turner <dturner@twosigma.com>
Date: Fri, 18 Nov 2016 15:30:49 -0500
Subject: [PATCH] remote-curl: don't hang when a server dies before any output

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 its object name.  In this case, the server dies before
sending any data.  Prior to this patch, fetch-pack would wait for
data from the server, and remote-curl would wait for fetch-pack,
causing a deadlock.

Despite this patch, there is other possible malformed input that could
cause the same deadlock (e.g. a half-finished pktline, or a pktline but
no trailing flush).  There are a few possible solutions to this:

1. Allowing remote-curl to tell fetch-pack about the EOF (so that
fetch-pack could know that no more data is coming until it says
something else).  This is tricky because an out-of-band signal would
be required, or the http response would have to be re-framed inside
another layer of pkt-line or something.

2. Make remote-curl understand some of the protocol.  It turns out
that in addition to understanding pkt-line, it would need to watch for
ack/nak.  This is somewhat fragile, as information about the protocol
would end up in two places.  Also, pkt-lines which are already at the
length limit would need special handling.

Both of these solutions would require a fair amount of work, whereas
this hack is easy and solves at least some of the problem.

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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 f14c41f4c0..ee4423659f 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 1ec5b2747a..43665ab4a8 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.11.0-442-g0c85c54a77


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2016-12-21 21:32   ` Junio C Hamano
@ 2016-12-21 22:31     ` Kai Zhang
  2017-01-12 18:24     ` Kai Zhang
  1 sibling, 0 replies; 8+ messages in thread
From: Kai Zhang @ 2016-12-21 22:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

I will verify it. Thanks.
> On Dec 21, 2016, at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> And the unexpected discrepancy is reported by find_symref() as
>> fatal.  The server side dies, and somehow that fact is lost between
>> the upload-pack process and the client and somebody in the middle
>> (e.g. fastcgi interface or nginx webserver on the server side, or
>> the remote-curl helper on the client side) keeps the "git fetch"
>> process waiting.
>> 
>> So there seem to be two issues.  
>> 
>> - Because of the unlocked read, find_symref() can observe an
>>   inconsistent state.  Perhaps it should be updated not to die but
>>   to retry, expecting that transient inconsistency will go away.
>> 
>> - A fatal error in upload-pack is not reported back to the client
>>   to cause it exit is an obvious one, and even if we find a way to
>>   make this fatal error in find_symref() not to trigger, fatal
>>   errors in other places in the code can trigger the same symptom.
> 
> I wonder if the latter is solved by recent patch 296b847c0d
> ("remote-curl: don't hang when a server dies before any output",
> 2016-11-18) on the client side.
> 
> -- >8 --
> From: David Turner <dturner@twosigma.com>
> Date: Fri, 18 Nov 2016 15:30:49 -0500
> Subject: [PATCH] remote-curl: don't hang when a server dies before any output
> 
> 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 its object name.  In this case, the server dies before
> sending any data.  Prior to this patch, fetch-pack would wait for
> data from the server, and remote-curl would wait for fetch-pack,
> causing a deadlock.
> 
> Despite this patch, there is other possible malformed input that could
> cause the same deadlock (e.g. a half-finished pktline, or a pktline but
> no trailing flush).  There are a few possible solutions to this:
> 
> 1. Allowing remote-curl to tell fetch-pack about the EOF (so that
> fetch-pack could know that no more data is coming until it says
> something else).  This is tricky because an out-of-band signal would
> be required, or the http response would have to be re-framed inside
> another layer of pkt-line or something.
> 
> 2. Make remote-curl understand some of the protocol.  It turns out
> that in addition to understanding pkt-line, it would need to watch for
> ack/nak.  This is somewhat fragile, as information about the protocol
> would end up in two places.  Also, pkt-lines which are already at the
> length limit would need special handling.
> 
> Both of these solutions would require a fair amount of work, whereas
> this hack is easy and solves at least some of the problem.
> 
> 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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 f14c41f4c0..ee4423659f 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 1ec5b2747a..43665ab4a8 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.11.0-442-g0c85c54a77
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2016-12-21 21:32   ` Junio C Hamano
  2016-12-21 22:31     ` Kai Zhang
@ 2017-01-12 18:24     ` Kai Zhang
  2017-01-12 21:12       ` Junio C Hamano
  1 sibling, 1 reply; 8+ messages in thread
From: Kai Zhang @ 2017-01-12 18:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Hi Junio,

After apply this patch, hanging did not happen again. Would this patch go to release in near future?

Thanks.

Regards,
Kai 
> On Dec 21, 2016, at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> And the unexpected discrepancy is reported by find_symref() as
>> fatal.  The server side dies, and somehow that fact is lost between
>> the upload-pack process and the client and somebody in the middle
>> (e.g. fastcgi interface or nginx webserver on the server side, or
>> the remote-curl helper on the client side) keeps the "git fetch"
>> process waiting.
>> 
>> So there seem to be two issues.  
>> 
>> - Because of the unlocked read, find_symref() can observe an
>>   inconsistent state.  Perhaps it should be updated not to die but
>>   to retry, expecting that transient inconsistency will go away.
>> 
>> - A fatal error in upload-pack is not reported back to the client
>>   to cause it exit is an obvious one, and even if we find a way to
>>   make this fatal error in find_symref() not to trigger, fatal
>>   errors in other places in the code can trigger the same symptom.
> 
> I wonder if the latter is solved by recent patch 296b847c0d
> ("remote-curl: don't hang when a server dies before any output",
> 2016-11-18) on the client side.
> 
> -- >8 --
> From: David Turner <dturner@twosigma.com>
> Date: Fri, 18 Nov 2016 15:30:49 -0500
> Subject: [PATCH] remote-curl: don't hang when a server dies before any output
> 
> 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 its object name.  In this case, the server dies before
> sending any data.  Prior to this patch, fetch-pack would wait for
> data from the server, and remote-curl would wait for fetch-pack,
> causing a deadlock.
> 
> Despite this patch, there is other possible malformed input that could
> cause the same deadlock (e.g. a half-finished pktline, or a pktline but
> no trailing flush).  There are a few possible solutions to this:
> 
> 1. Allowing remote-curl to tell fetch-pack about the EOF (so that
> fetch-pack could know that no more data is coming until it says
> something else).  This is tricky because an out-of-band signal would
> be required, or the http response would have to be re-framed inside
> another layer of pkt-line or something.
> 
> 2. Make remote-curl understand some of the protocol.  It turns out
> that in addition to understanding pkt-line, it would need to watch for
> ack/nak.  This is somewhat fragile, as information about the protocol
> would end up in two places.  Also, pkt-lines which are already at the
> length limit would need special handling.
> 
> Both of these solutions would require a fair amount of work, whereas
> this hack is easy and solves at least some of the problem.
> 
> 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>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 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 f14c41f4c0..ee4423659f 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 1ec5b2747a..43665ab4a8 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.11.0-442-g0c85c54a77


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2017-01-12 18:24     ` Kai Zhang
@ 2017-01-12 21:12       ` Junio C Hamano
  2017-01-12 21:17         ` Kai Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2017-01-12 21:12 UTC (permalink / raw)
  To: Kai Zhang; +Cc: git

Kai Zhang <kai@netskope.com> writes:

>> On Dec 21, 2016, at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>  ...
>> 
>> I wonder if the latter is solved by recent patch 296b847c0d
>> ("remote-curl: don't hang when a server dies before any output",
>> 2016-11-18) on the client side.
>> ...

> After apply this patch, hanging did not happen again. 

Thanks for confirming.

> Would this patch go to release in near future?

I see 296b847c0d in the message you are responding to (by the way,
do not top-post to this list).  

Let's check it together.

	$ git log master..296b847c0d
	$ git merge-base master 296b847c0d
        296b847c0d6de63353e236cfbf94163d24155529

Yup, it already is in master.  

Using a third-party script "when-merged" [*1*], we can easily find
that it was merged a few days ago to 'master', after cooking in
'next' for a handful of weeks:

	$ git when-merged 296b847c0d next
	refs/heads/next 3ea70d01afc6305b88d33b8585f1fc41c486a182
	$ git when-merged 296b847c0d master
	refs/heads/master d984592043aec3c9f5b1955560a133896ca115b5
	$ git show -s --format='%cI' 3ea70d01af d984592043 
        2016-12-05T11:38:03-08:00
        2017-01-10T15:24:25-08:00

Unless people find regressions caused by this change (in which case
we may have to revert it), this will be included in the release at
the end of this cycle.  http://tinyurl.com/gitCal tells us that the
current cycle is expected to complete early February.


[Footnote]

*1* git://github.com/mhagger/git-when-merged

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Bug report: Git pull hang occasionally
  2017-01-12 21:12       ` Junio C Hamano
@ 2017-01-12 21:17         ` Kai Zhang
  0 siblings, 0 replies; 8+ messages in thread
From: Kai Zhang @ 2017-01-12 21:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git


> On Jan 12, 2017, at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Kai Zhang <kai@netskope.com> writes:
> 
>>> On Dec 21, 2016, at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> ...
>>> 
>>> I wonder if the latter is solved by recent patch 296b847c0d
>>> ("remote-curl: don't hang when a server dies before any output",
>>> 2016-11-18) on the client side.
>>> ...
> 
>> After apply this patch, hanging did not happen again. 
> 
> Thanks for confirming.
> 
>> Would this patch go to release in near future?
> 
> I see 296b847c0d in the message you are responding to (by the way,
> do not top-post to this list).  
> 
> Let's check it together.
> 
> 	$ git log master..296b847c0d
> 	$ git merge-base master 296b847c0d
>        296b847c0d6de63353e236cfbf94163d24155529
> 
> Yup, it already is in master.  
> 
> Using a third-party script "when-merged" [*1*], we can easily find
> that it was merged a few days ago to 'master', after cooking in
> 'next' for a handful of weeks:
> 
> 	$ git when-merged 296b847c0d next
> 	refs/heads/next 3ea70d01afc6305b88d33b8585f1fc41c486a182
> 	$ git when-merged 296b847c0d master
> 	refs/heads/master d984592043aec3c9f5b1955560a133896ca115b5
> 	$ git show -s --format='%cI' 3ea70d01af d984592043 
>        2016-12-05T11:38:03-08:00
>        2017-01-10T15:24:25-08:00
> 
> Unless people find regressions caused by this change (in which case
> we may have to revert it), this will be included in the release at
> the end of this cycle.  http://tinyurl.com/gitCal tells us that the
> current cycle is expected to complete early February.
> 
> 
> [Footnote]
> 
> *1* git://github.com/mhagger/git-when-merged

Thank you for your help!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-01-12 21:17 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-21 19:47 Bug report: Git pull hang occasionally Kai Zhang
2016-12-21 20:59 ` Junio C Hamano
2016-12-21 21:10   ` Kai Zhang
2016-12-21 21:32   ` Junio C Hamano
2016-12-21 22:31     ` Kai Zhang
2017-01-12 18:24     ` Kai Zhang
2017-01-12 21:12       ` Junio C Hamano
2017-01-12 21:17         ` Kai Zhang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.