Patchwork [1,of,7] lock: add multilock class

login
register
mail settings
Submitter Angel Ezquerra
Date Nov. 25, 2015, 12:22 a.m.
Message ID <6b3bf294c85ce407f2bf.1448410945@waste.org>
Download mbox | patch
Permalink /patch/11631/
State Changes Requested
Headers show

Comments

Angel Ezquerra - Nov. 25, 2015, 12:22 a.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1446166297 -3600
#      Fri Oct 30 01:51:37 2015 +0100
# Node ID 6b3bf294c85ce407f2bf2f6f18eb07a85aa3ab54
# Parent  df9b73d2d444ae82fe8d3fe6cf682a93b2c4a7ef
lock: add multilock class

This new multilock class is a lock container that behaves as a single lock.
This will be useful to implement full shared repositories.

The multilock releases its locks in the reverse order that it locks them, which
is in the same order that they are passed to the multilock constructor.
Gregory Szorc - Nov. 28, 2015, 9:57 p.m.
On Tue, Nov 24, 2015 at 4:22 PM, Angel Ezquerra <angel.ezquerra@gmail.com>
wrote:

> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1446166297 -3600
> #      Fri Oct 30 01:51:37 2015 +0100
> # Node ID 6b3bf294c85ce407f2bf2f6f18eb07a85aa3ab54
> # Parent  df9b73d2d444ae82fe8d3fe6cf682a93b2c4a7ef
> lock: add multilock class
>
> This new multilock class is a lock container that behaves as a single lock.
> This will be useful to implement full shared repositories.
>
> The multilock releases its locks in the reverse order that it locks them,
> which
> is in the same order that they are passed to the multilock constructor.
>
> diff --git a/mercurial/lock.py b/mercurial/lock.py
> --- a/mercurial/lock.py
> +++ b/mercurial/lock.py
> @@ -19,7 +19,22 @@
>      util,
>  )
>
> -class lock(object):
> +
> +class abstractlock(object):
> +    '''define that interface that all lock classes must follow'''
> +    def lock(self):
> +        raise NotImplementedError('abstract method')
> +
> +    def trylock(self):
> +        raise NotImplementedError('abstract method')
> +
> +    def testlock(self):
> +        raise NotImplementedError('abstract method')
> +
> +    def release(self):
> +        raise NotImplementedError('abstract method')
> +
> +class lock(abstractlock):
>      '''An advisory lock held by one process to control access to a set
>      of files.  Non-cooperating processes or incorrectly written scripts
>      can ignore Mercurial's locking scheme and stomp all over the
> @@ -230,6 +245,58 @@
>                  for callback in self.postrelease:
>                      callback()
>
> +class multilock(abstractlock):
> +    """a group of locks that behave as one"""
> +    def __init__(self, *locks):
> +        self.locks = locks
> +        self.postrelease = []
> +
> +    def __del__(self):
> +        for l in reversed(self.locks):
> +            del l
> +        self.locks = []
>

__del__ is kind of evil since it can block garbage collection from
occurring, causing memory leaks. See
https://docs.python.org/2/library/gc.html#gc.garbage.

If you need to implement __del__ here to conform to the existing lock API,
so be it. But I would strongly prefer we refactor the lock API to use
context managers (preferred) or a cleanup function that isn't named __del__.


> +
> +    def lock(self):
> +        for l in self.locks:
> +            l.lock()
> +
> +    def trylock(self):
> +        locked = []
> +        try:
> +            for l in self.locks:
> +                l.trylock()
> +                locked.append(l)
> +        except (error.LockHeld, error.LockUnavailable):
> +            # unlock any locked locks
> +            for l in locked:
> +                l.unlock()
> +            raise
> +
> +    def testlock(self):
> +        """return id of first valid lock, else None"""
> +        for l in self.locks:
> +            res = l.testlock()
> +            if res is not None:
> +                return res
> +        return None
> +
> +    def release(self):
> +        """release all locks executing their callback functions if any"""
> +        for l in reversed(self.locks):
> +            l.release()
> +        # The postrelease functions typically assume the lock is not held
> at all
> +        if not self._parentheld:
> +            for callback in self.postrelease:
> +                callback()
> +
> +    @property
> +    def held(self):
> +        return any(l.held for l in self.locks)
> +
> +    @property
> +    def _parentheld(self):
> +        return any(l._parentheld for l in self.locks)
> +
>  def release(*locks):
>      for lock in locks:
>          if lock is not None:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - Nov. 29, 2015, 1:45 p.m.
On Tue, 24 Nov 2015 18:22:25 -0600, Angel Ezquerra wrote:
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1446166297 -3600
> #      Fri Oct 30 01:51:37 2015 +0100
> # Node ID 6b3bf294c85ce407f2bf2f6f18eb07a85aa3ab54
> # Parent  df9b73d2d444ae82fe8d3fe6cf682a93b2c4a7ef
> lock: add multilock class
> 
> This new multilock class is a lock container that behaves as a single lock.
> This will be useful to implement full shared repositories.
> 
> The multilock releases its locks in the reverse order that it locks them, which
> is in the same order that they are passed to the multilock constructor.
> 
> diff --git a/mercurial/lock.py b/mercurial/lock.py
> --- a/mercurial/lock.py
> +++ b/mercurial/lock.py
> @@ -19,7 +19,22 @@
>      util,
>  )
>  
> -class lock(object):
> +
> +class abstractlock(object):
> +    '''define that interface that all lock classes must follow'''
> +    def lock(self):
> +        raise NotImplementedError('abstract method')
> +
> +    def trylock(self):
> +        raise NotImplementedError('abstract method')

The lock class has no trylock() but _trylock().

And it can just "raise NotImplementedError" if there's no additional
information.

> +class multilock(abstractlock):
> +    """a group of locks that behave as one"""
> +    def __init__(self, *locks):
> +        self.locks = locks
> +        self.postrelease = []
> +
> +    def __del__(self):
> +        for l in reversed(self.locks):
> +            del l
> +        self.locks = []

"del l" will do nothing because "l" still exists in self.locks.

> +
> +    def lock(self):
> +        for l in self.locks:
> +            l.lock()
> +
> +    def trylock(self):
> +        locked = []
> +        try:
> +            for l in self.locks:
> +                l.trylock()
> +                locked.append(l)
> +        except (error.LockHeld, error.LockUnavailable):
> +            # unlock any locked locks
> +            for l in locked:
> +                l.unlock()
> +            raise
> +
> +    def testlock(self):
> +        """return id of first valid lock, else None"""
> +        for l in self.locks:
> +            res = l.testlock()
> +            if res is not None:
> +                return res
> +        return None

It seems multilock.inherit() is missing.

> +    def release(self):
> +        """release all locks executing their callback functions if any"""
> +        for l in reversed(self.locks):
> +            l.release()
> +        # The postrelease functions typically assume the lock is not held at all
> +        if not self._parentheld:
> +            for callback in self.postrelease:
> +                callback()
> +
> +    @property
> +    def held(self):
> +        return any(l.held for l in self.locks)
> +
> +    @property
> +    def _parentheld(self):
> +        return any(l._parentheld for l in self.locks)

I'm not sure if this _parentheld semantics is correct. Should we skip
postrelease if either one is _parentheld but the other isn't?
Angel Ezquerra - Dec. 7, 2015, 11:10 a.m.
On Sat, Nov 28, 2015 at 10:57 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> On Tue, Nov 24, 2015 at 4:22 PM, Angel Ezquerra <angel.ezquerra@gmail.com>
> wrote:
>>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>> # Date 1446166297 -3600
>> #      Fri Oct 30 01:51:37 2015 +0100
>> # Node ID 6b3bf294c85ce407f2bf2f6f18eb07a85aa3ab54
>> # Parent  df9b73d2d444ae82fe8d3fe6cf682a93b2c4a7ef
>> lock: add multilock class
>>
>> This new multilock class is a lock container that behaves as a single
>> lock.
>> This will be useful to implement full shared repositories.
>>
>> The multilock releases its locks in the reverse order that it locks them,
>> which
>> is in the same order that they are passed to the multilock constructor.
>>
>> diff --git a/mercurial/lock.py b/mercurial/lock.py
>> --- a/mercurial/lock.py
>> +++ b/mercurial/lock.py
>> @@ -19,7 +19,22 @@
>>      util,
>>  )
>>
>> -class lock(object):
>> +
>> +class abstractlock(object):
>> +    '''define that interface that all lock classes must follow'''
>> +    def lock(self):
>> +        raise NotImplementedError('abstract method')
>> +
>> +    def trylock(self):
>> +        raise NotImplementedError('abstract method')
>> +
>> +    def testlock(self):
>> +        raise NotImplementedError('abstract method')
>> +
>> +    def release(self):
>> +        raise NotImplementedError('abstract method')
>> +
>> +class lock(abstractlock):
>>      '''An advisory lock held by one process to control access to a set
>>      of files.  Non-cooperating processes or incorrectly written scripts
>>      can ignore Mercurial's locking scheme and stomp all over the
>> @@ -230,6 +245,58 @@
>>                  for callback in self.postrelease:
>>                      callback()
>>
>> +class multilock(abstractlock):
>> +    """a group of locks that behave as one"""
>> +    def __init__(self, *locks):
>> +        self.locks = locks
>> +        self.postrelease = []
>> +
>> +    def __del__(self):
>> +        for l in reversed(self.locks):
>> +            del l
>> +        self.locks = []
>
>
> __del__ is kind of evil since it can block garbage collection from
> occurring, causing memory leaks. See
> https://docs.python.org/2/library/gc.html#gc.garbage.
>
> If you need to implement __del__ here to conform to the existing lock API,
> so be it. But I would strongly prefer we refactor the lock API to use
> context managers (preferred) or a cleanup function that isn't named __del__.

