patcht4053: avoid race when killing background processes

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 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 <peff@peff.net>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>     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(-)
>
> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
> index a28b9ff2434..1fb7d334620 100755
> --- a/t/t4053-diff-no-index.sh
> +++ b/t/t4053-diff-no-index.sh
> @@ -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

> 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", 

>  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...

>> 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(-)
>>
>> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
>> index a28b9ff2434..1fb7d334620 100755
>> --- a/t/t4053-diff-no-index.sh
>> +++ b/t/t4053-diff-no-index.sh
>> @@ -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.
> 
> > 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.

> On 10/08/2023 18:40, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.