* [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)
@ 2019-05-07 0:14 Zac Medico
2019-05-07 14:55 ` Alec Warner
0 siblings, 1 reply; 6+ messages in thread
From: Zac Medico @ 2019-05-07 0:14 UTC (permalink / raw
To: gentoo-portage-dev; +Cc: Zac Medico
Add a reentrant lock/unlock method which is useful for things like
emaint binhost and eclean-pkg. The vardbapi class already provides
lock/unlock methods that behave the same way.
Bug: https://bugs.gentoo.org/685236
Signed-off-by: Zac Medico <zmedico@gentoo.org>
---
lib/portage/dbapi/bintree.py | 46 ++++++++++++++-----
lib/portage/emaint/modules/binhost/binhost.py | 9 ++--
2 files changed, 38 insertions(+), 17 deletions(-)
diff --git a/lib/portage/dbapi/bintree.py b/lib/portage/dbapi/bintree.py
index 9c2d877e7..707958858 100644
--- a/lib/portage/dbapi/bintree.py
+++ b/lib/portage/dbapi/bintree.py
@@ -1,4 +1,4 @@
-# Copyright 1998-2018 Gentoo Foundation
+# Copyright 1998-2019 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2
from __future__ import unicode_literals
@@ -94,6 +94,8 @@ class bindbapi(fakedbapi):
])
self._aux_cache_slot_dict = slot_dict_class(self._aux_cache_keys)
self._aux_cache = {}
+ self._lock = None
+ self._lock_count = 0
@property
def writable(self):
@@ -106,6 +108,34 @@ class bindbapi(fakedbapi):
"""
return os.access(first_existing(self.bintree.pkgdir), os.W_OK)
+ def lock(self):
+ """
+ Acquire a reentrant lock, blocking, for cooperation with concurrent
+ processes.
+ """
+ if self._lock_count <= 0:
+ if self._lock is not None:
+ raise AssertionError("already locked")
+ # At least the parent needs to exist for the lock file.
+ ensure_dirs(self.bintree.pkgdir)
+ self._lock = lockfile(self.bintree._pkgindex_file, wantnewlockfile=True)
+
+ self._lock_count += 1
+
+ def unlock(self):
+ """
+ Release a lock, decrementing the recursion level. Each unlock() call
+ must be matched with a prior lock() call, or else an AssertionError
+ will be raised if unlock() is called while not locked.
+ """
+ if self._lock_count <= 1:
+ if self._lock is None:
+ raise AssertionError("not locked")
+ unlockfile(self._lock)
+ self._lock = None
+
+ self._lock_count -= 1
+
def match(self, *pargs, **kwargs):
if self.bintree and not self.bintree.populated:
self.bintree.populate()
@@ -545,16 +575,13 @@ class binarytree(object):
# that changes made by a concurrent process cannot be lost. This
# case is avoided when possible, in order to minimize lock
# contention.
- pkgindex_lock = None
+ self.dbapi.lock()
try:
- pkgindex_lock = lockfile(self._pkgindex_file,
- wantnewlockfile=True)
update_pkgindex = self._populate_local()
if update_pkgindex:
self._pkgindex_write(update_pkgindex)
finally:
- if pkgindex_lock:
- unlockfile(pkgindex_lock)
+ self.dbapi.unlock()
if getbinpkgs:
if not self.settings.get("PORTAGE_BINHOST"):
@@ -1110,10 +1137,8 @@ class binarytree(object):
# Reread the Packages index (in case it's been changed by another
# process) and then updated it, all while holding a lock.
- pkgindex_lock = None
+ self.dbapi.lock()
try:
- pkgindex_lock = lockfile(self._pkgindex_file,
- wantnewlockfile=1)
if filename is not None:
new_filename = self.getname(cpv, allocate_new=True)
try:
@@ -1154,8 +1179,7 @@ class binarytree(object):
self._pkgindex_write(pkgindex)
finally:
- if pkgindex_lock:
- unlockfile(pkgindex_lock)
+ self.dbapi.unlock()
# This is used to record BINPKGMD5 in the installed package
# database, for a package that has just been built.
diff --git a/lib/portage/emaint/modules/binhost/binhost.py b/lib/portage/emaint/modules/binhost/binhost.py
index d3df0cbce..ceb9e87f3 100644
--- a/lib/portage/emaint/modules/binhost/binhost.py
+++ b/lib/portage/emaint/modules/binhost/binhost.py
@@ -1,4 +1,4 @@
-# Copyright 2005-2014 Gentoo Foundation
+# Copyright 2005-2019 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2
import errno
@@ -27,7 +27,6 @@ class BinhostHandler(object):
eroot = portage.settings['EROOT']
self._bintree = portage.db[eroot]["bintree"]
self._bintree.populate()
- self._pkgindex_file = self._bintree._pkgindex_file
self._pkgindex = self._bintree._load_pkgindex()
def _need_update(self, cpv, data):
@@ -118,9 +117,7 @@ class BinhostHandler(object):
missing.append(cpv)
if missing or stale:
- from portage import locks
- pkgindex_lock = locks.lockfile(
- self._pkgindex_file, wantnewlockfile=1)
+ bintree.dbapi.lock()
try:
# Repopulate with lock held. If _populate_local returns
# data then use that, since _load_pkgindex would return
@@ -174,7 +171,7 @@ class BinhostHandler(object):
bintree._pkgindex_write(self._pkgindex)
finally:
- locks.unlockfile(pkgindex_lock)
+ bintree.dbapi.unlock()
if onProgress:
if maxval == 0:
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)
2019-05-07 0:14 [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236) Zac Medico
@ 2019-05-07 14:55 ` Alec Warner
2019-05-07 20:01 ` Zac Medico
0 siblings, 1 reply; 6+ messages in thread
From: Alec Warner @ 2019-05-07 14:55 UTC (permalink / raw
To: gentoo-portage-dev; +Cc: Zac Medico
[-- Attachment #1: Type: text/plain, Size: 7815 bytes --]
On Mon, May 6, 2019 at 8:15 PM Zac Medico <zmedico@gentoo.org> wrote:
> Add a reentrant lock/unlock method which is useful for things like
> emaint binhost and eclean-pkg. The vardbapi class already provides
> lock/unlock methods that behave the same way.
>
Curious why we are not doing more encapsulation here.
I'd suggest a new class type in locks.py:
class RefCountedLock(object): ...
# This implements the Lock() / Unlock() interface, as well as perhaps
__enter__ and __exit__.
# It uses a lockfile at path and a reference count.
Then in bintree's init, set self.lock to
locks.refCountedLock(self.bintree._pkgindex_file)
def lock(self): # on bintree
self._lock.Lock()
def unlock(self):
self._lock.Unlock()
Also curious why we are not implementing enter and exit so we can avoid
unbalanced pairs by using context managers.
e.g. in match(), we could likely write:
with self.dbapi.lock():
# try to match
update_pkgindex = self._populate_local()
if update_pkgindex:
self._pkgindex_write(update_pkgindex)
If the block raises an exception, __exit__ is still called, so we can
eliminate the finally: blocks and replace them with a careful __exit__
implementation?
-A
>
> Bug: https://bugs.gentoo.org/685236
> Signed-off-by: Zac Medico <zmedico@gentoo.org>
> ---
> lib/portage/dbapi/bintree.py | 46 ++++++++++++++-----
> lib/portage/emaint/modules/binhost/binhost.py | 9 ++--
> 2 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/lib/portage/dbapi/bintree.py b/lib/portage/dbapi/bintree.py
> index 9c2d877e7..707958858 100644
> --- a/lib/portage/dbapi/bintree.py
> +++ b/lib/portage/dbapi/bintree.py
> @@ -1,4 +1,4 @@
> -# Copyright 1998-2018 Gentoo Foundation
> +# Copyright 1998-2019 Gentoo Authors
> # Distributed under the terms of the GNU General Public License v2
>
> from __future__ import unicode_literals
> @@ -94,6 +94,8 @@ class bindbapi(fakedbapi):
> ])
> self._aux_cache_slot_dict =
> slot_dict_class(self._aux_cache_keys)
> self._aux_cache = {}
> + self._lock = None
> + self._lock_count = 0
>
> @property
> def writable(self):
> @@ -106,6 +108,34 @@ class bindbapi(fakedbapi):
> """
> return os.access(first_existing(self.bintree.pkgdir),
> os.W_OK)
>
> + def lock(self):
> + """
> + Acquire a reentrant lock, blocking, for cooperation with
> concurrent
> + processes.
> + """
> + if self._lock_count <= 0:
> + if self._lock is not None:
> + raise AssertionError("already locked")
> + # At least the parent needs to exist for the lock
> file.
> + ensure_dirs(self.bintree.pkgdir)
> + self._lock = lockfile(self.bintree._pkgindex_file,
> wantnewlockfile=True)
> +
> + self._lock_count += 1
> +
> + def unlock(self):
> + """
> + Release a lock, decrementing the recursion level. Each
> unlock() call
> + must be matched with a prior lock() call, or else an
> AssertionError
> + will be raised if unlock() is called while not locked.
> + """
> + if self._lock_count <= 1:
> + if self._lock is None:
> + raise AssertionError("not locked")
> + unlockfile(self._lock)
> + self._lock = None
> +
> + self._lock_count -= 1
> +
> def match(self, *pargs, **kwargs):
> if self.bintree and not self.bintree.populated:
> self.bintree.populate()
> @@ -545,16 +575,13 @@ class binarytree(object):
> # that changes made by a concurrent
> process cannot be lost. This
> # case is avoided when possible, in order
> to minimize lock
> # contention.
> - pkgindex_lock = None
> + self.dbapi.lock()
> try:
> - pkgindex_lock =
> lockfile(self._pkgindex_file,
> - wantnewlockfile=True)
> update_pkgindex =
> self._populate_local()
> if update_pkgindex:
>
> self._pkgindex_write(update_pkgindex)
> finally:
> - if pkgindex_lock:
> - unlockfile(pkgindex_lock)
> + self.dbapi.unlock()
>
> if getbinpkgs:
> if not
> self.settings.get("PORTAGE_BINHOST"):
> @@ -1110,10 +1137,8 @@ class binarytree(object):
>
> # Reread the Packages index (in case it's been changed by
> another
> # process) and then updated it, all while holding a lock.
> - pkgindex_lock = None
> + self.dbapi.lock()
> try:
> - pkgindex_lock = lockfile(self._pkgindex_file,
> - wantnewlockfile=1)
> if filename is not None:
> new_filename = self.getname(cpv,
> allocate_new=True)
> try:
> @@ -1154,8 +1179,7 @@ class binarytree(object):
> self._pkgindex_write(pkgindex)
>
> finally:
> - if pkgindex_lock:
> - unlockfile(pkgindex_lock)
> + self.dbapi.unlock()
>
> # This is used to record BINPKGMD5 in the installed package
> # database, for a package that has just been built.
> diff --git a/lib/portage/emaint/modules/binhost/binhost.py
> b/lib/portage/emaint/modules/binhost/binhost.py
> index d3df0cbce..ceb9e87f3 100644
> --- a/lib/portage/emaint/modules/binhost/binhost.py
> +++ b/lib/portage/emaint/modules/binhost/binhost.py
> @@ -1,4 +1,4 @@
> -# Copyright 2005-2014 Gentoo Foundation
> +# Copyright 2005-2019 Gentoo Authors
> # Distributed under the terms of the GNU General Public License v2
>
> import errno
> @@ -27,7 +27,6 @@ class BinhostHandler(object):
> eroot = portage.settings['EROOT']
> self._bintree = portage.db[eroot]["bintree"]
> self._bintree.populate()
> - self._pkgindex_file = self._bintree._pkgindex_file
> self._pkgindex = self._bintree._load_pkgindex()
>
> def _need_update(self, cpv, data):
> @@ -118,9 +117,7 @@ class BinhostHandler(object):
> missing.append(cpv)
>
> if missing or stale:
> - from portage import locks
> - pkgindex_lock = locks.lockfile(
> - self._pkgindex_file, wantnewlockfile=1)
> + bintree.dbapi.lock()
> try:
> # Repopulate with lock held. If
> _populate_local returns
> # data then use that, since _load_pkgindex
> would return
> @@ -174,7 +171,7 @@ class BinhostHandler(object):
> bintree._pkgindex_write(self._pkgindex)
>
> finally:
> - locks.unlockfile(pkgindex_lock)
> + bintree.dbapi.unlock()
>
> if onProgress:
> if maxval == 0:
> --
> 2.21.0
>
>
>
[-- Attachment #2: Type: text/html, Size: 10544 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)
2019-05-07 14:55 ` Alec Warner
@ 2019-05-07 20:01 ` Zac Medico
2019-05-08 18:05 ` Zac Medico
0 siblings, 1 reply; 6+ messages in thread
From: Zac Medico @ 2019-05-07 20:01 UTC (permalink / raw
To: gentoo-portage-dev, Alec Warner; +Cc: Zac Medico
[-- Attachment #1.1: Type: text/plain, Size: 1894 bytes --]
On 5/7/19 7:55 AM, Alec Warner wrote:
>
>
> On Mon, May 6, 2019 at 8:15 PM Zac Medico <zmedico@gentoo.org
> <mailto:zmedico@gentoo.org>> wrote:
>
> Add a reentrant lock/unlock method which is useful for things like
> emaint binhost and eclean-pkg. The vardbapi class already provides
> lock/unlock methods that behave the same way.
>
>
> Curious why we are not doing more encapsulation here.
>
> I'd suggest a new class type in locks.py:
>
> class RefCountedLock(object): ...
> # This implements the Lock() / Unlock() interface, as well as perhaps
> __enter__ and __exit__.
> # It uses a lockfile at path and a reference count.
>
> Then in bintree's init, set self.lock to
> locks.refCountedLock(self.bintree._pkgindex_file)
>
> def lock(self): # on bintree
> self._lock.Lock()
>
> def unlock(self):
> self._lock.Unlock()
Yes, splitting out a reference counted lock class like this is a good
idea. I'll do that.
> Also curious why we are not implementing enter and exit so we can avoid
> unbalanced pairs by using context managers.
>
> e.g. in match(), we could likely write:
>
> with self.dbapi.lock():
> # try to match
> update_pkgindex = self._populate_local()
> if update_pkgindex:
> self._pkgindex_write(update_pkgindex)
>
> If the block raises an exception, __exit__ is still called, so we can
> eliminate the finally: blocks and replace them with a careful __exit__
> implementation?
>
> -A
Yeah, the reference counted lock class could have a method that returns
a context manager which increments/decrements the lock count. The
context manager can be implemented with the contextlib.contextmanager
decorator like this:
@contextlib.contextmanager
def contextmanager(self):
self.lock()
try:
yield self
finally:
self.unlock()
--
Thanks,
Zac
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)
2019-05-07 20:01 ` Zac Medico
@ 2019-05-08 18:05 ` Zac Medico
2019-05-08 18:09 ` Alec Warner
2019-05-10 9:05 ` Zac Medico
0 siblings, 2 replies; 6+ messages in thread
From: Zac Medico @ 2019-05-08 18:05 UTC (permalink / raw
To: Zac Medico, gentoo-portage-dev, Alec Warner
[-- Attachment #1.1: Type: text/plain, Size: 1431 bytes --]
On 5/7/19 1:01 PM, Zac Medico wrote:
> On 5/7/19 7:55 AM, Alec Warner wrote:
>
>> Also curious why we are not implementing enter and exit so we can avoid
>> unbalanced pairs by using context managers.
>>
>> e.g. in match(), we could likely write:
>>
>> with self.dbapi.lock():
>> # try to match
>> update_pkgindex = self._populate_local()
>> if update_pkgindex:
>> self._pkgindex_write(update_pkgindex)
>>
>> If the block raises an exception, __exit__ is still called, so we can
>> eliminate the finally: blocks and replace them with a careful __exit__
>> implementation?
>>
>> -A
>
> Yeah, the reference counted lock class could have a method that returns
> a context manager which increments/decrements the lock count. The
> context manager can be implemented with the contextlib.contextmanager
> decorator like this:
>
> @contextlib.contextmanager
> def contextmanager(self):
> self.lock()
> try:
> yield self
> finally:
> self.unlock()
>
Since we really don't want asynchronous code to block waiting for a
lock, we really should use a (python3.5+ only) asynchronous context manager:
https://www.python.org/dev/peps/pep-0492/#asynchronous-context-managers-and-async-with
Given that python2.7 is scheduled for retirement in 2020
(https://pythonclock.org/), maybe it's time to drop support for
python3.4 and earlier.
--
Thanks,
Zac
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)
2019-05-08 18:05 ` Zac Medico
@ 2019-05-08 18:09 ` Alec Warner
2019-05-10 9:05 ` Zac Medico
1 sibling, 0 replies; 6+ messages in thread
From: Alec Warner @ 2019-05-08 18:09 UTC (permalink / raw
To: Zac Medico; +Cc: gentoo-portage-dev
[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]
On Wed, May 8, 2019 at 2:05 PM Zac Medico <zmedico@gentoo.org> wrote:
> On 5/7/19 1:01 PM, Zac Medico wrote:
> > On 5/7/19 7:55 AM, Alec Warner wrote:
> >
> >> Also curious why we are not implementing enter and exit so we can avoid
> >> unbalanced pairs by using context managers.
> >>
> >> e.g. in match(), we could likely write:
> >>
> >> with self.dbapi.lock():
> >> # try to match
> >> update_pkgindex = self._populate_local()
> >> if update_pkgindex:
> >> self._pkgindex_write(update_pkgindex)
> >>
> >> If the block raises an exception, __exit__ is still called, so we can
> >> eliminate the finally: blocks and replace them with a careful __exit__
> >> implementation?
> >>
> >> -A
> >
> > Yeah, the reference counted lock class could have a method that returns
> > a context manager which increments/decrements the lock count. The
> > context manager can be implemented with the contextlib.contextmanager
> > decorator like this:
> >
> > @contextlib.contextmanager
> > def contextmanager(self):
> > self.lock()
> > try:
> > yield self
> > finally:
> > self.unlock()
> >
>
> Since we really don't want asynchronous code to block waiting for a
> lock, we really should use a (python3.5+ only) asynchronous context
> manager:
>
>
> https://www.python.org/dev/peps/pep-0492/#asynchronous-context-managers-and-async-with
>
> Given that python2.7 is scheduled for retirement in 2020
> (https://pythonclock.org/), maybe it's time to drop support for
> python3.4 and earlier.
>
I haven't used that stuff yet, so I will defer to you on its use; same with
python support.
-A
> --
> Thanks,
> Zac
>
>
[-- Attachment #2: Type: text/html, Size: 2616 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)
2019-05-08 18:05 ` Zac Medico
2019-05-08 18:09 ` Alec Warner
@ 2019-05-10 9:05 ` Zac Medico
1 sibling, 0 replies; 6+ messages in thread
From: Zac Medico @ 2019-05-10 9:05 UTC (permalink / raw
To: Zac Medico, gentoo-portage-dev, Alec Warner
[-- Attachment #1.1: Type: text/plain, Size: 2103 bytes --]
On 5/8/19 11:05 AM, Zac Medico wrote:
> On 5/7/19 1:01 PM, Zac Medico wrote:
>> On 5/7/19 7:55 AM, Alec Warner wrote:
>>
>>> Also curious why we are not implementing enter and exit so we can avoid
>>> unbalanced pairs by using context managers.
>>>
>>> e.g. in match(), we could likely write:
>>>
>>> with self.dbapi.lock():
>>> # try to match
>>> update_pkgindex = self._populate_local()
>>> if update_pkgindex:
>>> self._pkgindex_write(update_pkgindex)
>>>
>>> If the block raises an exception, __exit__ is still called, so we can
>>> eliminate the finally: blocks and replace them with a careful __exit__
>>> implementation?
>>>
>>> -A
>>
>> Yeah, the reference counted lock class could have a method that returns
>> a context manager which increments/decrements the lock count. The
>> context manager can be implemented with the contextlib.contextmanager
>> decorator like this:
>>
>> @contextlib.contextmanager
>> def contextmanager(self):
>> self.lock()
>> try:
>> yield self
>> finally:
>> self.unlock()
>>
>
> Since we really don't want asynchronous code to block waiting for a
> lock, we really should use a (python3.5+ only) asynchronous context manager:
>
> https://www.python.org/dev/peps/pep-0492/#asynchronous-context-managers-and-async-with
>
> Given that python2.7 is scheduled for retirement in 2020
> (https://pythonclock.org/), maybe it's time to drop support for
> python3.4 and earlier.
Adapting the sample code provided in PEP 492, we can do something like
this for compatibility with older versions of python:
from portage.util.futures.compat_coroutine
@compat_coroutine.coroutine
def async_with(mgr, async_func):
aexit = type(mgr).__aexit__
aenter = type(mgr).__aenter__(mgr)
aenter_result = (yield aenter)
try:
return_value = (yield async_func(aenter_result))
except:
if not (yield aexit(mgr, *sys.exc_info()):
raise
else:
yield aexit(mgr, None, None, None)
compat_coroutine.coroutine_return(return_value)
--
Thanks,
Zac
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 981 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-10 9:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-07 0:14 [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236) Zac Medico
2019-05-07 14:55 ` Alec Warner
2019-05-07 20:01 ` Zac Medico
2019-05-08 18:05 ` Zac Medico
2019-05-08 18:09 ` Alec Warner
2019-05-10 9:05 ` Zac Medico
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox