Patchwork [2,of,3,V4] repovfs: add a ward to check if locks are properly taken

login
register
mail settings
Submitter Boris Feld
Date July 14, 2017, 2:01 p.m.
Message ID <cea0a88164c6d003b43f.1500040909@FB>
Download mbox | patch
Permalink /patch/22333/
State Accepted
Headers show

Comments

Boris Feld - July 14, 2017, 2:01 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1499769497 -7200
#      Tue Jul 11 12:38:17 2017 +0200
# Node ID cea0a88164c6d003b43fe542ec12def2662c1273
# Parent  201d9cbd64a276db278552a8924206afb67b1a4c
# EXP-Topic vfs.ward
repovfs: add a ward to check if locks are properly taken


When the appropriate developer warning are enabled, We wrap 'repo.vfs.audit' to
check for locks when accessing file in '.hg' for writing. Another changeset will
add a 'ward' for the store vfs (svfs).

This check system have caught a handful of locking issues that  has been fixed
in previous series (mostly in 4.0). I expect another batch to be caught in third
party extensions.

We introduces two real exceptions from extensions 'blackbox.log' (because a lot of
read-only operations add entry to it), and 'last-email.txt' (because 'hg email'
is currently a read only operation and there is value to keep it this way).

In addition we are currently allowing bisect to operate outside of the lock
because the current code is a bit hard to get properly locked for now. Multiple
clean up have been made but there is still a couple of them to do and the freeze
is coming.
Yuya Nishihara - July 14, 2017, 2:51 p.m.
On Fri, 14 Jul 2017 16:01:49 +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1499769497 -7200
> #      Tue Jul 11 12:38:17 2017 +0200
> # Node ID cea0a88164c6d003b43fe542ec12def2662c1273
> # Parent  201d9cbd64a276db278552a8924206afb67b1a4c
> # EXP-Topic vfs.ward
> repovfs: add a ward to check if locks are properly taken

Queued, thanks.

> --- a/hgext/patchbomb.py	Tue Jul 11 12:27:58 2017 +0200
> +++ b/hgext/patchbomb.py	Tue Jul 11 12:38:17 2017 +0200
> @@ -122,6 +122,10 @@
>      cmdutil.extraexport.append('pullurl')
>      cmdutil.extraexportmap['pullurl'] = _addpullheader
>  
> +def reposetup(ui, repo):
> +    if not repo.local():
> +        return
> +    repo._wlockfreeprefix.add('last-email.txt')

Nit: It seems wrong to change class variable by reposetup(). Maybe
_wlockfreeprefix should be copied to repo instance.

>  def prompt(ui, prompt, default=None, rest=':'):
>      if default:
> diff -r 201d9cbd64a2 -r cea0a88164c6 mercurial/localrepo.py
> --- a/mercurial/localrepo.py	Tue Jul 11 12:27:58 2017 +0200
> +++ b/mercurial/localrepo.py	Tue Jul 11 12:38:17 2017 +0200
> @@ -289,6 +289,25 @@
>      # only functions defined in module of enabled extensions are invoked
>      featuresetupfuncs = set()
>  
> +    # list of prefix for file which can be written without 'wlock'
> +    # Extensions should extend this list when needed
> +    _wlockfreeprefix = set([# We migh consider requiring 'wlock' for the next
> +                            # two, but pretty much all the existing code assume
> +                            # wlock is not needed so we keep them excluded for
> +                            # now.
> +                            'hgrc',
> +                            'requires',
> +                            # XXX cache is a complicatged business someone
> +                            # should investigate this in depth at some point
> +                            'cache/',
> +                            # XXX shouldn't be dirstate covered by the wlock?
> +                            'dirstate',
> +                            # XXX bisect was still a bit too messy at the time
> +                            # this changeset was introduced. Someone should fix
> +                            # the remainig bit and drop this line
> +                            'bisect.state',
> +                            ])

