Re: [PATCH v2] receive-pack: fix stale packfile locks when dying
- From
Jeff King <peff@peff.net>
- Date
- Mar 10, 2023, 13:20 UTC
- Message-ID
- <ZAsupPsPPiRnEs4h@coredump.intra.peff.net>
- In-Reply-To
- <ZAsZMAhgag1iBSK5@ncase>
On Fri, Mar 10, 2023 at 12:49:04PM +0100, Patrick Steinhardt wrote:
Show 21 quoted lines
> > There's no test here, and I think we _could_ make a reliable one with > > something like: > > > > 1. Have a proc-receive hook that signals via fifo that it's running, > > then pauses indefinitely. > > > > 2. Start a push in the background, then wait on the fifo signal. > > > > 3. Kill receive-pack. > > > > But it's awfully complicated for little gain. I'm fine with not worrying > > about it (and I did something similar manually to make to sure it works > > as we expect). > > I actually tried doing exactly what you're proposing here yesterday. It > caused hangs, lingering processes, you had to play games to not trigger > the &&-chain linter due to the backgrounding via &, and ultimately it > somehow just didn't work. > > So at some point I came to the same conclusion as you did, that it's > just too complicated for little gain.
So I don't think we should apply this, because it's horrific. But because I like a challenge, here is a test that should be race-free, doesn't hang, and doesn't leave lingering processes (the blocking hook waits for its parent to die).
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh index 3f81f16e133..ca6950ef433 100755 --- a/t/t5400-send-pack.sh +++ b/t/t5400-send-pack.sh @@ -300,4 +300,37 @@ test_expect_success 'receive-pack de-dupes .have lines' ' test_cmp expect refs ' +test_expect_success PIPE 'receive-pack cleans up .keep after signal' ' + git init --bare keep.git && + write_script keep.git/hooks/proc-receive <<-\EOF && + # tell the other side we are in the hook + echo running proc-receive >&9 + # and then wait until our parent receive-pack exits + wait "$PPID" + EOF + git -C keep.git config receive.procReceiveRefs refs/ && + + mkfifo rpstatus && + ( + # this will fail because we kill receive-pack while it hangs, + # but we will not see its exit code until we "wait" below. + git push \ + --receive-pack="echo \$\$ >&9; exec git-receive-pack" \ + --all keep.git 9>rpstatus & + ) && + + exec 9<rpstatus && + read <&9 rp_pid && + read <&9 ready_signal && + kill -15 "$rp_pid" && + # in theory we get the signal code from receive-pack here, + # but we are racing with send-pack to call wait(), so we + # might also get "not found" from wait + { wait "$rp_pid"; OUT=$?; } && + { test_match_signal 15 $OUT || test "$OUT" = 127; } && + + find keep.git/objects/pack -name "*.keep" >actual && + test_must_be_empty actual +' + test_done It fails as expected without your patch (the .keep file is left in "actual"), and passes with it. -Peff