Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> From: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
>
> Johannes found a flaky hang in t5411 in the osx-clang job of the CI/PR
> builds, and ran into this issue using `--stress` option with the
> following errror messages:
s/errror/error/;
> 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
Micronit. "dies first" may be easier to read.
> should forward the error message of the "proc-receive" hook to the
> client side, but there is no such error message in output. It seems
> that the expected error message is overridden by the broken pipe error
> message. The broken pipe error is because "receive-pack" sends other
> commands to the "proc-receive" hook, but the hook dies earlier.
The way the exchange designed to happen in a successfull case is
that receive-pack process feeds the commands over the pipe, and
after feeding all commands start reading response? Even if the
hooks were forbidden from (1) disconnect before reading the commands
and/or (2) speaking before receive-pack finishes feeding the
commands, since they are end-user-supplied random scripts,
receive-pack needs to be prepared to deal with misbehaving hooks.
> To fix this issue, these changes are made:
>
> 1. In "receive-pack", close the input stream for the "proc-receive" hook
> before reading result from "proc-receive".
This is necessary because...? Until/unless we close our end, the
hook would not know we finished talking to them, so it is a good
discipline to close our end of the pipe, but it is unclear to me how
this causes the broken pipe failure, i.e. write by receive-pack into
a pipe connected to the hook that has already died.
> 2. The test helper for "proc-receive" consumes the input stream before
> it die earlier.
"before it dies."
This is merely a workaround in the hook used for testing. End-user
hook that misbehaves can still disconnect without reading anything,
and receive-pack needs to be prepared for such a case, no?
Is it irrelevant because this is only about forcing a flakey test
that fails in two different ways to fail in a predictable way?
Thanks.
> Reported-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> Signed-off-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
> ---
> builtin/receive-pack.c | 4 +++-
> t/helper/test-proc-receive.c | 8 +++++---
> t/t5411/test-0013-bad-protocol.sh | 3 +--
> 3 files changed, 9 insertions(+), 6 deletions(-)
>
> 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..132c74db52 100644
> --- a/t/helper/test-proc-receive.c
> +++ b/t/helper/test-proc-receive.c
> @@ -79,9 +79,11 @@ static void proc_receive_read_commands(struct packet_reader *reader,
> *p++ != ' ' ||
> parse_oid_hex(p, &new_oid, &p) ||
> *p++ != ' ' ||
> - die_readline)
> + die_readline) {
> + while (packet_reader_read(reader) != PACKET_READ_EOF);
Have the empty statement on its own line, i.e.
while (condition)
; /* do nothing */
> die("protocol error: expected 'old new ref', got '%s'",
> - reader->line);
> + die_readline? "<call with --die-readline>": reader->line);
SP around both sides of "?" and ":", and if that makes the line too
long, consider wrapping the line, i.e.
die_readline
? "<call with --die-readline>"
: reader->line);
> + }
> refname = p;
> FLEX_ALLOC_STR(cmd, ref_name, refname);
> oidcpy(&cmd->old_oid, &old_oid);
> @@ -136,7 +138,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);
> diff --git a/t/t5411/test-0013-bad-protocol.sh b/t/t5411/test-0013-bad-protocol.sh
> index c5fe4cb37b..ee75515430 100644
> --- a/t/t5411/test-0013-bad-protocol.sh
> +++ b/t/t5411/test-0013-bad-protocol.sh
> @@ -91,8 +91,7 @@ test_expect_success "proc-receive: bad protocol (hook --die-readline, $PROTOCOL)
> HEAD:refs/for/master/topic \
> >out 2>&1 &&
> make_user_friendly_and_stable_output <out >actual &&
> -
> - grep "remote: fatal: protocol error: expected \"old new ref\", got \"<ZERO-OID> <COMMIT-A> refs/for/master/topic\"" actual &&
> + grep "remote: fatal: protocol error: expected \"old new ref\", got \"<call with --die-readline>\"" actual &&
>
> git -C "$upstream" show-ref >out &&
> make_user_friendly_and_stable_output <out >actual &&
