From 8e659f512e00534c6af427ef3832db68fcd2f2d3 Mon Sep 17 00:00:00 2001 From: Tom Scogland Date: Wed, 22 Dec 2021 07:25:05 -0800 Subject: locks: allow locks to work under high contention (#27846) * locks: allow locks to work under high contention This is a bug found by Harshitha Menon. The `lock=None` line shouldn't be a release but should be ``` return (lock_type, None) ``` to inform the caller it couldn't get the lock type requested without disturbing the existing lock object in the database. There were also a couple of bugs due to taking write locks at the beginning without any checking or release, and not releasing read locks before requeueing. This version no longer gives me read upgrade to write errors, even running 200 instances on one box. * Change lock in check_deps_status to read, release if not installed, not sure why this was ever write, but read definitely is more appropriate here, and the read lock is only held out of the scope if the package is installed. * Release read lock before requeueing to reduce chance of livelock, the timeout that caused the original issue now happens in roughly 3 of 200 workers instead of 199 on average. --- lib/spack/spack/installer.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index 196858830a..df1d704cd7 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -794,10 +794,10 @@ class PackageInstaller(object): .format(dep_id, action) raise InstallError(err.format(request.pkg_id, msg)) - # Attempt to get a write lock to ensure another process does not + # Attempt to get a read lock to ensure another process does not # uninstall the dependency while the requested spec is being # installed - ltype, lock = self._ensure_locked('write', dep_pkg) + ltype, lock = self._ensure_locked('read', dep_pkg) if lock is None: msg = '{0} is write locked by another process'.format(dep_id) raise InstallError(err.format(request.pkg_id, msg)) @@ -816,6 +816,8 @@ class PackageInstaller(object): tty.debug('Flagging {0} as installed per the database' .format(dep_id)) self._flag_installed(dep_pkg) + else: + lock.release_read() def _prepare_for_install(self, task): """ @@ -1027,7 +1029,7 @@ class PackageInstaller(object): except (lk.LockDowngradeError, lk.LockTimeoutError) as exc: tty.debug(err.format(op, desc, pkg_id, exc.__class__.__name__, str(exc))) - lock = None + return (lock_type, None) except (Exception, KeyboardInterrupt, SystemExit) as exc: tty.error(err.format(op, desc, pkg_id, exc.__class__.__name__, @@ -1626,6 +1628,7 @@ class PackageInstaller(object): # established by the other process -- failed, installed, or # uninstalled -- on the next pass. if ltype == 'read': + lock.release_read() self._requeue_task(task) continue -- cgit v1.2.3-70-g09d2