| Message ID | pull.1571.git.1691677993195.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 231e86c10c674235cf28447a8486f7955d5f4dd9 |
| Headers | show |
| Series | t4053: avoid race when killing background processes | expand |
"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
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
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
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.
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