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

From: Jeff King <peff@peff.net>
Date: Jul 12, 2017, 16:58 UTC
Message-ID: <20170712165817.xcq4we5ynl3opm37@sigill.intra.peff.net>
In-Reply-To: <xmqqvamx1u3i.fsf@gitster.mtv.corp.google.com>

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

Show 15 quoted lines

> 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