Re: [gentoo-portage-dev] [PATCH] bindbapi: add reentrant lock method (bug 685236)

[Alec Warner <antarus@gentoo.org>]

[-- 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 --]