On Mon, May 6, 2019 at 8:15 PM Zac Medico <zmed...@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 <zmed...@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
>
>
>
