Patchwork [2,of,3,fyi] hack: lock validation

login
register
mail settings
Submitter Mads Kiilerich
Date Nov. 17, 2013, 5:53 p.m.
Message ID <ca1828f0c90d81978daa.1384710838@localhost.localdomain>
Download mbox | patch
Permalink /patch/3024/
State Superseded
Headers show

Comments

Mads Kiilerich - Nov. 17, 2013, 5:53 p.m.
# HG changeset patch
# User Mads Kiilerich <mads@kiilerich.com>
# Date 1326326392 -3600
#      Thu Jan 12 00:59:52 2012 +0100
# Branch stable
# Node ID ca1828f0c90d81978daa555fdcdbe841d4fe70b3
# Parent  41d9e7b97f3ced21c0665253a6495165f58a2730
hack: lock validation

Instrumenting critical places with these checks can help verifying compliance
with the locking strategy described on
http://mercurial.selenic.com/wiki/LockingDesign.

Occasionally running the test suite with this patch might catch some locking
errors, but the checking as it is is probably too intrusive for inclusion in
core Mercurial.

Essential parts of this patch had conflicts and has been left out. It should be
verified that the checks are reasonable complete.

A part of this patch now lives (and dies) in contrib/lock-checker.py .
Christian Ebert - Nov. 17, 2013, 6:20 p.m.
* Mads Kiilerich on Sunday, November 17, 2013 at 12:53:58 -0500
> # HG changeset patch
> # User Mads Kiilerich <mads@kiilerich.com>
> # Date 1326326392 -3600
> #      Thu Jan 12 00:59:52 2012 +0100
> # Branch stable
> # Node ID ca1828f0c90d81978daa555fdcdbe841d4fe70b3
> # Parent  41d9e7b97f3ced21c0665253a6495165f58a2730
> hack: lock validation
> 
> Instrumenting critical places with these checks can help verifying compliance
> with the locking strategy described on
> http://mercurial.selenic.com/wiki/LockingDesign.
> 
> Occasionally running the test suite with this patch might catch some locking
> errors, but the checking as it is is probably too intrusive for inclusion in
> core Mercurial.
> 
> Essential parts of this patch had conflicts and has been left out. It should be
> verified that the checks are reasonable complete.
> 
> A part of this patch now lives (and dies) in contrib/lock-checker.py .
> 
> diff --git a/hgext/keyword.py b/hgext/keyword.py
> --- a/hgext/keyword.py
> +++ b/hgext/keyword.py
> @@ -439,7 +439,11 @@ def demo(ui, repo, *args, **opts):
>     repo[None].add([fn])
>     ui.note(_('\nkeywords written to %s:\n') % fn)
>     ui.note(keywords)
> -    repo.dirstate.setbranch('demobranch')
> +    wlock = repo.wlock()
> +    try:
> +        repo.dirstate.setbranch('demobranch')
> +    finally:
> +        wlock.release()
>     for name, cmd in ui.configitems('hooks'):
>         if name.split('.', 1)[0].find('commit') > -1:
>             repo.ui.setconfig('hooks', name, '')

imho the above is unneeded, as this happens in a temporary demo
repo with no precious data, it's part of hg kwdemo and never used
outside that particular command.
Mads Kiilerich - Nov. 17, 2013, 9:04 p.m.
On 11/17/2013 01:20 PM, Christian Ebert wrote:
> * Mads Kiilerich on Sunday, November 17, 2013 at 12:53:58 -0500
>> # HG changeset patch
>> # User Mads Kiilerich <mads@kiilerich.com>
>> # Date 1326326392 -3600
>> #      Thu Jan 12 00:59:52 2012 +0100
>> # Branch stable
>> # Node ID ca1828f0c90d81978daa555fdcdbe841d4fe70b3
>> # Parent  41d9e7b97f3ced21c0665253a6495165f58a2730
>> hack: lock validation
>>
>> Instrumenting critical places with these checks can help verifying compliance
>> with the locking strategy described on
>> http://mercurial.selenic.com/wiki/LockingDesign.
>>
>> Occasionally running the test suite with this patch might catch some locking
>> errors, but the checking as it is is probably too intrusive for inclusion in
>> core Mercurial.
>>
>> Essential parts of this patch had conflicts and has been left out. It should be
>> verified that the checks are reasonable complete.
>>
>> A part of this patch now lives (and dies) in contrib/lock-checker.py .
>>
>> diff --git a/hgext/keyword.py b/hgext/keyword.py
>> --- a/hgext/keyword.py
>> +++ b/hgext/keyword.py
>> @@ -439,7 +439,11 @@ def demo(ui, repo, *args, **opts):
>>      repo[None].add([fn])
>>      ui.note(_('\nkeywords written to %s:\n') % fn)
>>      ui.note(keywords)
>> -    repo.dirstate.setbranch('demobranch')
>> +    wlock = repo.wlock()
>> +    try:
>> +        repo.dirstate.setbranch('demobranch')
>> +    finally:
>> +        wlock.release()
>>      for name, cmd in ui.configitems('hooks'):
>>          if name.split('.', 1)[0].find('commit') > -1:
>>              repo.ui.setconfig('hooks', name, '')
> imho the above is unneeded, as this happens in a temporary demo
> repo with no precious data, it's part of hg kwdemo and never used
> outside that particular command.