Greg, thanks for the review. Sorry it took me a while to get back to you.

I agree with the sentiment, but I think that refactoring the lock API
is well beyond the scope of what I am trying to do. I think is should
be handled separately, on another future set of patches. Do you think
that makes sense?

Cheers,

Angel
Angel Ezquerra - Dec. 7, 2015, 11:13 a.m.
On Sun, Nov 29, 2015 at 2:45 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Tue, 24 Nov 2015 18:22:25 -0600, Angel Ezquerra wrote:
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>> # Date 1446166297 -3600
>> #      Fri Oct 30 01:51:37 2015 +0100
>> # Node ID 6b3bf294c85ce407f2bf2f6f18eb07a85aa3ab54
>> # Parent  df9b73d2d444ae82fe8d3fe6cf682a93b2c4a7ef
>> lock: add multilock class
>>
>> This new multilock class is a lock container that behaves as a single lock.
>> This will be useful to implement full shared repositories.
>>
>> The multilock releases its locks in the reverse order that it locks them, which
>> is in the same order that they are passed to the multilock constructor.
>>
>> diff --git a/mercurial/lock.py b/mercurial/lock.py
>> --- a/mercurial/lock.py
>> +++ b/mercurial/lock.py
>> @@ -19,7 +19,22 @@
>>      util,
>>  )
>>
>> -class lock(object):
>> +
>> +class abstractlock(object):
>> +    '''define that interface that all lock classes must follow'''
>> +    def lock(self):
>> +        raise NotImplementedError('abstract method')
>> +
>> +    def trylock(self):
>> +        raise NotImplementedError('abstract method')
>
> The lock class has no trylock() but _trylock().
>
> And it can just "raise NotImplementedError" if there's no additional
> information.

