t5411: consistent result for proc-receive broken test

Commit Message

Jiang Xin Nov. 7, 2020, 2:57 a.m. UTC

From: Jiang Xin <[e-mail]>

Johannes found a flaky hang in `t5411/test-0013-bad-protocol.sh` in the
osx-clang job of the CI/PR builds, and ran into an issue when using
the `--stress` option with the following error messages:

    fatal: unable to write flush packet: Broken pipe
    send-pack: unexpected disconnect while reading sideband packet
    fatal: the remote end hung up unexpectedly

In this test case, the "proc-receive" hook sends an error message and
dies earlier. While "receive-pack" on the other side of the pipe
should forward the error message of the "proc-receive" hook to the
client side, but it fails to do so. Perhaps it depends on the size of
the pipe buffer and timing of the processes getting scheduled.

The way the exchange designed to happen in a successful case is that
"receive-pack" process feeds the commands, and optional push options
over the pipe, and after feeding all these information, "receive-pack"
starts reading the response of "proc-receive". Let "receive-pack" close
the input stream to "proc-receive" right before reading the response,
so that "proc-receive" may consume all the input from "receive-pack"
before sending an error message and closing the pipe.

The "proc-receive" hook may close the pipe at any time. Three options
(--die-version, --die-readline, and --die-report) can be used for the
test helper to simulate different broken cases. In order to keep the
test results consistent under stress test, only status reports are
matched.

Reported-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Jiang Xin <[e-mail]>
---
 builtin/receive-pack.c            |  4 ++-
 t/helper/test-proc-receive.c      | 20 +++++++++---
 t/t5411/test-0013-bad-protocol.sh | 53 +++++++++++++++++++++++++------
 3 files changed, 63 insertions(+), 14 deletions(-)

Comments

Jiang Xin Nov. 9, 2020, 7:29 a.m. UTC | #1

Jiang Xin <[e-mail]> 于2020年11月7日周六 上午10:57写道：
> @@ -75,13 +78,18 @@ static void proc_receive_read_commands(struct packet_reader *reader,
>                 if (packet_reader_read(reader) != PACKET_READ_NORMAL)
>                         break;
>
> +               if (die_readline)
> +                       die("die with the --die-readline option");
> +
>                 if (parse_oid_hex(reader->line, &old_oid, &p) ||
>                     *p++ != ' ' ||
>                     parse_oid_hex(p, &new_oid, &p) ||
> -                   *p++ != ' ' ||
> -                   die_readline)
> +                   *p++ != ' ') {
> +                       while (packet_reader_read(reader) != PACKET_READ_EOF)
> +                               ; /* do nothing */
>                         die("protocol error: expected 'old new ref', got '%s'",
>                             reader->line);
> +               }
>                 refname = p;
>                 FLEX_ALLOC_STR(cmd, ref_name, refname);
>                 oidcpy(&cmd->old_oid, &old_oid);

Still have problems under the stress test.  Today I figured out how to
run stress test by setting proper environment variable
GIT_TEST_STRESS_LOAD without consuming too many resources and leading
to random strange errors. E.g.:

    $ GIT_TEST_STRESS_LOAD=11 sh  t5411-proc-receive-hook.sh -v --stress

Will send patch v2 later with the following changes:

-- snip --
diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c
index ee5717ba0a..9f7fbc5b7a 100644
--- a/t/helper/test-proc-receive.c
+++ b/t/helper/test-proc-receive.c
@@ -78,17 +78,18 @@ static void proc_receive_read_commands(struct
packet_reader *reader,
                if (packet_reader_read(reader) != PACKET_READ_NORMAL)
                        break;

-               if (die_readline)
-                       die("die with the --die-readline option");
-
                if (parse_oid_hex(reader->line, &old_oid, &p) ||
                    *p++ != ' ' ||
                    parse_oid_hex(p, &new_oid, &p) ||
-                   *p++ != ' ') {
+                   *p++ != ' ' ||
+                   die_readline) {
+                       char *bad_line = xstrdup(reader->line);
                        while (packet_reader_read(reader) != PACKET_READ_EOF)
                                ; /* do nothing */
+                       if (die_readline)
+                               die("die with the --die-readline option");
                        die("protocol error: expected 'old new ref', got '%s'",
-                           reader->line);
+                           bad_line);
                }
                refname = p;
                FLEX_ALLOC_STR(cmd, ref_name, refname);
