Patchwork [4,of,4] devel: also warn about transaction started without a lock

login
register
mail settings
Submitter Pierre-Yves David
Date March 11, 2015, 4:56 a.m.
Message ID <5cab671ca5e7346e25b7.1426049810@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8002/
State Superseded
Commit 026f8af88e49ae11ef4e5af3acb0c6171c50aaf8
Headers show

Comments

Pierre-Yves David - March 11, 2015, 4:56 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1426046625 25200
#      Tue Mar 10 21:03:45 2015 -0700
# Node ID 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894
# Parent  fb5fc276e7112b1d2b849d42d3502103e95a6d6a
devel: also warn about transaction started without a lock

Nobody should even start a transaction on an unlocked repository. If developer
warning are enabled this will be reported. This use the same config as the bad
locking order as this is closely related.
Ryan McElroy - March 11, 2015, 3:53 p.m.
On 3/10/2015 9:56 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1426046625 25200
> #      Tue Mar 10 21:03:45 2015 -0700
> # Node ID 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894
> # Parent  fb5fc276e7112b1d2b849d42d3502103e95a6d6a
> devel: also warn about transaction started without a lock
>
> Nobody should even start a transaction on an unlocked repository. If developer
> warning are enabled this will be reported. This use the same config as the bad
> locking order as this is closely related.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -896,10 +896,18 @@ class localrepository(object):
>           if tr and tr.running():
>               return tr
>           return None
>   
>       def transaction(self, desc, report=None):
> +        if self.ui.develflag('check-locks'):
> +            l = self._lockref and self._lockref()
> +            if l is None or not l.held:
> +                msg = 'transaction with no lock\n'

No translation because it's a debug statement, even if we print it as an 
error? Why not move the creation of this down to where it's used?

> +                if self.ui.tracebackflag:
> +                    util.debugstacktrace(msg, 2)

This 2 is a disappointing magic number. I have no idea what this line 
does. Is this just something mercurial-y or python-y that is obvious to 
trained eyes? I'd much prefer a comment or something more explicit about 
what you're pulling out here.

> +                else:
> +                    self.ui.write_err(msg)
>           tr = self.currenttransaction()
>           if tr is not None:
>               return tr.nest()
>   
>           # abort here if the journal already exists
> 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
> @@ -8,10 +8,11 @@
>     > cmdtable = {}
>     > command = cmdutil.command(cmdtable)
>     >
>     > @command('buggylocking', [], '')
>     > def buggylocking(ui, repo):
> +  >     tr = repo.transaction('buggy')
>     >     lo = repo.lock()
>     >     wl = repo.wlock()
>     > EOF
>   
>     $ cat << EOF >> $HGRCPATH
> @@ -22,14 +23,16 @@
>     > EOF
>   
>     $ hg init lock-checker
>     $ cd lock-checker
>     $ hg buggylocking
> +  transaction with no lock
>     "lock" taken before "wlock"
>     $ cat << EOF >> $HGRCPATH
>     > [devel]
>     > all=0
>     > check-locks=1
>     > EOF
>     $ hg buggylocking
> +  transaction with no lock
>     "lock" taken before "wlock"
>     $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - March 11, 2015, 4:01 p.m.
On Tue, Mar 10, 2015 at 09:56:50PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1426046625 25200
> #      Tue Mar 10 21:03:45 2015 -0700
> # Node ID 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894
> # Parent  fb5fc276e7112b1d2b849d42d3502103e95a6d6a
> devel: also warn about transaction started without a lock

I don't disagree with Ryan's comments, but wanted to say that I
support this series. I've been meaning to get a buildbot running with
the lock checker enabled for a while, and this is probably a better
solution.

>
> Nobody should even start a transaction on an unlocked repository. If developer
> warning are enabled this will be reported. This use the same config as the bad
> locking order as this is closely related.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -896,10 +896,18 @@ class localrepository(object):
>          if tr and tr.running():
>              return tr
>          return None
>
>      def transaction(self, desc, report=None):
> +        if self.ui.develflag('check-locks'):
> +            l = self._lockref and self._lockref()
> +            if l is None or not l.held:
> +                msg = 'transaction with no lock\n'
> +                if self.ui.tracebackflag:
> +                    util.debugstacktrace(msg, 2)
> +                else:
> +                    self.ui.write_err(msg)
>          tr = self.currenttransaction()
>          if tr is not None:
>              return tr.nest()
>
>          # abort here if the journal already exists
> 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
> @@ -8,10 +8,11 @@
>    > cmdtable = {}
>    > command = cmdutil.command(cmdtable)
>    >
>    > @command('buggylocking', [], '')
>    > def buggylocking(ui, repo):
> +  >     tr = repo.transaction('buggy')
>    >     lo = repo.lock()
>    >     wl = repo.wlock()
>    > EOF
>
>    $ cat << EOF >> $HGRCPATH
> @@ -22,14 +23,16 @@
>    > EOF
>
>    $ hg init lock-checker
>    $ cd lock-checker
>    $ hg buggylocking
> +  transaction with no lock
>    "lock" taken before "wlock"
>    $ cat << EOF >> $HGRCPATH
>    > [devel]
>    > all=0
>    > check-locks=1
>    > EOF
>    $ hg buggylocking
> +  transaction with no lock
>    "lock" taken before "wlock"
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Mads Kiilerich - March 11, 2015, 4:08 p.m.
On 03/11/2015 04:53 PM, Ryan McElroy wrote:
> On 3/10/2015 9:56 PM, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1426046625 25200
>> #      Tue Mar 10 21:03:45 2015 -0700
>> # Node ID 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894
>> # Parent  fb5fc276e7112b1d2b849d42d3502103e95a6d6a
>> devel: also warn about transaction started without a lock
>>
>> Nobody should even start a transaction on an unlocked repository. If 
>> developer
>> warning are enabled this will be reported. This use the same config 
>> as the bad
>> locking order as this is closely related.
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -896,10 +896,18 @@ class localrepository(object):
>>           if tr and tr.running():
>>               return tr
>>           return None
>>         def transaction(self, desc, report=None):
>> +        if self.ui.develflag('check-locks'):
>> +            l = self._lockref and self._lockref()
>> +            if l is None or not l.held:
>> +                msg = 'transaction with no lock\n'
>
> No translation because it's a debug statement, even if we print it as 
> an error? Why not move the creation of this down to where it's used?
>
>> +                if self.ui.tracebackflag:
>> +                    util.debugstacktrace(msg, 2)
>
> This 2 is a disappointing magic number. I have no idea what this line 
> does. Is this just something mercurial-y or python-y that is obvious 
> to trained eyes? I'd much prefer a comment or something more explicit 
> about what you're pulling out here.

