{"thread":{"id":"60099","subject":"[PATCH] t4053: avoid race when killing background processes","startedAt":"2023-08-10T14:33:19Z","lastAt":"2023-08-11T15:47:37Z","messageCount":5,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Phillip Wood","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480436","messageId":"pull.1571.git.1691677993195.gitgitgadget@gmail.com","threadId":"60099","inReplyTo":null,"subject":"[PATCH] t4053: avoid race when killing background processes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T14:33:13Z","receivedAt":"2023-08-10T14:33:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nThe test 'diff --no-index reads from pipes' starts a couple of\nbackground processes that write to the pipes that are passed to \"diff\n--no-index\". If the test passes then we expect these processes to exit\nas all their output will have been read. However if the test fails\nthen we want to make sure they do not hang about on the users machine\nand the test remembers they should be killed by calling\n\n      test_when_finished  \"! kill $!\"\n\nafter each background process is created. Unfortunately there is a\nrace where test_when_finished may run before the background process\nexits even when all its output has been read resulting in the kill\ncommand succeeding which causes the test to fail. Fix this by ignoring\nthe exit status of the kill command. If the diff is successful we\ncould instead wait for the background process to exit and check their\nstatus but that feels like it is testing the platform's printf\nimplementation rather than git's code.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    t4053: avoid race when killing background processes\n    \n    Thanks to Peff for reporting this. Junio - this fixes a regression\n    introduced in the current cycle. It is fairly minor though so I'm not\n    sure if you want to pick it up now or wait until 2.42.0 is out.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1571%2Fphillipwood%2Fdiff-no-index-pipes-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1571/phillipwood/diff-no-index-pipes-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1571\n\n t/t4053-diff-no-index.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex a28b9ff2434..1fb7d334620 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -248,11 +248,11 @@ test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n \t{\n \t\t(test_write_lines a b c >old) &\n \t} &&\n-\ttest_when_finished \"! kill $!\" &&\n+\ttest_when_finished \"kill $! || :\" &&\n \t{\n \t\t(test_write_lines a x c >new) &\n \t} &&\n-\ttest_when_finished \"! kill $!\" &&\n+\ttest_when_finished \"kill $! || :\" &&\n \n \tcat >expect <<-EOF &&\n \tdiff --git a/old b/new-link\n\nbase-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\n-- \ngitgitgadget\n"},{"id":"480460","messageId":"xmqqedkassng.fsf@gitster.g","threadId":"60099","inReplyTo":"pull.1571.git.1691677993195.gitgitgadget@gmail.com","subject":"Re: [PATCH] t4053: avoid race when killing background processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T17:40:03Z","receivedAt":"2023-08-10T17:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> The test 'diff --no-index reads from pipes' starts a couple of\n> background processes that write to the pipes that are passed to \"diff\n> --no-index\". If the test passes then we expect these processes to exit\n> as all their output will have been read. However if the test fails\n> then we want to make sure they do not hang about on the users machine\n> and the test remembers they should be killed by calling\n>\n>       test_when_finished  \"! kill $!\"\n>\n> after each background process is created. Unfortunately there is a\n> race where test_when_finished may run before the background process\n> exits even when all its output has been read resulting in the kill\n> command succeeding which causes the test to fail. Fix this by ignoring\n> the exit status of the kill command. If the diff is successful we\n> could instead wait for the background process to exit and check their\n> status but that feels like it is testing the platform's printf\n> implementation rather than git's code.\n>\n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>     t4053: avoid race when killing background processes\n>     \n>     Thanks to Peff for reporting this. Junio - this fixes a regression\n>     introduced in the current cycle. It is fairly minor though so I'm not\n>     sure if you want to pick it up now or wait until 2.42.0 is out.\n\nThis did not cut -rc1 that was scheduled for yesterday, but a fix\nfor a new test added during the cycle is a very welcome addition.\n\nWhile I can see that \"kill\" in the when-finished handler may or may\nnot find the backgrounded process by the time it is run, and\nignoring its exit status (hence keeping test_when_finished happy)\nwould be a reasonable thing to do.  I can understand if this patch\nis to fix a different symptom, namely, when-finished handler\nsometimes fails and makes the test fail.\n\nBut I am not sure how this causes the test to \"hang\", which\npresumably is a symptom that somebody is trying to read from\na pipe that nobody is making progress to write into?  We will\nsend a signal either way to the writers, and the only difference is\nthat we ignore the exit code.\n\nGranted, when-finished handlers are concatenated with \"&&-\", and one\n\"kill\"s failure will cause the other \"kill\" not to run, so we may\nsend a signal to only one but not to the other, but that should all\nhappen after \"diff --no-index\" returns, so it still does not explain\nthe \"hang\".\n\nPuzzled...\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1571%2Fphillipwood%2Fdiff-no-index-pipes-fixes-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1571/phillipwood/diff-no-index-pipes-fixes-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1571\n>\n>  t/t4053-diff-no-index.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n> index a28b9ff2434..1fb7d334620 100755\n> --- a/t/t4053-diff-no-index.sh\n> +++ b/t/t4053-diff-no-index.sh\n> @@ -248,11 +248,11 @@ test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n>  \t{\n>  \t\t(test_write_lines a b c >old) &\n>  \t} &&\n> -\ttest_when_finished \"! kill $!\" &&\n> +\ttest_when_finished \"kill $! || :\" &&\n>  \t{\n>  \t\t(test_write_lines a x c >new) &\n>  \t} &&\n> -\ttest_when_finished \"! kill $!\" &&\n> +\ttest_when_finished \"kill $! || :\" &&\n>  \n>  \tcat >expect <<-EOF &&\n>  \tdiff --git a/old b/new-link\n>\n> base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\n"},{"id":"480528","messageId":"2d9e9b59-bf78-8470-0731-3cd7fe15a727@gmail.com","threadId":"60099","inReplyTo":"xmqqedkassng.fsf@gitster.g","subject":"Re: [PATCH] t4053: avoid race when killing background processes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-11T09:56:40Z","receivedAt":"2023-08-11T09:56:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/08/2023 18:40, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n >\n> While I can see that \"kill\" in the when-finished handler may or may\n> not find the backgrounded process by the time it is run, and\n> ignoring its exit status (hence keeping test_when_finished happy)\n> would be a reasonable thing to do.  I can understand if this patch\n> is to fix a different symptom, namely, when-finished handler\n> sometimes fails and makes the test fail.\n> \n> But I am not sure how this causes the test to \"hang\", \n\n\nThis is only a fix for the test failure that Peff saw when running with \n--stress.\n\n> which\n> presumably is a symptom that somebody is trying to read from\n> a pipe that nobody is making progress to write into?\n\nThat or a process blocking when tying to open a fifo seems the mostly \nlikely cause but I can't see where that is happening. As you say this \npatch does not obviously change anything that would be causing the test \nto hang.\n\nBest Wishes\n\nPhillip\n\n>  We will\n> send a signal either way to the writers, and the only difference is\n> that we ignore the exit code.\n> \n> Granted, when-finished handlers are concatenated with \"&&-\", and one\n> \"kill\"s failure will cause the other \"kill\" not to run, so we may\n> send a signal to only one but not to the other, but that should all\n> happen after \"diff --no-index\" returns, so it still does not explain\n> the \"hang\".\n> \n> Puzzled...\n\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1571%2Fphillipwood%2Fdiff-no-index-pipes-fixes-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1571/phillipwood/diff-no-index-pipes-fixes-v1\n>> Pull-Request: https://github.com/gitgitgadget/git/pull/1571\n>>\n>>   t/t4053-diff-no-index.sh | 4 ++--\n>>   1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\n>> index a28b9ff2434..1fb7d334620 100755\n>> --- a/t/t4053-diff-no-index.sh\n>> +++ b/t/t4053-diff-no-index.sh\n>> @@ -248,11 +248,11 @@ test_expect_success PIPE,SYMLINKS 'diff --no-index reads from pipes' '\n>>   \t{\n>>   \t\t(test_write_lines a b c >old) &\n>>   \t} &&\n>> -\ttest_when_finished \"! kill $!\" &&\n>> +\ttest_when_finished \"kill $! || :\" &&\n>>   \t{\n>>   \t\t(test_write_lines a x c >new) &\n>>   \t} &&\n>> -\ttest_when_finished \"! kill $!\" &&\n>> +\ttest_when_finished \"kill $! || :\" &&\n>>   \n>>   \tcat >expect <<-EOF &&\n>>   \tdiff --git a/old b/new-link\n>>\n>> base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\n"},{"id":"480537","messageId":"20230811144403.GA2303200@coredump.intra.peff.net","threadId":"60099","inReplyTo":"2d9e9b59-bf78-8470-0731-3cd7fe15a727@gmail.com","subject":"Re: [PATCH] t4053: avoid race when killing background processes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-11T14:44:03Z","receivedAt":"2023-08-11T14:44:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 11, 2023 at 10:56:40AM +0100, Phillip Wood wrote:\n\n> This is only a fix for the test failure that Peff saw when running with\n> --stress.\n> \n> > which\n> > presumably is a symptom that somebody is trying to read from\n> > a pipe that nobody is making progress to write into?\n> \n> That or a process blocking when tying to open a fifo seems the mostly likely\n> cause but I can't see where that is happening. As you say this patch does\n> not obviously change anything that would be causing the test to hang.\n\nYeah, sorry if my initial report was unclear. It was while digging for\nthe cause of the hang that I ran into the racy failures. This patch\nlooks good for fixing the race. I'm still waiting to hit the hang again\nto get more data. But if it never happens again, then that is a success\nof sorts. :)\n\n-Peff\n"},{"id":"480546","messageId":"xmqqsf8poa28.fsf@gitster.g","threadId":"60099","inReplyTo":"2d9e9b59-bf78-8470-0731-3cd7fe15a727@gmail.com","subject":"Re: [PATCH] t4053: avoid race when killing background processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-11T15:47:27Z","receivedAt":"2023-08-11T15:47:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 10/08/2023 18:40, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>\n>> While I can see that \"kill\" in the when-finished handler may or may\n>> not find the backgrounded process by the time it is run, and\n>> ignoring its exit status (hence keeping test_when_finished happy)\n>> would be a reasonable thing to do.  I can understand if this patch\n>> is to fix a different symptom, namely, when-finished handler\n>> sometimes fails and makes the test fail.\n>> But I am not sure how this causes the test to \"hang\", \n>\n>\n> This is only a fix for the test failure that Peff saw when running\n> with --stress.\n\nAh, OK.  I obviously misread the proposed commit log message.  And\nas a race-fix, this would be good.\n\nThanks.\n"}]}