-- snap --

Patch

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb9909c52e..6bd402897c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1172,6 +1172,7 @@  static int run_proc_receive_hook(struct command *commands,
 	if (version != 1) {
 		strbuf_addf(&errmsg, "proc-receive version '%d' is not supported",
 			    version);
+		close(proc.in);
 		code = -1;
 		goto cleanup;
 	}
@@ -1196,11 +1197,12 @@  static int run_proc_receive_hook(struct command *commands,
 		packet_flush(proc.in);
 	}
 
+	close(proc.in);
+
 	/* Read result from proc-receive */
 	code = read_proc_receive_report(&reader, commands, &errmsg);
 
 cleanup:
-	close(proc.in);
 	close(proc.out);
 	if (use_sideband)
 		finish_async(&muxer);
diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c
index 42164d9898..ee5717ba0a 100644
--- a/t/helper/test-proc-receive.c
+++ b/t/helper/test-proc-receive.c
@@ -12,6 +12,7 @@  static const char *proc_receive_usage[] = {
 
 static int die_version;
 static int die_readline;
+static int die_report;
 static int no_push_options;
 static int use_atomic;
 static int use_push_options;
@@ -52,8 +53,10 @@  static void proc_receive_verison(struct packet_reader *reader) {
 		}
 	}
 
-	if (server_version != 1 || die_version)
+	if (server_version != 1)
 		die("bad protocol version: %d", server_version);
+	if (die_version)
+		die("die with the --die-version option");
 
 	packet_write_fmt(1, "version=%d%c%s\n",
 			 version, '\0',
@@ -75,13 +78,18 @@  static void proc_receive_read_commands(struct packet_reader *reader,
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
 			break;
 
+		if (die_readline)
+			die("die with the --die-readline option");
+
 		if (parse_oid_hex(reader->line, &old_oid, &p) ||
 		    *p++ != ' ' ||
 		    parse_oid_hex(p, &new_oid, &p) ||
-		    *p++ != ' ' ||
-		    die_readline)
+		    *p++ != ' ') {
+			while (packet_reader_read(reader) != PACKET_READ_EOF)
+				; /* do nothing */
 			die("protocol error: expected 'old new ref', got '%s'",
 			    reader->line);
+		}
 		refname = p;
 		FLEX_ALLOC_STR(cmd, ref_name, refname);
 		oidcpy(&cmd->old_oid, &old_oid);
@@ -121,6 +129,8 @@  int cmd__proc_receive(int argc, const char **argv)
 			 "die during version negotiation"),
 		OPT_BOOL(0, "die-readline", &die_readline,
 			 "die when readline"),
+		OPT_BOOL(0, "die-report", &die_report,
+			 "die when reporting"),
 		OPT_STRING_LIST('r', "return", &returns, "old/new/ref/status/msg",
 				"return of results"),
 		OPT__VERBOSE(&verbose, "be verbose"),
@@ -136,7 +146,7 @@  int cmd__proc_receive(int argc, const char **argv)
 		usage_msg_opt("Too many arguments.", proc_receive_usage, options);
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
-			   PACKET_READ_DIE_ON_ERR_PACKET);
+			   PACKET_READ_GENTLE_ON_EOF);
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 	proc_receive_verison(&reader);
@@ -166,6 +176,8 @@  int cmd__proc_receive(int argc, const char **argv)
 				fprintf(stderr, "proc-receive> %s\n", item->string);
 	}
 
