git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 01/10] built-in add -i/-p: treat SIGPIPE as EOF

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 14, 2020, 12:47 UTC
Message-ID
<nycvar.QRO.7.76.6.2001141301310.46@tvgsbejvaqbjf.bet>
In-Reply-To
<20200113170417.GK32750@szeder.dev>
Hi Gábor,
On Mon, 13 Jan 2020, SZEDER Gábor wrote:
Show 18 quoted lines
> On Mon, Jan 13, 2020 at 08:29:22AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > As noticed by Gábor Szeder, if we want to run `git add -p` with
> > redirected input through `test_must_fail` in the test suite, we must
> > expect that a SIGPIPE can happen due to `stdin` coming to its end.
>
> I don't think this issue is related to the redirected input: I
> modified that flaky test to send "unlimited" data to 'git add's stdin,
> i.e.:
>
>   /usr/bin/yes | test_must_fail force_color git add -p
>
> and the test with --stress still failed with SIGPIPE all the same and
> just as fast.
>
> After looking into it, the issue seems to be sending data to the
> broken diffFilter process.

Ouch. Thank you for investigating. For my education, how did you debug this? I could not find a way to identify *what* caused that SIGPIPE...

Show 10 quoted lines
> So in that test the diff is "filtered" through 'echo too-short', which
> exits real fast, and doesn't read its standard input at all (well, apart
> from e.g. the usual kernel buffering that might happen on a pipe between
> the two processes). Making sure that the diffFilter process reads all
> the data before exiting, i.e. changing it to:
>
>   test_config interactive.diffFilter "cat >/dev/null ; echo too-short" &&
>
> made the test reliable, with over 2000 --stress repetitions, and that
> with only a single "y" on 'git add's stdin.

Ah, my diff filter simply ignores the `stdin`... That's easy enough to fix, and since real-world diff filters probably won't just blatantly ignore the input, I think it is legitimate to change the test.

> Now, merely tweaking the test is clearly insufficient, because we not
> only want the test to be realiable, but we want 'git add' to die
> gracefully when users out there mess up their configuration.

I think it is sufficient to tweak the test, but I agree that a better error message might be good when users out there mess up their configuration.

