Submitter | phabricator |
---|---|
Date | June 25, 2020, 7:24 p.m. |
Message ID | <differential-rev-PHID-DREV-6rgkj52irukxlheyniuf-req@mercurial-scm.org> |
Download | mbox | patch |
Permalink | /patch/46571/ |
State | Superseded |
Headers | show |
Comments
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? On 6/25/20 9:24 PM, martinvonz (Martin von Zweigbergk) wrote: > martinvonz created this revision. > Herald added a reviewer: hg-reviewers. > Herald added a subscriber: mercurial-patches. > > REVISION 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. > > REPOSITORY > rHG Mercurial > > BRANCH > default > > REVISION DETAIL > https://phab.mercurial-scm.org/D8666 > > AFFECTED FILES > mercurial/localrepo.py > > CHANGE DETAILS > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > --- a/mercurial/localrepo.py > +++ b/mercurial/localrepo.py > @@ -1235,8 +1235,9 @@ > if path.startswith(b'cache/'): > msg = b'accessing cache with vfs instead of cachevfs: "%s"' > repo.ui.develwarn(msg % path, stacklevel=3, config=b"cache-vfs") > - if path.startswith(b'journal.') or path.startswith(b'undo.'): > - # journal is covered by 'lock' > + # path prefixes 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: > repo.ui.develwarn( > b'write with no lock: "%s"' % path, > > > > To: martinvonz, #hg-reviewers > Cc: mercurial-patches, mercurial-devel > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
martinvonz added a comment. In D8666#130894 <https://phab.mercurial-scm.org/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? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D8666/new/ REVISION DETAIL https://phab.mercurial-scm.org/D8666 To: martinvonz, #hg-reviewers, durin42, pulkit Cc: marmoute, mercurial-devel, durin42, mercurial-patches
On 7/16/20 7:49 PM, martinvonz (Martin von Zweigbergk) wrote: > martinvonz added a comment. > > > In D8666#130894 <https://phab.mercurial-scm.org/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? I think that would be it (not necessary extension since we have core command creating backup, but they might old the lock). However regarding writing the file either we are using atomictemp and we should be fine or we are using direct write in which case the result would be giberish. In practice I suspect most command doing a strip will probably be holding the lock (since they touch the changelog too), so we are probably fine.
martinvonz added a comment. 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. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D8666/new/ REVISION DETAIL https://phab.mercurial-scm.org/D8666 To: martinvonz, #hg-reviewers, durin42, pulkit Cc: marmoute, mercurial-devel, durin42, mercurial-patches
Patch
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -1235,8 +1235,9 @@ if path.startswith(b'cache/'): msg = b'accessing cache with vfs instead of cachevfs: "%s"' repo.ui.develwarn(msg % path, stacklevel=3, config=b"cache-vfs") - if path.startswith(b'journal.') or path.startswith(b'undo.'): - # journal is covered by 'lock' + # path prefixes 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: repo.ui.develwarn( b'write with no lock: "%s"' % path,