[PATCH] t4053: avoid race when killing background processes

[PATCH] t4053: avoid race when killing background processes

[Phillip Wood via GitGitGadget]

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

Re: [PATCH] t4053: avoid race when killing background processes

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

This did not cut -rc1 that was scheduled for yesterday, but a fix
for a new test added during the cycle is a very welcome addition.

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

> 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

Re: [PATCH] t4053: avoid race when killing background processes

[Phillip Wood]

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.

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

Best Wishes

Phillip

>  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

Re: [PATCH] t4053: avoid race when killing background processes

[Jeff King]

On Fri, Aug 11, 2023 at 10:56:40AM +0100, Phillip Wood wrote:

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

Yeah, sorry if my initial report was unclear. It was while digging for
the cause of the hang that I ran into the racy failures. This patch
looks good for fixing the race. I'm still waiting to hit the hang again
to get more data. But if it never happens again, then that is a success
of sorts. :)

-Peff

Re: [PATCH] t4053: avoid race when killing background processes

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

Ah, OK.  I obviously misread the proposed commit log message.  And
as a race-fix, this would be good.

Thanks.