OK

>> +class multilock(abstractlock):
>> +    """a group of locks that behave as one"""
>> +    def __init__(self, *locks):
>> +        self.locks = locks
>> +        self.postrelease = []
>> +
>> +    def __del__(self):
>> +        for l in reversed(self.locks):
>> +            del l
>> +        self.locks = []
>
> "del l" will do nothing because "l" still exists in self.locks.

Good catch, and it ties back with get Greg said on another email.

>> +
>> +    def lock(self):
>> +        for l in self.locks:
>> +            l.lock()
>> +
>> +    def trylock(self):
>> +        locked = []
>> +        try:
>> +            for l in self.locks:
>> +                l.trylock()
>> +                locked.append(l)
>> +        except (error.LockHeld, error.LockUnavailable):
>> +            # unlock any locked locks
>> +            for l in locked:
>> +                l.unlock()
>> +            raise
>> +
>> +    def testlock(self):
>> +        """return id of first valid lock, else None"""
>> +        for l in self.locks:
>> +            res = l.testlock()
>> +            if res is not None:
>> +                return res
>> +        return None
>
> It seems multilock.inherit() is missing.
>
>> +    def release(self):
>> +        """release all locks executing their callback functions if any"""
>> +        for l in reversed(self.locks):
>> +            l.release()
>> +        # The postrelease functions typically assume the lock is not held at all
>> +        if not self._parentheld:
>> +            for callback in self.postrelease:
>> +                callback()
>> +
>> +    @property
>> +    def held(self):
>> +        return any(l.held for l in self.locks)
>> +
>> +    @property
>> +    def _parentheld(self):
>> +        return any(l._parentheld for l in self.locks)
>
> I'm not sure if this _parentheld semantics is correct. Should we skip
> postrelease if either one is _parentheld but the other isn't?