Show 6 quoted lines
> Ignoring SIGPIPE can surely accomplish that, but I'm not sure about
> the scope.  I mean your patch seems to ignore SIGPIPE basically for
> almost the whole 'git add -(i|p)' process, but perhaps it should be
> limited only to the surroundings of the pipe_command() call running
> the diffFilter, and be done as part of the next patch adding the 'if
> (diff_filter)' block.
Right. Very heavy-handed, and probably inviting unwanted side effects.
Show 71 quoted lines
> Furthermore, I'm worried that by simply ignoring SIGPIPE we might just
> ignore a more fundamental issue in pipe_command(): shouldn't that
> function be smart enough not to write() to a fd that has no one on the
> other side to read it in the first place?!
>
> So, when the diffFilter process exits unexpectedly early, then the
> poll() call in pipe_command() -> pump_io() -> pump_io_round() returns
> with success and usually sets 'revents' for the child process' stdin
> to 12 (i.e. 'POLLOUT | POLLERR'; gah, how I hate unnamed constants :).
> Unfortunately, at that point we don't take any special action on
> POLLERR, but call xwrite() to try to write to the dead fd anyway,
> which then promptly triggers SIGPIPE.  (This is what usually happens
> when stepping through the statements of those functions in a debugger,
> and the diffFilter process has all the time in the world to exit.)
>
> We could handle POLLERR with a patch like this:
>
>   --- >8 ---
>
> Subject: run-command: handle POLLERR in pump_io_round() to reduce risk of SIGPIPE
>
> diff --git a/run-command.c b/run-command.c
> index 3449db319b..57093f0acc 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1416,25 +1416,31 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
>  	if (poll(pfd, pollsize, -1) < 0) {
>  		if (errno == EINTR)
>  			return 1;
>  		die_errno("poll failed");
>  	}
>
>  	for (i = 0; i < nr; i++) {
>  		struct io_pump *io = &slots[i];
>
>  		if (io->fd < 0)
>  			continue;
>
> -		if (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))
> +		if (io->pfd->revents & POLLERR) {
> +			io->error = ECONNRESET;  /* What should we report to the caller? */
> +			close(io->fd);
> +			io->fd = -1;
> +			continue;
> +		}
> +		if (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLNVAL)))
>  			continue;
>
>  		if (io->type == POLLOUT) {
>  			ssize_t len = xwrite(io->fd,
>  					     io->u.out.buf, io->u.out.len);
>  			if (len < 0) {
>  				io->error = errno;
>  				close(io->fd);
>  				io->fd = -1;
>  			} else {
>  				io->u.out.buf += len;
>  				io->u.out.len -= len;
>
>   --- >8 ---
>
> Unfortunately #1, this changes the error 'git add -p' dies with from:
>
>   error: mismatched output from interactive.diffFilter
>
> to:
>
>   error: failed to run 'echo too-short'
>
> It might affect other commands as well, but FWIW the test suite
> doesn't catch any.

Hmm. My first impression is that the error message could be a bit better, but that it is probably a good thing to have. It would have helped _me_ understand the issue at hand.

Show 83 quoted lines
> Unfortunately #2, the above patch doesn't completely eliminates the
> SIGPIPE, but only (greatly) reduces its probability.  It is possible
> that:
>
>   - poll() returns with success and indicating a writable fd without
>     any error, i.e. 'revents = 4'.
>
>   - the bogus diffFilter exits, closing its stdin.
>
>   - 'git add' attempts to xwrite() to the now closed fd, and triggers
>     a SIGPIPE right away.
>
> This happens much rarer, 'GIT_TEST_ADD_I_USE_BUILTIN=1
> ./t3701-add-interactive.sh -r 39,49 --stress-jobs=<4*nr-of-cores>
> --stress' tends to take over 200 repetitions.  The patch below
> reproduces it fairly reliably by adding two strategically-placed
> sleep()s, with a bit of extra debug output:
>
>   --- >8 ---
>
> diff --git a/add-patch.c b/add-patch.c
> index d8dafa8168..0fd017bbd3 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -421,6 +421,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  			filter_cp.git_cmd = 0;
>  			filter_cp.use_shell = 1;
>  			strbuf_reset(&s->buf);
> +			fprintf(stderr, "about to run diffFilter\n");
>  			if (pipe_command(&filter_cp,
>  					 colored->buf, colored->len,
>  					 &s->buf, colored->len,
> diff --git a/run-command.c b/run-command.c
> index 57093f0acc..49ae88a922 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1419,6 +1419,7 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
>  		die_errno("poll failed");
>  	}
>
> +	sleep(2);
>  	for (i = 0; i < nr; i++) {
>  		struct io_pump *io = &slots[i];
>
> @@ -1435,8 +1436,11 @@ static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
>  			continue;
>
>  		if (io->type == POLLOUT) {
> -			ssize_t len = xwrite(io->fd,
> +			ssize_t len;
> +			fprintf(stderr, "attempting to xwrite() %lu bytes to a fd with revents flags 0x%hx\n", io->u.out.len, io->pfd->revents);
> +			len = xwrite(io->fd,
>  					     io->u.out.buf, io->u.out.len);
> +			fprintf(stderr, "after xwrite()\n");
>  			if (len < 0) {
>  				io->error = errno;
>  				close(io->fd);
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 12ee321707..acffc9af37 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -561,7 +561,7 @@ test_expect_success 'detect bogus diffFilter output' '
>  	git reset --hard &&
>
>  	echo content >test &&
> -	test_config interactive.diffFilter "echo too-short" &&
> +	test_config interactive.diffFilter "sleep 1 ; echo too-short" &&
>  	printf y >y &&
>  	test_must_fail force_color git add -p <y
>  '
>
>   --- >8 ---
>
> and 'GIT_TEST_ADD_I_USE_BUILTIN=1 ./t3701-add-interactive.sh -r 39,49'
> fails with:
>
>   + test_must_fail force_color git add -p
>   about to run diffFilter
>   attempting to xwrite() 224 bytes to a fd with revents flags 0x4
>   test_must_fail: died by signal 13: force_color git add -p
>
> I don't understand why we get SIGPIPE right away instead of some error
> that we can act upon (ECONNRESET?).
Isn't it buffered?

In any case, I would take the above-mentioned patch, even if it makes it "only" less likely to hit `SIGPIPE`.

Show 8 quoted lines
> FWIW, it fails the same way not only on my box, but on Travis CI's Linux
> and OSX images as well.
>
>   https://travis-ci.org/szeder/git/jobs/636446843#L2937
>
>
> Cc'ing Peff for all things SIGPIPE :) who also happens to be the
> author of both pipe_command() and that now flaky test.

I'll go with Peff's suggestion to use `sed 1d` instead of `echo too-short`.

Thanks, Dscho

Previous: Jeff KingNext: SZEDER Gábor
Message 41 of 63 in “built-in add -p: add support for the same config settings as the Perl version”
  1. 0/9 built-in add -p: add support for the same config settings as the Perl versionJohannes Schindelin via GitGitGadget, Dec 21, 2019
  2. 1/9 built-in add -p: support interactive.diffFilterJohannes Schindelin via GitGitGadget, Dec 21, 2019
  3. 2/9 built-in add -p: handle diff.algorithmJohannes Schindelin via GitGitGadget, Dec 21, 2019
  4. 6/9 built-in add -p: respect the `interactive.singlekey` config settingJohannes Schindelin via GitGitGadget, Dec 21, 2019
  5. 8/9 built-in add -p: handle Escape sequences more efficientlyJohannes Schindelin via GitGitGadget, Dec 21, 2019
  6. 4/9 terminal: accommodate Git for Windows' default terminalJohannes Schindelin via GitGitGadget, Dec 21, 2019
  7. 3/9 terminal: make the code of disable_echo() reusableJohannes Schindelin via GitGitGadget, Dec 21, 2019
  8. 7/9 built-in add -p: handle Escape sequences in interactive.singlekey modeJohannes Schindelin via GitGitGadget, Dec 21, 2019
  9. 5/9 terminal: add a new function to read a single keystrokeJohannes Schindelin via GitGitGadget, Dec 21, 2019
  10. 9/9 ci: include the built-in `git add -i` in the `linux-gcc` jobJohannes Schindelin via GitGitGadget, Dec 21, 2019
  11. SZEDER GáborDec 21, 2019
  12. Johannes SchindelinDec 25, 2019
  13. Junio C HamanoDec 22, 2019
  14. Johannes SchindelinDec 25, 2019
  15. Junio C HamanoDec 24, 2019
  16. Junio C HamanoDec 24, 2019
  17. Simon RuderichDec 25, 2019
  18. Johannes SchindelinDec 25, 2019
  19. Johannes SchindelinDec 25, 2019
  20. 0/9 built-in add -p: add support for the same config settings as the Perl versionJohannes Schindelin via GitGitGadget, Dec 25, 2019
  21. 1/9 built-in add -p: support interactive.diffFilterJohannes Schindelin via GitGitGadget, Dec 25, 2019
  22. SZEDER GáborJan 7, 2020
  23. Johannes SchindelinJan 13, 2020
  24. 2/9 built-in add -p: handle diff.algorithmJohannes Schindelin via GitGitGadget, Dec 25, 2019
  25. 3/9 terminal: make the code of disable_echo() reusableJohannes Schindelin via GitGitGadget, Dec 25, 2019
  26. 4/9 terminal: accommodate Git for Windows' default terminalJohannes Schindelin via GitGitGadget, Dec 25, 2019
  27. 6/9 built-in add -p: respect the `interactive.singlekey` config settingJohannes Schindelin via GitGitGadget, Dec 25, 2019
  28. 8/9 built-in add -p: handle Escape sequences more efficientlyJohannes Schindelin via GitGitGadget, Dec 25, 2019
  29. 9/9 ci: include the built-in `git add -i` in the `linux-gcc` jobJohannes Schindelin via GitGitGadget, Dec 25, 2019
  30. Derrick StoleeDec 26, 2019
  31. Johannes SchindelinJan 1, 2020
  32. 7/9 built-in add -p: handle Escape sequences in interactive.singlekey modeJohannes Schindelin via GitGitGadget, Dec 25, 2019
  33. 5/9 terminal: add a new function to read a single keystrokeJohannes Schindelin via GitGitGadget, Dec 25, 2019
  34. Junio C HamanoDec 26, 2019
  35. 00/10 built-in add -p: add support for the same config settings as the Perl versionJohannes Schindelin via GitGitGadget, Jan 13, 2020
  36. 01/10 built-in add -i/-p: treat SIGPIPE as EOFJohannes Schindelin via GitGitGadget, Jan 13, 2020
  37. SZEDER GáborJan 13, 2020
  38. Jeff KingJan 13, 2020
  39. Junio C HamanoJan 15, 2020
  40. Jeff KingJan 15, 2020
  41. Johannes SchindelinJan 14, 2020
  42. SZEDER GáborJan 17, 2020
  43. Jeff KingJan 17, 2020
  44. 02/10 built-in add -p: support interactive.diffFilterJohannes Schindelin via GitGitGadget, Jan 13, 2020
  45. 03/10 built-in add -p: handle diff.algorithmJohannes Schindelin via GitGitGadget, Jan 13, 2020
  46. 05/10 terminal: accommodate Git for Windows' default terminalJohannes Schindelin via GitGitGadget, Jan 13, 2020
  47. 04/10 terminal: make the code of disable_echo() reusableJohannes Schindelin via GitGitGadget, Jan 13, 2020
  48. 06/10 terminal: add a new function to read a single keystrokeJohannes Schindelin via GitGitGadget, Jan 13, 2020
  49. 07/10 built-in add -p: respect the `interactive.singlekey` config settingJohannes Schindelin via GitGitGadget, Jan 13, 2020
  50. 10/10 ci: include the built-in `git add -i` in the `linux-gcc` jobJohannes Schindelin via GitGitGadget, Jan 13, 2020
  51. 09/10 built-in add -p: handle Escape sequences more efficientlyJohannes Schindelin via GitGitGadget, Jan 13, 2020
  52. 08/10 built-in add -p: handle Escape sequences in interactive.singlekey modeJohannes Schindelin via GitGitGadget, Jan 13, 2020
  53. 00/10 built-in add -p: add support for the same config settings as the Perl versionJohannes Schindelin via GitGitGadget, Jan 14, 2020
  54. 01/10 t3701: adjust difffilter testJohannes Schindelin via GitGitGadget, Jan 14, 2020
  55. 02/10 built-in add -p: support interactive.diffFilterJohannes Schindelin via GitGitGadget, Jan 14, 2020
  56. 05/10 terminal: accommodate Git for Windows' default terminalJohannes Schindelin via GitGitGadget, Jan 14, 2020
  57. 08/10 built-in add -p: handle Escape sequences in interactive.singlekey modeJohannes Schindelin via GitGitGadget, Jan 14, 2020
  58. 10/10 ci: include the built-in `git add -i` in the `linux-gcc` jobJohannes Schindelin via GitGitGadget, Jan 14, 2020
  59. 07/10 built-in add -p: respect the `interactive.singlekey` config settingJohannes Schindelin via GitGitGadget, Jan 14, 2020
  60. 03/10 built-in add -p: handle diff.algorithmJohannes Schindelin via GitGitGadget, Jan 14, 2020
  61. 06/10 terminal: add a new function to read a single keystrokeJohannes Schindelin via GitGitGadget, Jan 14, 2020
  62. 04/10 terminal: make the code of disable_echo() reusableJohannes Schindelin via GitGitGadget, Jan 14, 2020
  63. 09/10 built-in add -p: handle Escape sequences more efficientlyJohannes Schindelin via GitGitGadget, Jan 14, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.