This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8666

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

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

Details

Reviewers
durin42
pulkit
Group Reviewers
hg-reviewers
Commits
rHG5797dbb630df: locks: expect repo lock, not wlock, when writing to .hg/strip-backup/
rHG27f24dd2a45d: locks: expect repo lock, not wlock, when writing to .hg/strip-backup/
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
marmoute added subscribers: mercurial-devel, marmoute.Jul 16 2020, 1:42 PM

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?

martinvonz added a comment.Jul 16 2020, 1:49 PM
In D8666#130894, @marmoute wrote:

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?

martinvonz added a comment.Jul 16 2020, 4:07 PM

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.

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (5 lines)

Diff 21729

mercurial/localrepo.py

if mode in (None, b'r', b'rb'): if mode in (None, b'r', b'rb'):
return return
if path.startswith(repo.path): if path.startswith(repo.path):
# truncate name relative to the repository (.hg) # truncate name relative to the repository (.hg)
path = path[len(repo.path) + 1 :] path = path[len(repo.path) + 1 :]
if path.startswith(b'cache/'): if path.startswith(b'cache/'):
msg = b'accessing cache with vfs instead of cachevfs: "%s"' msg = b'accessing cache with vfs instead of cachevfs: "%s"'
repo.ui.develwarn(msg % path, stacklevel=3, config=b"cache-vfs") repo.ui.develwarn(msg % path, stacklevel=3, config=b"cache-vfs")
if path.startswith(b'journal.') or path.startswith(b'undo.'): # path prefixes covered by 'lock'
# journal is covered by 'lock' vfs_path_prefixes = (b'journal.', b'undo.', b'strip-backup/')
if any(path.startswith(prefix) for prefix in vfs_path_prefixes):
if repo._currentlock(repo._lockref) is None: if repo._currentlock(repo._lockref) is None:
repo.ui.develwarn( repo.ui.develwarn(
b'write with no lock: "%s"' % path, b'write with no lock: "%s"' % path,
stacklevel=3, stacklevel=3,
config=b'check-locks', config=b'check-locks',
) )
elif repo._currentlock(repo._wlockref) is None: elif repo._currentlock(repo._wlockref) is None:
# rest of vfs files are covered by 'wlock' # rest of vfs files are covered by 'wlock'