| Message ID | 20201105152329.12736-1-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] t5411: fix broken pipe write error on proc-receive | expand |
Jiang Xin <worldhello.net@gmail.com> writes: > From: Jiang Xin <zhiyou.jx@alibaba-inc.com> > > 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@gmx.de> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> > --- > 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 &&
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); die("protocol error: expected 'old new ref', got '%s'", - reader->line); + 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 &&