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

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 14, 2016, 1:56 a.m.
Message ID <8d3ae769555bd7134bb7.1476410174@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/17080/
State Changes Requested
Headers show

Comments

Pierre-Yves David - Oct. 14, 2016, 1:56 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1471990626 -7200
#      Wed Aug 24 00:17:06 2016 +0200
# Node ID 8d3ae769555bd7134bb70f31b16e67d17726b627
# Parent  678c3cf029eceec20325928cff063ab71ea99761
# EXP-Topic vfs.ward
repovfs: add a ward to check if locks are properly taken

We use the 'ward' feature introduced in the previous email to check for locks
when accessing file in '.hg' for writing. Another changeset will add a 'ward'
for the store vfs (svfs).

This 'ward' system have caught an handful of locking issue that  has been fixed
in previous series. 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.
timeless - Oct. 26, 2016, 7:22 p.m.
Pierre-Yves David wrote:
> +                            # the remainig bit and drop this line

remaining ;-)

-- I really should try to put together a version of my spelling tool
as a test to see what people think.

I'd envision it running w/ a stored cache of allowed words, each run
of the test would generate a new word list, diff it with the old one,
and complain about words that aren't in the old one and aren't in the
user's dictionary. It could also be adjusted to only complain about
words of length {x,y} instead of {1,Inf}...
Mads Kiilerich - Oct. 26, 2016, 7:33 p.m.
On 10/26/2016 09:22 PM, timeless wrote:
> Pierre-Yves David wrote:
>> +                            # the remainig bit and drop this line
> remaining ;-)
>
> -- I really should try to put together a version of my spelling tool
> as a test to see what people think.
>
> I'd envision it running w/ a stored cache of allowed words, each run
> of the test would generate a new word list, diff it with the old one,
> and complain about words that aren't in the old one and aren't in the
> user's dictionary. It could also be adjusted to only complain about
> words of length {x,y} instead of {1,Inf}...

That sounds kind of like 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2012-August/043846.html 
(early version) that I use for patches like 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089492.html 
.

We do however use a lot of weird words so it seems like unless we limit 
the scope a lot, there will inherently be more false hits than genuine 
catches.

/Mads

Patch

diff --git a/hgext/blackbox.py b/hgext/blackbox.py
--- a/hgext/blackbox.py
+++ b/hgext/blackbox.py
@@ -220,6 +220,7 @@  def reposetup(ui, repo):
 
     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 --git a/hgext/journal.py b/hgext/journal.py
--- a/hgext/journal.py
+++ b/hgext/journal.py
@@ -69,6 +69,7 @@  def extsetup(ui):
 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 --git a/hgext/patchbomb.py b/hgext/patchbomb.py
--- a/hgext/patchbomb.py
+++ b/hgext/patchbomb.py
@@ -110,6 +110,10 @@  def uisetup(ui):
     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 --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -244,6 +244,25 @@  class localrepository(object):
     # 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=None, create=False):
         self.requirements = set()
         self.wvfs = scmutil.vfs(path, expandpath=True, realpath=True)
@@ -254,11 +273,15 @@  class localrepository(object):
         self.auditor = pathutil.pathauditor(self.root, self._checknested)
         self.nofsauditor = pathutil.pathauditor(self.root, self._checknested,
                                                 realfs=False)
-        self.vfs = scmutil.vfs(self.path)
-        self.opener = self.vfs
         self.baseui = baseui
         self.ui = baseui.copy()
         self.ui.copy = baseui.copy # prevent copying repo configuration
+        _vfsward = None
+        if (self.ui.configbool('devel', 'all-warnings') or
+            self.ui.configbool('devel', 'check-locks')):
+                _vfsward = self._getvfsward()
+        self.vfs = scmutil.vfs(self.path, ward=_vfsward)
+        self.opener = self.vfs
         # 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
@@ -357,6 +380,36 @@  class localrepository(object):
         # generic mapping between names and nodes
         self.names = namespaces.namespaces()
 
+    def _getvfsward(self):
+        """build a ward for self.vfs"""
+        rref = weakref.ref(self)
+        def checkvfs(f, mode, atomictemp):
+            repo = rref()
+            if (repo is None
+                or not util.safehasattr(repo, '_wlockref')
+                or not util.safehasattr(repo, '_lockref')):
+                return
+            if mode in ('r', 'rb'):
+                return
+            assert f.startswith(repo.path)
+            # truncate name relative to the repository (.hg)
+            relname = f[len(repo.path) + 1:]
+            if relname.startswith('journal.'):
+                # journal is covered by 'lock'
+                if repo._currentlock(repo._lockref) is None:
+                    repo.ui.develwarn('write with no lock: "%s"' % relname,
+                                      stacklevel=2)
+            elif repo._currentlock(repo._wlockref) is None:
+                # rest of vfs file are covered by 'wlock'
+                #
+                # exclude special files
+                for prefix in self._wlockfreeprefix:
+                    if relname.startswith(prefix):
+                        return
+                repo.ui.develwarn('write with no wlock: "%s"' % relname,
+                                  stacklevel=2)
+        return checkvfs
+
     def close(self):
         self._writecaches()
 
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -106,6 +106,7 @@ 
   $ hg add a
   $ hg commit -m a
   $ hg stripintr
+  devel-warn: write with no wlock: "strip-backup/*-backup.hg" at: */mercurial/changegroup.py:* (writechunks) (glob)
   saved backup bundle to $TESTTMP/lock-checker/.hg/strip-backup/*-backup.hg (glob)
   abort: programming error: cannot strip from inside a transaction
   (contact your extension maintainer)