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

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

[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

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

[Alec Warner]

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

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

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

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

[Zac Medico]

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

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

[Alec Warner]

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

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

[Zac Medico]

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