Re: [gentoo-portage-dev] [PATCH] BinpkgFetcher: use async lock (bug 614110)

2018-04-22 Thread Zac Medico 
On 04/21/2018 03:07 PM, Brian Dolbec wrote:
> On Sat, 21 Apr 2018 12:27:28 -0700
> Zac Medico  wrote:
> 
>> In order to avoid event loop recursion, convert the
>> _BinpkgFetcherProcess.lock() method to an async_lock
>> method for use by BinpkgFetcher.
>>
>> Bug: https://bugs.gentoo.org/614110
>> ---
>>  pym/_emerge/BinpkgFetcher.py | 53
>> +++- 1 file changed, 33
>> insertions(+), 20 deletions(-)
>>
>> diff --git a/pym/_emerge/BinpkgFetcher.py
>> b/pym/_emerge/BinpkgFetcher.py index 5ca7a45cf..2bbc0a26f 100644
>> --- a/pym/_emerge/BinpkgFetcher.py
>> +++ b/pym/_emerge/BinpkgFetcher.py
>> @@ -32,11 +32,24 @@ class BinpkgFetcher(CompositeTask):
>>  pkg.cpv) + ".partial"
>>  
>>  def _start(self):
>> -self._start_task(
>> -
>> _BinpkgFetcherProcess(background=self.background,
>> -logfile=self.logfile, pkg=self.pkg,
>> pkg_path=self.pkg_path,
>> -pretend=self.pretend,
>> scheduler=self.scheduler),
>> -self._fetcher_exit)
>> +fetcher =
>> _BinpkgFetcherProcess(background=self.background,
>> +logfile=self.logfile, pkg=self.pkg,
>> pkg_path=self.pkg_path,
>> +pretend=self.pretend,
>> scheduler=self.scheduler) +
>> +if not self.pretend:
>> +
>> portage.util.ensure_dirs(os.path.dirname(self.pkg_path))
>> +if "distlocks" in
>> self.pkg.root_config.settings.features:
>> +self._start_task(
>> +
>> AsyncTaskFuture(future=fetcher.async_lock()),
>> +
>> functools.partial(self._start_locked, fetcher))
>> +return
>> +
>> +self._start_task(fetcher, self._fetcher_exit)
>> +
>> +def _start_locked(self, fetcher, lock_task):
>> +self._assert_current(lock_task)
>> +lock_task.future.result()
>> +self._start_task(fetcher, self._fetcher_exit)
>>  
>>  def _fetcher_exit(self, fetcher):
>>  self._assert_current(fetcher)
>> @@ -68,13 +81,8 @@ class _BinpkgFetcherProcess(SpawnProcess):
>>  pretend = self.pretend
>>  bintree = pkg.root_config.trees["bintree"]
>>  settings = bintree.settings
>> -use_locks = "distlocks" in settings.features
>>  pkg_path = self.pkg_path
>>  
>> -if not pretend:
>> -
>> portage.util.ensure_dirs(os.path.dirname(pkg_path))
>> -if use_locks:
>> -self.lock()
>>  exists = os.path.exists(pkg_path)
>>  resume = exists and os.path.basename(pkg_path) in
>> bintree.invalids if not (pretend or resume):
>> @@ -184,7 +192,7 @@ class _BinpkgFetcherProcess(SpawnProcess):
>>  except
>> OSError: pass
>>  
>> -def lock(self):
>> +def async_lock(self):
>>  """
>>  This raises an AlreadyLocked exception if lock() is
>> called while a lock is already held. In order to avoid this, call
>> @@ -194,17 +202,22 @@ class _BinpkgFetcherProcess(SpawnProcess):
>>  if self._lock_obj is not None:
>>  raise self.AlreadyLocked((self._lock_obj,))
>>  
>> -async_lock = AsynchronousLock(path=self.pkg_path,
>> -scheduler=self.scheduler)
>> -async_lock.start()
>> +result = self.scheduler.create_future()
>>  
>> -if async_lock.wait() != os.EX_OK:
>> -# TODO: Use CompositeTask for better
>> handling, like in EbuildPhase.
>> -raise AssertionError("AsynchronousLock
>> failed with returncode %s" \
>> -% (async_lock.returncode,))
>> +def acquired_lock(async_lock):
>> +if async_lock.wait() == os.EX_OK:
>> +self.locked = True
>> +result.set_result(None)
>> +else:
>> +result.set_exception(AssertionError(
>> +"AsynchronousLock failed
>> with returncode %s"
>> +% (async_lock.returncode,)))
>>  
>> -self._lock_obj = async_lock
>> -self.locked = True
>> +self._lock_obj = AsynchronousLock(path=self.pkg_path,
>> +scheduler=self.scheduler)
>> +self._lock_obj.addExitListener(acquired_lock)
>> +self._lock_obj.start()
>> +return result
>>  
>>  class AlreadyLocked(portage.exception.PortageException):
>>  pass
> 
> 
> Looks fine to me :)
> 

Thanks, merged:

https://gitweb.gentoo.org/proj/portage.git/commit/?id=adf2e6dd57b5bcaa4a668e3085024ebc3224a2ca

-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature

Re: [gentoo-portage-dev] [PATCH] BinpkgFetcher: use async lock (bug 614110)

2018-04-21 Thread Brian Dolbec 
On Sat, 21 Apr 2018 12:27:28 -0700
Zac Medico  wrote:

> In order to avoid event loop recursion, convert the
> _BinpkgFetcherProcess.lock() method to an async_lock
> method for use by BinpkgFetcher.
> 
> Bug: https://bugs.gentoo.org/614110
> ---
>  pym/_emerge/BinpkgFetcher.py | 53
> +++- 1 file changed, 33
> insertions(+), 20 deletions(-)
> 
> diff --git a/pym/_emerge/BinpkgFetcher.py
> b/pym/_emerge/BinpkgFetcher.py index 5ca7a45cf..2bbc0a26f 100644
> --- a/pym/_emerge/BinpkgFetcher.py
> +++ b/pym/_emerge/BinpkgFetcher.py
> @@ -32,11 +32,24 @@ class BinpkgFetcher(CompositeTask):
>   pkg.cpv) + ".partial"
>  
>   def _start(self):
> - self._start_task(
> -
> _BinpkgFetcherProcess(background=self.background,
> - logfile=self.logfile, pkg=self.pkg,
> pkg_path=self.pkg_path,
> - pretend=self.pretend,
> scheduler=self.scheduler),
> - self._fetcher_exit)
> + fetcher =
> _BinpkgFetcherProcess(background=self.background,
> + logfile=self.logfile, pkg=self.pkg,
> pkg_path=self.pkg_path,
> + pretend=self.pretend,
> scheduler=self.scheduler) +
> + if not self.pretend:
> +
> portage.util.ensure_dirs(os.path.dirname(self.pkg_path))
> + if "distlocks" in
> self.pkg.root_config.settings.features:
> + self._start_task(
> +
> AsyncTaskFuture(future=fetcher.async_lock()),
> +
> functools.partial(self._start_locked, fetcher))
> + return
> +
> + self._start_task(fetcher, self._fetcher_exit)
> +
> + def _start_locked(self, fetcher, lock_task):
> + self._assert_current(lock_task)
> + lock_task.future.result()
> + self._start_task(fetcher, self._fetcher_exit)
>  
>   def _fetcher_exit(self, fetcher):
>   self._assert_current(fetcher)
> @@ -68,13 +81,8 @@ class _BinpkgFetcherProcess(SpawnProcess):
>   pretend = self.pretend
>   bintree = pkg.root_config.trees["bintree"]
>   settings = bintree.settings
> - use_locks = "distlocks" in settings.features
>   pkg_path = self.pkg_path
>  
> - if not pretend:
> -
> portage.util.ensure_dirs(os.path.dirname(pkg_path))
> - if use_locks:
> - self.lock()
>   exists = os.path.exists(pkg_path)
>   resume = exists and os.path.basename(pkg_path) in
> bintree.invalids if not (pretend or resume):
> @@ -184,7 +192,7 @@ class _BinpkgFetcherProcess(SpawnProcess):
>   except
> OSError: pass
>  
> - def lock(self):
> + def async_lock(self):
>   """
>   This raises an AlreadyLocked exception if lock() is
> called while a lock is already held. In order to avoid this, call
> @@ -194,17 +202,22 @@ class _BinpkgFetcherProcess(SpawnProcess):
>   if self._lock_obj is not None:
>   raise self.AlreadyLocked((self._lock_obj,))
>  
> - async_lock = AsynchronousLock(path=self.pkg_path,
> - scheduler=self.scheduler)
> - async_lock.start()
> + result = self.scheduler.create_future()
>  
> - if async_lock.wait() != os.EX_OK:
> - # TODO: Use CompositeTask for better
> handling, like in EbuildPhase.
> - raise AssertionError("AsynchronousLock
> failed with returncode %s" \
> - % (async_lock.returncode,))
> + def acquired_lock(async_lock):
> + if async_lock.wait() == os.EX_OK:
> + self.locked = True
> + result.set_result(None)
> + else:
> + result.set_exception(AssertionError(
> + "AsynchronousLock failed
> with returncode %s"
> + % (async_lock.returncode,)))
>  
> - self._lock_obj = async_lock
> - self.locked = True
> + self._lock_obj = AsynchronousLock(path=self.pkg_path,
> + scheduler=self.scheduler)
> + self._lock_obj.addExitListener(acquired_lock)
> + self._lock_obj.start()
> + return result
>  
>   class AlreadyLocked(portage.exception.PortageException):
>   pass


Looks fine to me :)
-- 
Brian Dolbec