I've updated it to a set literal.
Yuya Nishihara - July 14, 2017, 3:17 p.m.
On Fri, 14 Jul 2017 16:01:49 +0200, Boris Feld wrote:
> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1499769497 -7200
> #      Tue Jul 11 12:38:17 2017 +0200
> # Node ID cea0a88164c6d003b43fe542ec12def2662c1273
> # Parent  201d9cbd64a276db278552a8924206afb67b1a4c
> # EXP-Topic vfs.ward
> repovfs: add a ward to check if locks are properly taken

> +    def _getvfsward(self, origfunc):
> +        """build a ward for self.vfs"""
> +        rref = weakref.ref(self)
> +        def checkvfs(path, mode=None):
> +            ret = origfunc(path, mode=mode)
> +            repo = rref()
> +            if (repo is None
> +                or not util.safehasattr(repo, '_wlockref')
> +                or not util.safehasattr(repo, '_lockref')):
> +                return
> +            if mode in (None, 'r', 'rb'):
> +                return
> +            if path.startswith(repo.path):
> +                # truncate name relative to the repository (.hg)
> +                path = path[len(repo.path) + 1:]
> +            if path.startswith('journal.'):
> +                # journal is covered by 'lock'
> +                if repo._currentlock(repo._lockref) is None:
> +                    repo.ui.develwarn('write with no lock: "%s"' % path,
> +                                      stacklevel=2)
> +            elif repo._currentlock(repo._wlockref) is None:
> +                # rest of vfs files are covered by 'wlock'
> +                #
> +                # exclude special files
> +                for prefix in self._wlockfreeprefix:
> +                    if path.startswith(prefix):
> +                        return
> +                repo.ui.develwarn('write with no wlock: "%s"' % path,
> +                                  stacklevel=2)
> +            return ret

Nit: this function should do either "return" or "return ret" consistently.

Patch

diff -r 201d9cbd64a2 -r cea0a88164c6 hgext/blackbox.py
--- a/hgext/blackbox.py	Tue Jul 11 12:27:58 2017 +0200
+++ b/hgext/blackbox.py	Tue Jul 11 12:38:17 2017 +0200
@@ -236,6 +236,7 @@ 
 
     if util.safehasattr(ui, 'setrepo'):
         ui.setrepo(repo)
