Re: [PATCH] gc: run pre-detach operations under lock

[Jeff King <peff@peff.net>]

On Wed, Jul 12, 2017 at 09:46:25AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Instead, we can do something a bit simpler: take the lock
> > only for the duration of the pre-detach work, then detach,
> > then take it again for the post-detach work. Technically,
> > this means that the post-detach lock could lose to another
> > process doing pre-detach work. But in the long run this
> > works out.
> 
> You might have found this part gross, but I actually don't.  It
> looks like a reasonable practical compromise, and I tried to think
> of a scenario that this would do a wrong thing but I didn't---it is
> not like we carry information off-disk from the pre-detach to
> post-detach work to cause the latter make decisions on it, so this
> "split into two phrases" looks fairly safe.

Anytime I have to spend a few paragraphs saying "well, it looks like
this might behave terribly, but it doesn't because..." I get worried
that my analysis is missing a case. And that writing it in a way that
avoids that analysis might be safer, even if it's a little more work.

I gave it some more thought after sending the earlier message. And I
really think it's not "a little more work". Even if we decided to keep
the same file and replace the PID in it with the daemonized one, I think
that still isn't quite right. Because we don't do so atomically unless
we take gc.pid.lock again. But we may actually conflict with somebody
else on that! Even though that somebody would just pick up the lock,
read gc.pid and say "well, looks like somebody else is running" and
release it again. So we'd have to either hold the lock the whole time,
or do some kind of retry loop to race with other processes picking up
the lock.

It's definitely possible, but it's fighting an uphill battle against the
way our locking and tempfile code works. So I came to the conclusion
that it's not worth the trouble, and what I posted is probably a good
compromise.

-Peff