Patchwork [14,of,14] reposvfs: add a ward to check if locks are properly taken

login
register
mail settings
Submitter Pierre-Yves David
Date July 2, 2017, 2:56 a.m.
Message ID <a3c809a23ca3b90105e2.1498964199@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/21891/
State Accepted
Headers show

Comments

Pierre-Yves David - July 2, 2017, 2:56 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1470672882 -7200
#      Mon Aug 08 18:14:42 2016 +0200
# Node ID a3c809a23ca3b90105e2156d30dc2700488598dd
# Parent  4ef6ab01026fcb12b02f9224140e5e42cff56d61
# EXP-Topic vfs.ward
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r a3c809a23ca3
reposvfs: add a ward to check if locks are properly taken

We use the 'ward' to check for the store lock when accessing file in '.hg/store'
for writing. This caught a couple of instance where the transaction was released
after the lock, we should probably have a dedicated checker for that case.
Sean Farley - July 5, 2017, 3:31 a.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1470672882 -7200
> #      Mon Aug 08 18:14:42 2016 +0200
> # Node ID a3c809a23ca3b90105e2156d30dc2700488598dd
> # Parent  4ef6ab01026fcb12b02f9224140e5e42cff56d61
> # EXP-Topic vfs.ward
> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r a3c809a23ca3
> reposvfs: add a ward to check if locks are properly taken
>
> We use the 'ward' to check for the store lock when accessing file in '.hg/store'
> for writing. This caught a couple of instance where the transaction was released
> after the lock, we should probably have a dedicated checker for that case.

In general, I like this cleanup and new ward feature. Though, I'd like
if Yuya or others more familiar with vfs could take a second look. I've
marked it as pre-reviewed in patchwork.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -389,8 +389,14 @@  class localrepository(object):
             if inst.errno != errno.ENOENT:
                 raise
 
+        getsvfs = vfsmod.vfs
+        if (self.ui.configbool('devel', 'all-warnings') or
+            self.ui.configbool('devel', 'check-locks')):
+                def getsvfs(*args, **kwargs):
+                    kwargs['ward'] = self._getsvfsward()
+                    return vfsmod.vfs(*args, **kwargs)
         self.store = store.store(
-                self.requirements, self.sharedpath, vfsmod.vfs)
+                self.requirements, self.sharedpath, getsvfs)
         self.spath = self.store.path
         self.svfs = self.store.vfs
         self.sjoin = self.store.join
@@ -458,6 +464,23 @@  class localrepository(object):
                                   stacklevel=2)
         return checkvfs
 
+    def _getsvfsward(self):
+        """build a ward for self.svfs"""
+        rref = weakref.ref(self)
+        def checksvfs(f, mode, atomictemp):
+            repo = rref()
+            if repo is None or not util.safehasattr(repo, '_lockref'):
+                return
+            if mode in ('r', 'rb'):
+                return
+            assert f.startswith(repo.sharedpath), (repo.path, f)
+            # truncate name relative to the repository (.hg)
+            relname = f[len(repo.sharedpath) + 1:]
+            if repo._currentlock(repo._lockref) is None:
+                repo.ui.develwarn('write with no lock: "%s"' % relname,
+                                  stacklevel=3)
+        return checksvfs
+
     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
@@ -49,6 +49,11 @@ 
   >     with repo.vfs(b'branch', 'a'):
   >         pass
   > 
+  > @command(b'no-lock-write', [], '')
+  > def nolockwrite(ui, repo):
+  >     with repo.svfs(b'fncache', 'a'):
+  >         pass
+  > 
   > @command(b'stripintr', [], '')
   > def stripintr(ui, repo):
   >     lo = repo.lock()
@@ -114,6 +119,9 @@  Writing without lock
   $ hg no-wlock-write
   devel-warn: write with no wlock: "branch" at: $TESTTMP/buggylocking.py:* (nowlockwrite) (glob)
 
+  $ hg no-lock-write
+  devel-warn: write with no lock: "store/fncache" at: $TESTTMP/buggylocking.py:* (nolockwrite) (glob)
+
 Stripping from a transaction
 
   $ echo a > a