+    repo._wlockfreeprefix.add('blackbox.log')
 
 @command('^blackbox',
     [('l', 'limit', 10, _('the number of events to show')),
diff -r 201d9cbd64a2 -r cea0a88164c6 hgext/journal.py
--- a/hgext/journal.py	Tue Jul 11 12:27:58 2017 +0200
+++ b/hgext/journal.py	Tue Jul 11 12:38:17 2017 +0200
@@ -69,6 +69,7 @@ 
 def reposetup(ui, repo):
     if repo.local():
         repo.journal = journalstorage(repo)
+        repo._wlockfreeprefix.add('namejournal')
 
 def runcommand(orig, lui, repo, cmd, fullargs, *args):
     """Track the command line options for recording in the journal"""
diff -r 201d9cbd64a2 -r cea0a88164c6 hgext/patchbomb.py
--- a/hgext/patchbomb.py	Tue Jul 11 12:27:58 2017 +0200
+++ b/hgext/patchbomb.py	Tue Jul 11 12:38:17 2017 +0200
@@ -122,6 +122,10 @@ 
     cmdutil.extraexport.append('pullurl')
     cmdutil.extraexportmap['pullurl'] = _addpullheader
 
+def reposetup(ui, repo):
+    if not repo.local():
+        return
+    repo._wlockfreeprefix.add('last-email.txt')
 
 def prompt(ui, prompt, default=None, rest=':'):
     if default:
diff -r 201d9cbd64a2 -r cea0a88164c6 mercurial/localrepo.py
--- a/mercurial/localrepo.py	Tue Jul 11 12:27:58 2017 +0200
+++ b/mercurial/localrepo.py	Tue Jul 11 12:38:17 2017 +0200
@@ -289,6 +289,25 @@ 
     # only functions defined in module of enabled extensions are invoked
     featuresetupfuncs = set()
 
+    # list of prefix for file which can be written without 'wlock'
+    # Extensions should extend this list when needed
+    _wlockfreeprefix = set([# We migh consider requiring 'wlock' for the next
+                            # two, but pretty much all the existing code assume
+                            # wlock is not needed so we keep them excluded for
+                            # now.
+                            'hgrc',
+                            'requires',
+                            # XXX cache is a complicatged business someone
+                            # should investigate this in depth at some point
+                            'cache/',
+                            # XXX shouldn't be dirstate covered by the wlock?
+                            'dirstate',
+                            # XXX bisect was still a bit too messy at the time
+                            # this changeset was introduced. Someone should fix
+                            # the remainig bit and drop this line
+                            'bisect.state',
+                            ])
+
     def __init__(self, baseui, path, create=False):
         self.requirements = set()
         self.filtername = None
@@ -308,10 +327,13 @@ 
         self.auditor = pathutil.pathauditor(self.root, self._checknested)
         self.nofsauditor = pathutil.pathauditor(self.root, self._checknested,
                                                 realfs=False)
-        self.vfs = vfsmod.vfs(self.path)
         self.baseui = baseui
         self.ui = baseui.copy()
         self.ui.copy = baseui.copy # prevent copying repo configuration
+        self.vfs = vfsmod.vfs(self.path)
+        if (self.ui.configbool('devel', 'all-warnings') or
+            self.ui.configbool('devel', 'check-locks')):
+                self.vfs.audit = self._getvfsward(self.vfs.audit)
         # A list of callback to shape the phase if no data were found.
         # Callback are in the form: func(repo, roots) --> processed root.
         # This list it to be filled by extension during repo setup
@@ -427,6 +449,38 @@ 
         # Signature to cached matcher instance.
         self._sparsematchercache = {}
 
+    def _getvfsward(self, origfunc):
+        """build a ward for self.vfs"""
+        rref = weakref.ref(self)
+        def checkvfs(path, mode=None):
+            ret = origfunc(path, mode=mode)
+            repo = rref()
+            if (repo is None
+                or not util.safehasattr(repo, '_wlockref')
+                or not util.safehasattr(repo, '_lockref')):
+                return
+            if mode in (None, 'r', 'rb'):
+                return
+            if path.startswith(repo.path):
+                # truncate name relative to the repository (.hg)
+                path = path[len(repo.path) + 1:]
+            if path.startswith('journal.'):
+                # journal is covered by 'lock'
+                if repo._currentlock(repo._lockref) is None:
+                    repo.ui.develwarn('write with no lock: "%s"' % path,
+                                      stacklevel=2)
+            elif repo._currentlock(repo._wlockref) is None:
+                # rest of vfs files are covered by 'wlock'
+                #
+                # exclude special files
+                for prefix in self._wlockfreeprefix:
+                    if path.startswith(prefix):
+                        return
+                repo.ui.develwarn('write with no wlock: "%s"' % path,
+                                  stacklevel=2)
+            return ret
+        return checkvfs
+
     def close(self):
         self._writecaches()
 
diff -r 201d9cbd64a2 -r cea0a88164c6 tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t	Tue Jul 11 12:27:58 2017 +0200
+++ b/tests/test-devel-warnings.t	Tue Jul 11 12:38:17 2017 +0200
@@ -44,6 +44,11 @@ 
   >     wl.release()
   >     lo.release()
   > 
+  > @command(b'no-wlock-write', [], '')
+  > def nowlockwrite(ui, repo):
+  >     with repo.vfs(b'branch', 'a'):
+  >         pass
+  > 
   > @command(b'stripintr', [], '')
   > def stripintr(ui, repo):
   >     lo = repo.lock()
@@ -104,6 +109,11 @@ 
   $ hg properlocking
   $ hg nowaitlocking
 
+Writing without lock
+
+  $ hg no-wlock-write
+  devel-warn: write with no wlock: "branch" at: $TESTTMP/buggylocking.py:* (nowlockwrite) (glob)
+
 Stripping from a transaction
 
   $ echo a > a