I think the number 2 totally makes sense. It is not magic, considering 
what debugstacktrace do according to its docstring. I don't think it is 
necessary to duplicate the information here.

+1 to the series. The lock checker in contrib was not so useful.

The other part of the original lock checker was to hook into opener 
functions and verify that the right locks had been locked first. That is 
probably still not possible to do without gross hacks.

/Mads
Pierre-Yves David - March 11, 2015, 10:16 p.m.
On 03/11/2015 08:53 AM, Ryan McElroy wrote:
> On 3/10/2015 9:56 PM, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1426046625 25200
>> #      Tue Mar 10 21:03:45 2015 -0700
>> # Node ID 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894
>> # Parent  fb5fc276e7112b1d2b849d42d3502103e95a6d6a
>> devel: also warn about transaction started without a lock
>>
>> Nobody should even start a transaction on an unlocked repository. If
>> developer
>> warning are enabled this will be reported. This use the same config as
>> the bad
>> locking order as this is closely related.
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -896,10 +896,18 @@ class localrepository(object):
>>           if tr and tr.running():
>>               return tr
>>           return None
>>       def transaction(self, desc, report=None):
>> +        if self.ui.develflag('check-locks'):
>> +            l = self._lockref and self._lockref()
>> +            if l is None or not l.held:
>> +                msg = 'transaction with no lock\n'
>
> No translation because it's a debug statement, even if we print it as an
> error? Why not move the creation of this down to where it's used?

No translation because the target are Mercurial developers, not users. I 
expect all people writing mercurial code to be able to read, understand 
and grep english sentence.
(I will not make any assumption their ability to write proper english as 
this could backfire quickly)

The message is used twice, so having a temporary variable right before 
the if/else seems perfectly sensible to me.


>> +                if self.ui.tracebackflag:
>> +                    util.debugstacktrace(msg, 2)
>
> This 2 is a disappointing magic number. I have no idea what this line
> does. Is this just something mercurial-y or python-y that is obvious to
> trained eyes? I'd much prefer a comment or something more explicit about
> what you're pulling out here.

This the standard argument of python's traceback method. This tell the 
number of frame that need to be skipped when reporting the traceback. 
This is used to make the traceback conclude to the function that did the 
infamous call. This is standard python'
s stuff and I do not think we should document it. However, you comment 
points out that the --traceback variant is untested and the value is 
wrong (should be 1 since we have less nesting than in the extension)

I'll send a V2 with the right value and proper testing.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -896,10 +896,18 @@  class localrepository(object):
         if tr and tr.running():
             return tr
         return None
 
     def transaction(self, desc, report=None):
+        if self.ui.develflag('check-locks'):
+            l = self._lockref and self._lockref()
+            if l is None or not l.held:
+                msg = 'transaction with no lock\n'
+                if self.ui.tracebackflag:
+                    util.debugstacktrace(msg, 2)
+                else:
+                    self.ui.write_err(msg)
         tr = self.currenttransaction()
         if tr is not None:
             return tr.nest()
 
         # abort here if the journal already exists
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
@@ -8,10 +8,11 @@ 
   > cmdtable = {}
   > command = cmdutil.command(cmdtable)
   > 
   > @command('buggylocking', [], '')
   > def buggylocking(ui, repo):
+  >     tr = repo.transaction('buggy')
   >     lo = repo.lock()
   >     wl = repo.wlock()
   > EOF
 
   $ cat << EOF >> $HGRCPATH
@@ -22,14 +23,16 @@ 
   > EOF
 
   $ hg init lock-checker
   $ cd lock-checker
   $ hg buggylocking
+  transaction with no lock
   "lock" taken before "wlock"
   $ cat << EOF >> $HGRCPATH
   > [devel]
   > all=0
   > check-locks=1
   > EOF
   $ hg buggylocking
+  transaction with no lock
   "lock" taken before "wlock"
   $ cd ..