+	if (die_report)
+		die("die with the --die-report option");
 	if (returns.nr)
 		for_each_string_list_item(item, &returns)
 			packet_write_fmt(1, "%s\n", item->string);
diff --git a/t/t5411/test-0013-bad-protocol.sh b/t/t5411/test-0013-bad-protocol.sh
index c5fe4cb37b..5c5241bc95 100644
--- a/t/t5411/test-0013-bad-protocol.sh
+++ b/t/t5411/test-0013-bad-protocol.sh
@@ -55,19 +55,16 @@  test_expect_success "proc-receive: bad protocol (hook --die-version, $PROTOCOL)"
 	test_must_fail git -C workbench push origin \
 		HEAD:refs/for/master/topic \
 		>out 2>&1 &&
-	make_user_friendly_and_stable_output <out >actual &&
-
+	make_user_friendly_and_stable_output <out |
+		sed -n \
+			-e "/^To / { s/   */ /g; p; }" \
+			-e "/^ ! / { s/   */ /g; p; }" \
+			>actual &&
 	cat >expect <<-EOF &&
-	remote: # pre-receive hook
-	remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/master/topic
-	remote: # proc-receive hook
-	remote: fatal: bad protocol version: 1
-	remote: error: proc-receive version "0" is not supported
 	To <URL/of/upstream.git>
 	 ! [remote rejected] HEAD -> refs/for/master/topic (fail to run proc-receive hook)
 	EOF
 	test_cmp expect actual &&
-
 	git -C "$upstream" show-ref >out &&
 	make_user_friendly_and_stable_output <out >actual &&
 	cat >expect <<-EOF &&
@@ -90,10 +87,48 @@  test_expect_success "proc-receive: bad protocol (hook --die-readline, $PROTOCOL)
 	test_must_fail git -C workbench push origin \
 		HEAD:refs/for/master/topic \
 		>out 2>&1 &&
+	make_user_friendly_and_stable_output <out |
+		sed -n \
+			-e "/^To / { s/   */ /g; p; }" \
+			-e "/^ ! / { s/   */ /g; p; }" \
+			>actual &&
+	cat >expect <<-EOF &&
+	To <URL/of/upstream.git>
+	 ! [remote rejected] HEAD -> refs/for/master/topic (proc-receive failed to report status)
+	EOF
+	test_cmp expect actual &&
+	git -C "$upstream" show-ref >out &&
 	make_user_friendly_and_stable_output <out >actual &&
+	cat >expect <<-EOF &&
+	<COMMIT-A> refs/heads/master
+	EOF
+	test_cmp expect actual
+'
 
-	grep "remote: fatal: protocol error: expected \"old new ref\", got \"<ZERO-OID> <COMMIT-A> refs/for/master/topic\"" actual &&
+test_expect_success "setup proc-receive hook (hook --die-report, $PROTOCOL)" '
+	write_script "$upstream/hooks/proc-receive" <<-EOF
+	printf >&2 "# proc-receive hook\n"
+	test-tool proc-receive -v --die-report
+	EOF
+'
 
+# Refs of upstream : master(A)
+# Refs of workbench: master(A)  tags/v123
+# git push         :                       refs/for/master/topic(A)
+test_expect_success "proc-receive: bad protocol (hook --die-report, $PROTOCOL)" '
+	test_must_fail git -C workbench push origin \
+		HEAD:refs/for/master/topic \
+		>out 2>&1 &&
+	make_user_friendly_and_stable_output <out |
+		sed -n \
+			-e "/^To / { s/   */ /g; p; }" \
+			-e "/^ ! / { s/   */ /g; p; }" \
+			>actual &&
+	cat >expect <<-EOF &&
+	To <URL/of/upstream.git>
+	 ! [remote rejected] HEAD -> refs/for/master/topic (proc-receive failed to report status)
+	EOF
+	test_cmp expect actual &&
 	git -C "$upstream" show-ref >out &&
 	make_user_friendly_and_stable_output <out >actual &&
 	cat >expect <<-EOF &&