Yes, you have made that point before.

I agree that this protocol violation don't have any real impact. But it 
do still violate the locking protocol, and this odd and stupid command 
do thus cause an overhead every time we try to validate that we do 
correct locking. Just fixing it is less overhead than working around it.

/Mads
Christian Ebert - Nov. 18, 2013, 1:44 p.m.
* Mads Kiilerich on Sunday, November 17, 2013 at 16:04:08 -0500
> Yes, you have made that point before.
> 
> I agree that this protocol violation don't have any real impact. But
> it do still violate the locking protocol, and this odd and stupid
> command do thus cause an overhead every time we try to validate that
> we do correct locking. Just fixing it is less overhead than working
> around it.

Sure. Maybe I find the time to move it out of the extension to
contrib - I agree that this would be way cleaner; although then
it will be harder to find for users which found the odd and
stupid command quite useful.

Patch

diff --git a/hgext/keyword.py b/hgext/keyword.py
--- a/hgext/keyword.py
+++ b/hgext/keyword.py
@@ -439,7 +439,11 @@  def demo(ui, repo, *args, **opts):
     repo[None].add([fn])
     ui.note(_('\nkeywords written to %s:\n') % fn)
     ui.note(keywords)
-    repo.dirstate.setbranch('demobranch')
+    wlock = repo.wlock()
+    try:
+        repo.dirstate.setbranch('demobranch')
+    finally:
+        wlock.release()
     for name, cmd in ui.configitems('hooks'):
         if name.split('.', 1)[0].find('commit') > -1:
             repo.ui.setconfig('hooks', name, '')
diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -2481,6 +2481,7 @@  def refresh(ui, repo, *pats, **opts):
     setupheaderopts(ui, opts)
     wlock = repo.wlock()
     try:
+        repo.checkwlock()
         ret = q.refresh(repo, pats, msg=message, **opts)
         q.savedirty()
         return ret
@@ -2575,6 +2576,7 @@  def fold(ui, repo, *files, **opts):
     diffopts = q.patchopts(q.diffopts(), *patches)
     wlock = repo.wlock()
     try:
+        repo.checkwlock()
         q.refresh(repo, msg=message, git=diffopts.git)
         q.delete(repo, patches, opts)
         q.savedirty()
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -8,7 +8,7 @@  import errno
 
 from node import nullid
 from i18n import _
-import scmutil, util, ignore, osutil, parsers, encoding
+import scmutil, util, ignore, osutil, parsers, encoding, ui
 import os, stat, errno, gc
 
 propertycache = util.propertycache
@@ -241,6 +241,7 @@  class dirstate(object):
         return copies
 
     def setbranch(self, branch):
+        if not os.path.lexists(self._join('.hg/wlock')): ui.warnstack()
         self._branch = encoding.fromlocal(branch)
         f = self._opener('branch', 'w', atomictemp=True)
         try:
@@ -497,6 +498,7 @@  class dirstate(object):
         self._dirty = True
 
     def write(self):
+        if not os.path.lexists(self._join('.hg/wlock')): ui.warnstack()
         if not self._dirty:
             return
         st = self._opener("dirstate", "w", atomictemp=True)
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -16,6 +16,7 @@  import tags as tagsmod
 from lock import release
 import weakref, errno, os, time, inspect
 import branchmap
+import ui
 propertycache = util.propertycache
 filecache = scmutil.filecache
 
@@ -244,10 +245,6 @@  class localrepository(object):
         self.sopener = self.svfs
         self.sjoin = self.store.join
         self.vfs.createmode = self.store.createmode
-        self._applyrequirements(requirements)
-        if create:
-            self._writerequirements()
-
 
         self._branchcaches = {}
         self.filterpats = {}
@@ -269,6 +266,10 @@  class localrepository(object):
         # - bookmark changes
         self.filteredrevcache = {}
 
+        self._applyrequirements(requirements)
+        if create:
+            self._writerequirements()
+
     def close(self):
         pass
 
@@ -281,6 +282,7 @@  class localrepository(object):
                                            if r in self.openerreqs)
 
     def _writerequirements(self):
