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

login
register
mail settings
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

phabricator - June 25, 2020, 7:24 p.m.
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




To: martinvonz, #hg-reviewers
Cc: mercurial-patches, mercurial-devel
Pierre-Yves David - July 16, 2020, 5:41 p.m.
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
>
phabricator - July 16, 2020, 5:49 p.m.
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
Pierre-Yves David - July 16, 2020, 5:56 p.m.
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.
phabricator - July 16, 2020, 8:07 p.m.
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,