Page MenuHomePhabricator

locks: expect repo lock, not wlock, when writing to .hg/strip-backup/
ClosedPublic

Authored by martinvonz on Jun 25 2020, 3:24 PM.

Details

Summary

There should be no need for a working copy lock when creating (or
reading) bundles in .hg/strip-backup/ since they don't affect the
working copy.

I noticed this because we have an extension that tries to strip some
revisions while holding only a repo lock. I guess we have no such
cases in core, which seems a bit surprising. Maybe we always take a
wlock at a higher level so the working copy is not updated while the
target commit is being stripped.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Jun 25 2020, 3:24 PM
durin42 accepted this revision as: durin42.Jun 25 2020, 3:34 PM
durin42 added a subscriber: durin42.

My feelings on this are "obviously yes" but I thought that so quickly I lack confidence that I'm not missing something. :)

pulkit accepted this revision.Jun 29 2020, 4:40 AM
This revision is now accepted and ready to land.Jun 29 2020, 4:40 AM

I am a bit late to the party, but does this mean that and old client and
a new client could do conflicting write to strip backup because they
would be respectively holding wlock and lock, thinking that make them safe?

I am a bit late to the party, but does this mean that and old client and
a new client could do conflicting write to strip backup because they
would be respectively holding wlock and lock, thinking that make them safe?

Yes, if you have an old client with an extension that strips while holding wlock, and a new client with an extension that strips while holding a lock, then you might be in a bit of trouble if you run them at the same time. I can't think of a case where that trouble wouldn't be very limited, though, since the strip bundles are content-addressed, so it seems the only case is if they both try to write the same bundle. In that case, one of them would fail to open the file for writes and would presumably crash. All in all, that seems extremely unlikely. Is there a more common case I'm missing?

From Pierre-Yves (probably didn't make it here because the reply didn't have the link or something):

I think that would be it (not necessary extension since we have core
command creating backup, but they might old the lock).

I suspect that core always grabs both locks and that's why we had not seen the warning in core.