I'm really not sure. It'd be great if someone else who knows more
about how locks are used would comment on this.

Cheers,

Angel
Gregory Szorc - Dec. 12, 2015, 9:52 p.m.
> On Dec 7, 2015, at 06:10, Angel Ezquerra <angel.ezquerra@gmail.com> wrote:
> 
>> On Sat, Nov 28, 2015 at 10:57 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>> On Tue, Nov 24, 2015 at 4:22 PM, Angel Ezquerra <angel.ezquerra@gmail.com>
>> wrote:
>>> 
>>> # HG changeset patch
>>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>>> # Date 1446166297 -3600
>>> #      Fri Oct 30 01:51:37 2015 +0100
>>> # Node ID 6b3bf294c85ce407f2bf2f6f18eb07a85aa3ab54
>>> # Parent  df9b73d2d444ae82fe8d3fe6cf682a93b2c4a7ef
>>> lock: add multilock class
>>> 
>>> This new multilock class is a lock container that behaves as a single
>>> lock.
>>> This will be useful to implement full shared repositories.
>>> 
>>> The multilock releases its locks in the reverse order that it locks them,
>>> which
>>> is in the same order that they are passed to the multilock constructor.
>>> 
>>> diff --git a/mercurial/lock.py b/mercurial/lock.py
>>> --- a/mercurial/lock.py
>>> +++ b/mercurial/lock.py
>>> @@ -19,7 +19,22 @@
>>>     util,
>>> )
>>> 
>>> -class lock(object):
>>> +
>>> +class abstractlock(object):
>>> +    '''define that interface that all lock classes must follow'''
>>> +    def lock(self):
>>> +        raise NotImplementedError('abstract method')
>>> +
>>> +    def trylock(self):
>>> +        raise NotImplementedError('abstract method')
>>> +
>>> +    def testlock(self):
>>> +        raise NotImplementedError('abstract method')
>>> +
>>> +    def release(self):
>>> +        raise NotImplementedError('abstract method')
>>> +
>>> +class lock(abstractlock):
>>>     '''An advisory lock held by one process to control access to a set
>>>     of files.  Non-cooperating processes or incorrectly written scripts
>>>     can ignore Mercurial's locking scheme and stomp all over the
>>> @@ -230,6 +245,58 @@
>>>                 for callback in self.postrelease:
>>>                     callback()
>>> 
>>> +class multilock(abstractlock):
>>> +    """a group of locks that behave as one"""
>>> +    def __init__(self, *locks):
>>> +        self.locks = locks
>>> +        self.postrelease = []
>>> +
>>> +    def __del__(self):
>>> +        for l in reversed(self.locks):
>>> +            del l
>>> +        self.locks = []
>> 
>> 
>> __del__ is kind of evil since it can block garbage collection from
>> occurring, causing memory leaks. See
>> https://docs.python.org/2/library/gc.html#gc.garbage.
>> 
>> If you need to implement __del__ here to conform to the existing lock API,
>> so be it. But I would strongly prefer we refactor the lock API to use
>> context managers (preferred) or a cleanup function that isn't named __del__.
> 
> Greg, thanks for the review. Sorry it took me a while to get back to you.
> 
> I agree with the sentiment, but I think that refactoring the lock API
> is well beyond the scope of what I am trying to do. I think is should
> be handled separately, on another future set of patches. Do you think
> that makes sense?

I agree it is scope bloat. But I defer to someone with queueing privileges for the final word.
Yuya Nishihara - Dec. 13, 2015, 8:35 a.m.
On Sat, 12 Dec 2015 16:52:13 -0500, Gregory Szorc wrote:
> > On Dec 7, 2015, at 06:10, Angel Ezquerra <angel.ezquerra@gmail.com> wrote:
> >> On Sat, Nov 28, 2015 at 10:57 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> >> On Tue, Nov 24, 2015 at 4:22 PM, Angel Ezquerra <angel.ezquerra@gmail.com>
> >> wrote:
> >>> +class multilock(abstractlock):
> >>> +    """a group of locks that behave as one"""
> >>> +    def __init__(self, *locks):
> >>> +        self.locks = locks
> >>> +        self.postrelease = []
> >>> +
> >>> +    def __del__(self):
> >>> +        for l in reversed(self.locks):
> >>> +            del l
> >>> +        self.locks = []
> >> 
> >> 
> >> __del__ is kind of evil since it can block garbage collection from
> >> occurring, causing memory leaks. See
> >> https://docs.python.org/2/library/gc.html#gc.garbage.
> >> 
> >> If you need to implement __del__ here to conform to the existing lock API,
> >> so be it. But I would strongly prefer we refactor the lock API to use
> >> context managers (preferred) or a cleanup function that isn't named __del__.
> > 
> > Greg, thanks for the review. Sorry it took me a while to get back to you.
> > 
> > I agree with the sentiment, but I think that refactoring the lock API
> > is well beyond the scope of what I am trying to do. I think is should
> > be handled separately, on another future set of patches. Do you think
> > that makes sense?
> 
> I agree it is scope bloat. But I defer to someone with queueing privileges
> for the final word.

Because I've not joined the discussion about the subrepo cache, I'm not sure
this multilock class is the right path. It won't be easy to write multilock
fully conforming to the lock interface.

Anyway this __del__ has no effect, it can simply be removed.

Patch

diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -19,7 +19,22 @@ 
     util,
 )
 
-class lock(object):
+
+class abstractlock(object):
+    '''define that interface that all lock classes must follow'''
+    def lock(self):
+        raise NotImplementedError('abstract method')
+
+    def trylock(self):
+        raise NotImplementedError('abstract method')
+
+    def testlock(self):
+        raise NotImplementedError('abstract method')
+
+    def release(self):
+        raise NotImplementedError('abstract method')
+
+class lock(abstractlock):
     '''An advisory lock held by one process to control access to a set
     of files.  Non-cooperating processes or incorrectly written scripts
     can ignore Mercurial's locking scheme and stomp all over the
