Patchwork journal: properly check for held lock (issue5349)

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1473791419 -7200
#      Tue Sep 13 20:30:19 2016 +0200
# Node ID 61784f683c494f547565122716bdb2234d5360ae
# Parent  be16091ac14d03f3cc038b2fb26efe46f785f8d7
# EXP-Topic pypy.journal
journal: properly check for held lock (issue5349)

The 'jlock' code meant to check for a held lock, but it actually just checking for a
lock object. With CPython, this worked because the 'jlock' object is not
referenced outside the '_write' function so reference counting would garbage
collect it and the '_lockref' would return None. With pypy, the garbage
collection would happen at an undefined time and the '_lockref' can still point
to a 'jlock' object outside of '_write'.

The right thing to do here is not only to check for a lock object but also to
check if the lock is held. We update the code to do so and reuse a utility
method that exist on 'localrepo' to help readability. This fix journal related
tests with pypy.

On Tue, Sep 13, 2016 at 08:39:30PM +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1473791419 -7200
> #      Tue Sep 13 20:30:19 2016 +0200
> # Node ID 61784f683c494f547565122716bdb2234d5360ae
> # Parent  be16091ac14d03f3cc038b2fb26efe46f785f8d7
> # EXP-Topic pypy.journal
> journal: properly check for held lock (issue5349)

Queued, but please see a comment below.

[...]

> diff --git a/hgext/journal.py b/hgext/journal.py
> --- a/hgext/journal.py
> +++ b/hgext/journal.py
> @@ -267,9 +267,21 @@ class journalstorage(object):
>          # with a non-local repo (cloning for example).
>          cls._currentcommand = fullargs
>
> +    def _currentlock(self, lockref):

Why is this method inside the journalstorage class? Could it not be at
module-level and therefore invite fewer questions about its
implementation when being read?

(It also strikes me this is probably a pattern we need to employ on
other weakrefs to locks, so there's probably room for some sort of
lockutil package eventually.)

> +        """Returns the lock if it's held, or None if it's not.
> +
> +        (This is copied from the localrepo class)
> +        """
> +        if lockref is None:
> +            return None
> +        l = lockref()
> +        if l is None or not l.held:
> +            return None
> +        return l
> +
>      def jlock(self, vfs):
>          """Create a lock for the journal file"""
> -        if self._lockref and self._lockref():
> +        if self._currentlock(self._lockref) is not None:
>              raise error.Abort(_('journal lock does not support nesting'))
>          desc = _('journal of %s') % vfs.base
>          try:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On 09/13/2016 08:54 PM, Augie Fackler wrote:
> On Tue, Sep 13, 2016 at 08:39:30PM +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1473791419 -7200
>> #      Tue Sep 13 20:30:19 2016 +0200
>> # Node ID 61784f683c494f547565122716bdb2234d5360ae
>> # Parent  be16091ac14d03f3cc038b2fb26efe46f785f8d7
>> # EXP-Topic pypy.journal
>> journal: properly check for held lock (issue5349)
>
> Queued, but please see a comment below.
>
> [...]
>
>> diff --git a/hgext/journal.py b/hgext/journal.py
>> --- a/hgext/journal.py
>> +++ b/hgext/journal.py
>> @@ -267,9 +267,21 @@ class journalstorage(object):
>>          # with a non-local repo (cloning for example).
>>          cls._currentcommand = fullargs
>>
>> +    def _currentlock(self, lockref):
>
> Why is this method inside the journalstorage class? Could it not be at
> module-level and therefore invite fewer questions about its
> implementation when being read?
>
> (It also strikes me this is probably a pattern we need to employ on
> other weakrefs to locks, so there's probably room for some sort of
> lockutil package eventually.)

This is exactly copied from the local repository class (So it is already 
employ for other weakrefs to core locks).
You are right,  It could (and most probably should) be a utility 
function instead of an object method. This time I went for the 
sub-optimal but consistent codebase instead of doing the cleanup detour. 
I was mostly aiming at getting the pypy test  green again using the 
shortest path as they were failing for over a month without much 
reaction from people with ownership of pypy or journal code.

Cheers,