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

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 11, 2016, 10:52 a.m.
Message ID <dd18fd65bc8322a8bbac.1470912745@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16249/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Pierre-Yves David - Aug. 11, 2016, 10:52 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1470606246 -7200
#      Sun Aug 07 23:44:06 2016 +0200
# Node ID dd18fd65bc8322a8bbacf2b165228b843ba849ae
# Parent  b84b218971cfb131023b7e9550cb112e6195841b
# 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 writting. 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 introduce two 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).
timeless - Aug. 12, 2016, 12:59 a.m.
Pierre-Yves David wrote:
> We use the 'ward' feature introduced in the previous email to check for locks

`email` is wrong, sure I'm reading it in an email, but everyone else
is seeing it in a Commit message in the HG log...

> when accessing file in '.hg' for writting. Another changeset will add a 'ward'

a file | files

> This 'ward' system have caught an handful of locking issue that  has been fixed

has caught a handful ... that have been
Yuya Nishihara - Aug. 14, 2016, 3:20 a.m.
On Thu, 11 Aug 2016 12:52:25 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1470606246 -7200
> #      Sun Aug 07 23:44:06 2016 +0200
> # Node ID dd18fd65bc8322a8bbacf2b165228b843ba849ae
> # Parent  b84b218971cfb131023b7e9550cb112e6195841b
> # 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 writting. 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 introduce two 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).

Maybe patchbomb should use atomictemp to write last-email.txt without locking.

> --- a/mercurial/localrepo.py	Thu Aug 04 17:07:46 2016 +0200
> +++ b/mercurial/localrepo.py	Sun Aug 07 23:44:06 2016 +0200
> @@ -244,6 +244,16 @@ 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',
> +                            ])
> +
>      def __init__(self, baseui, path=None, create=False):
>          self.requirements = set()
>          self.wvfs = scmutil.vfs(path, expandpath=True, realpath=True)
> @@ -254,11 +264,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 +371,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):
> +            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

This weakref stuff smells like a layering violation. If vfs should be fully
covered by a lock, I think the lock should be managed by vfs.
Pierre-Yves David - Aug. 14, 2016, 12:08 p.m.
On 08/14/2016 05:20 AM, Yuya Nishihara wrote:
> On Thu, 11 Aug 2016 12:52:25 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1470606246 -7200
>> #      Sun Aug 07 23:44:06 2016 +0200
>> # Node ID dd18fd65bc8322a8bbacf2b165228b843ba849ae
>> # Parent  b84b218971cfb131023b7e9550cb112e6195841b
>> # 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 writting. 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 introduce two 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).
>
> Maybe patchbomb should use atomictemp to write last-email.txt without locking.

That's a good idea. Will still need some exception if we get tempfile 
covered.

>> --- a/mercurial/localrepo.py	Thu Aug 04 17:07:46 2016 +0200
>> +++ b/mercurial/localrepo.py	Sun Aug 07 23:44:06 2016 +0200
>> @@ -244,6 +244,16 @@ 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',
>> +                            ])
>> +
>>      def __init__(self, baseui, path=None, create=False):
>>          self.requirements = set()
>>          self.wvfs = scmutil.vfs(path, expandpath=True, realpath=True)
>> @@ -254,11 +264,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 +371,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):
>> +            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
>
> This weakref stuff smells like a layering violation. If vfs should be fully
> covered by a lock, I think the lock should be managed by vfs.

I do not think vfs should handled the lock. locks cover multiple vfs and 
multiple locks can be relevant for a vfs. Also vfs semantic is about 
file system access, not locking and transaction logic. That is something 
higher level.

The ward is some developer tool that step over the boundary a bit to get 
its job done, it stay disabled in normal mode. In addition, all the 
logic of the ward stay defined at the repository level, where the rest 
of the locking logic stay. The vfs stay ignorant of the upper tier 
logic. The ward could be used to prevent curse word in filename if would 
work all the same. Actually, another step that would be useful to takes 
with ward is to split a dedicated vfs for operation on file that are 
only relevant for the working copy, that would help the shared extensions.

Cheers,

Patch

diff -r b84b218971cf -r dd18fd65bc83 hgext/blackbox.py
--- a/hgext/blackbox.py	Thu Aug 04 17:07:46 2016 +0200
+++ b/hgext/blackbox.py	Sun Aug 07 23:44:06 2016 +0200
@@ -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 -r b84b218971cf -r dd18fd65bc83 hgext/patchbomb.py
--- a/hgext/patchbomb.py	Thu Aug 04 17:07:46 2016 +0200
+++ b/hgext/patchbomb.py	Sun Aug 07 23:44:06 2016 +0200
@@ -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 -r b84b218971cf -r dd18fd65bc83 mercurial/localrepo.py
--- a/mercurial/localrepo.py	Thu Aug 04 17:07:46 2016 +0200
+++ b/mercurial/localrepo.py	Sun Aug 07 23:44:06 2016 +0200
@@ -244,6 +244,16 @@  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',
+                            ])
+
     def __init__(self, baseui, path=None, create=False):
         self.requirements = set()
         self.wvfs = scmutil.vfs(path, expandpath=True, realpath=True)
@@ -254,11 +264,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 +371,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):
+            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 -r b84b218971cf -r dd18fd65bc83 tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t	Thu Aug 04 17:07:46 2016 +0200
+++ b/tests/test-devel-warnings.t	Sun Aug 07 23:44:06 2016 +0200
@@ -105,6 +105,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)