@@ -230,6 +245,58 @@ 
                 for callback in self.postrelease:
                     callback()
 
+class multilock(abstractlock):
+    """a group of locks that behave as one"""
+    def __init__(self, *locks):
+        self.locks = locks
+        self.postrelease = []
+
+    def __del__(self):
+        for l in reversed(self.locks):
+            del l
+        self.locks = []
+
+    def lock(self):
+        for l in self.locks:
+            l.lock()
+
+    def trylock(self):
+        locked = []
+        try:
+            for l in self.locks:
+                l.trylock()
+                locked.append(l)
+        except (error.LockHeld, error.LockUnavailable):
+            # unlock any locked locks
+            for l in locked:
+                l.unlock()
+            raise
+
+    def testlock(self):
+        """return id of first valid lock, else None"""
+        for l in self.locks:
+            res = l.testlock()
+            if res is not None:
+                return res
+        return None
+
+    def release(self):
+        """release all locks executing their callback functions if any"""
+        for l in reversed(self.locks):
+            l.release()
+        # The postrelease functions typically assume the lock is not held at all
+        if not self._parentheld:
+            for callback in self.postrelease:
+                callback()
+
+    @property
+    def held(self):
+        return any(l.held for l in self.locks)
+
+    @property
+    def _parentheld(self):
+        return any(l._parentheld for l in self.locks)
+
 def release(*locks):
     for lock in locks:
         if lock is not None: