t4053: avoid race when killing background processes

From: Phillip Wood <[email protected]>

The test 'diff --no-index reads from pipes' starts a couple of background processes that write to the pipes that are passed to "diff --no-index". If the test passes then we expect these processes to exit as all their output will have been read. However if the test fails then we want to make sure they do not hang about on the users machine and the test remembers they should be killed by calling

test_when_finished "! kill $!"

after each background process is created. Unfortunately there is a race where test_when_finished may run before the background process exits even when all its output has been read resulting in the kill command succeeding which causes the test to fail. Fix this by ignoring the exit status of the kill command. If the diff is successful we could instead wait for the background process to exit and check their status but that feels like it is testing the platform's printf implementation rather than git's code.

Reported-by Jeff King <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
t4053: avoid race when killing background processes

Thanks to Peff for reporting this. Junio - this fixes a regression introduced in the current cycle. It is fairly minor though so I'm not sure if you want to pick it up now or wait until 2.42.0 is out.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1571%2Fphillipwood%2Fdiff-no-index-pipes-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1571/phillipwood/diff-no-index-pipes-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1571

 t/t4053-diff-no-index.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t4053-diff-no-index.sh +2 -2

index a28b9ff2434..1fb7d334620 100755
@@ -248,11 +248,11 @@ test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '
 	{
 		(test_write_lines a b c >old) &
 	} &&
-	test_when_finished "! kill $!" &&
+	test_when_finished "kill $! || :" &&
 	{
 		(test_write_lines a x c >new) &
 	} &&
-	test_when_finished "! kill $!" &&
+	test_when_finished "kill $! || :" &&
 
 	cat >expect <<-EOF &&
 	diff --git a/old b/new-link

base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61

"Phillip Wood via GitGitGadget" <[email protected]> writes:

While I can see that "kill" in the when-finished handler may or may not find the backgrounded process by the time it is run, and ignoring its exit status (hence keeping test_when_finished happy) would be a reasonable thing to do. I can understand if this patch is to fix a different symptom, namely, when-finished handler sometimes fails and makes the test fail.

But I am not sure how this causes the test to "hang",

which
presumably is a symptom that somebody is trying to read from a pipe that nobody is making progress to write into?

We will
send a signal either way to the writers, and the only difference is that we ignore the exit code.

Granted, when-finished handlers are concatenated with "&&-", and one "kill"s failure will cause the other "kill" not to run, so we may send a signal to only one but not to the other, but that should all happen after "diff --no-index" returns, so it still does not explain the "hang".

Puzzled...

Quoted 27 lines

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1571%2Fphillipwood%2Fdiff-no-index-pipes-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1571/phillipwood/diff-no-index-pipes-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1571

  t/t4053-diff-no-index.sh | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

t/t4053-diff-no-index.sh +2 -2

index a28b9ff2434..1fb7d334620 100755
@@ -248,11 +248,11 @@ test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '
  	{
  		(test_write_lines a b c >old) &
  	} &&
-	test_when_finished "! kill $!" &&
+	test_when_finished "kill $! || :" &&
  	{
  		(test_write_lines a x c >new) &
  	} &&
-	test_when_finished "! kill $!" &&
+	test_when_finished "kill $! || :" &&
 
  	cat >expect <<-EOF &&
  	diff --git a/old b/new-link

base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61

This is only a fix for the test failure that Peff saw when running with --stress.

Quoted 3 lines

which
presumably is a symptom that somebody is trying to read from a pipe that nobody is making progress to write into?

That or a process blocking when tying to open a fifo seems the mostly likely cause but I can't see where that is happening. As you say this patch does not obviously change anything that would be causing the test to hang.

Quoted 9 lines · Junio C Hamano

"Phillip Wood via GitGitGadget" <[email protected]> writes:

While I can see that "kill" in the when-finished handler may or may not find the backgrounded process by the time it is run, and ignoring its exit status (hence keeping test_when_finished happy) would be a reasonable thing to do. I can understand if this patch is to fix a different symptom, namely, when-finished handler sometimes fails and makes the test fail.
But I am not sure how this causes the test to "hang",

This is only a fix for the test failure that Peff saw when running with --stress.