+        #self.checklock()
         reqfile = self.opener("requires", "w")
         for r in sorted(self.requirements):
             reqfile.write("%s\n" % r)
@@ -441,6 +443,7 @@  class localrepository(object):
 
     @unfilteredmethod
     def _tag(self, names, node, message, local, user, date, extra={}):
+        self.checklock()
         if isinstance(names, str):
             names = (names,)
 
@@ -819,6 +822,7 @@  class localrepository(object):
         return self._filter(self._decodefilterpats, filename, data)
 
     def transaction(self, desc, report=None):
+        self.checklock()
         tr = self._transref and self._transref() or None
         if tr and tr.running():
             return tr.nest()
@@ -850,6 +854,7 @@  class localrepository(object):
         return [vfs.join(undoname(x)) for vfs, x in self._journalfiles()]
 
     def _writejournal(self, desc):
+        self.checklock()
         self.opener.write("journal.dirstate",
                           self.opener.tryread("dirstate"))
         self.opener.write("journal.branch",
@@ -1004,8 +1009,8 @@  class localrepository(object):
         except error.LockHeld, inst:
             if not wait:
                 raise
-            self.ui.warn(_("waiting for lock on %s held by %r\n") %
-                         (desc, inst.locker))
+            ui.warnstack(_("%s waiting for %s on %s held by %r\n") %
+                    (os.getpid(), lockname, desc, inst.locker), skip=1)
             # default to 600 seconds timeout
             l = lock.lock(lockname, int(self.ui.config("ui", "timeout", "600")),
                           releasefn, desc=desc)
@@ -1046,6 +1051,11 @@  class localrepository(object):
         self._lockref = weakref.ref(l)
         return l
 
+    def checklock(self, skip=1):
+        l = self._lockref and self._lockref()
+        if l is None or not l.held:
+            ui.warnstack('no lock', skip=skip)
+
     def wlock(self, wait=True):
         '''Lock the non-store parts of the repository (everything under
         .hg except .hg/store) and return a weak reference to the lock.
@@ -1055,7 +1065,16 @@  class localrepository(object):
             l.lock()
             return l
 
+        s = self._lockref and self._lockref()
+        if s is not None and s.held:
+            ui.warnstack('trying to take wlock with lock held', skip=1)
+
         def unlock():
+            s = self._lockref and self._lockref()
+            if s is not None and s.held:
+                ui.warnstack('trying to unlock wlock with lock held',
+                    skip=1)
+
             self.dirstate.write()
             self._filecache['dirstate'].refresh()
 
@@ -1065,6 +1084,11 @@  class localrepository(object):
         self._wlockref = weakref.ref(l)
         return l
 
+    def checkwlock(self, skip=1):
+        l = self._wlockref and self._wlockref()
+        if l is None or not l.held:
+            ui.warnstack('no wlock', skip=skip)
+
     def _filecommit(self, fctx, manifest1, manifest2, linkrev, tr, changelist):
         """
         commit an individual file as part of a larger transaction
diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -8,6 +8,7 @@ 
 import util, error
 import errno, os, socket, time
 import warnings
+import ui
 
 class lock(object):
     '''An advisory lock held by one process to control access to a set
@@ -133,12 +134,13 @@  class lock(object):
         if self.held > 1:
             self.held -= 1
         elif self.held == 1:
-            self.held = 0
+            if not self.held: ui.warnstack('held=%s' % self.held)
             if os.getpid() != self.pid:
                 # we forked, and are not the parent
                 return
             if self.releasefn:
                 self.releasefn()
+            self.held = 0
             try:
                 util.unlink(self.f)
             except OSError:
diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -337,6 +337,7 @@  def createhgrc(path, options):
     hgrc = open(path, 'w')
     hgrc.write('[ui]\n')
     hgrc.write('slash = True\n')
+    hgrc.write('timeout = 5\n')
     hgrc.write('interactive = False\n')
     hgrc.write('[defaults]\n')
     hgrc.write('backout = -d "0 0"\n')
diff --git a/tests/test-commandserver.py.out b/tests/test-commandserver.py.out
--- a/tests/test-commandserver.py.out
+++ b/tests/test-commandserver.py.out
@@ -76,6 +76,7 @@  defaults.commit=-d "0 0"
 defaults.shelve=--date "0 0"
 defaults.tag=-d "0 0"
 ui.slash=True
+ui.timeout=5
 ui.interactive=False
 ui.foo=bar
  runcommand init foo
@@ -85,6 +86,7 @@  defaults.commit=-d "0 0"
 defaults.shelve=--date "0 0"
 defaults.tag=-d "0 0"
 ui.slash=True
+ui.timeout=5
 ui.interactive=False
 
 testing hookoutput: