Re: [PATCH] t4053: avoid race when killing background processes
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Aug 11, 2023, 09:56 UTC
- Message-ID
- <2d9e9b59-bf78-8470-0731-3cd7fe15a727@gmail.com>
- In-Reply-To
- <xmqqedkassng.fsf@gitster.g>
On 10/08/2023 18:40, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
Show 8 quoted lines
> 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
Show 11 quoted lines
> 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...
Show 27 quoted 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(-)
>>
>> 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