lock transactions: fix non-transactional writes

Lock transactions were actually writing *after* the lock was
released. The code was looking at the result of `release_write()` before
writing, then writing based on whether the lock was released.  This is
pretty obviously wrong.

- [x] Refactor `Lock` so that a release function can be passed to the
      `Lock` and called *only* when a lock is really released.

- [x] Refactor `LockTransaction` classes to use the release function
  instead of checking the return value of `release_read()` / `release_write()`

lib/spack/llnl/util/lock.py

@@ -95,10 +95,6 @@ def _lock(self, op, timeout=None):
         The lock is implemented as a spin lock using a nonblocking call
         to ``lockf()``.
 
-        On acquiring an exclusive lock, the lock writes this process's
-        pid and host to the lock file, in case the holding process needs
-        to be killed later.
-
         If the lock times out, it raises a ``LockError``. If the lock is
         successfully acquired, the total wait time and the number of attempts
         is returned.
@@ -284,11 +280,19 @@ def acquire_write(self, timeout=None):
             self._writes += 1
             return False
 
-    def release_read(self):
+    def release_read(self, release_fn=None):
         """Releases a read lock.
 
-        Returns True if the last recursive lock was released, False if
-        there are still outstanding locks.
+        Arguments:
+            release_fn (callable): function to call *before* the last recursive
+                lock (read or write) is released.
+
+        If the last recursive lock will be released, then this will call
+        release_fn and return its result (if provided), or return True
+        (if release_fn was not provided).
+
+        Otherwise, we are still nested inside some other lock, so do not
+        call the release_fn and, return False.
 
         Does limited correctness checking: if a read lock is released
         when none are held, this will raise an assertion error.
@@ -300,18 +304,30 @@ def release_read(self):
             self._debug(
                 'READ LOCK: {0.path}[{0._start}:{0._length}] [Released]'
                 .format(self))
+
+            result = True
+            if release_fn is not None:
+                result = release_fn()
+
             self._unlock()      # can raise LockError.
             self._reads -= 1
-            return True
+            return result
         else:
             self._reads -= 1
             return False
 
-    def release_write(self):
+    def release_write(self, release_fn=None):
         """Releases a write lock.
 
-        Returns True if the last recursive lock was released, False if
-        there are still outstanding locks.
+        Arguments:
+            release_fn (callable): function to call before the last recursive
+                write is released.
+
+        If the last recursive *write* lock will be released, then this
+        will call release_fn and return its result (if provided), or
+        return True (if release_fn was not provided). Otherwise, we are
+        still nested inside some other write lock, so do not call the
+        release_fn, and return False.
 
         Does limited correctness checking: if a read lock is released
         when none are held, this will raise an assertion error.
@@ -323,9 +339,16 @@ def release_write(self):
             self._debug(
                 'WRITE LOCK: {0.path}[{0._start}:{0._length}] [Released]'
                 .format(self))
+
+            # we need to call release_fn before releasing the lock
+            result = True
+            if release_fn is not None:
+                result = release_fn()
+
             self._unlock()      # can raise LockError.
             self._writes -= 1
-            return True
+            return result
+
         else:
             self._writes -= 1
             return False
@@ -349,28 +372,36 @@ def _acquired_debug(self, lock_type, wait_time, nattempts):
 class LockTransaction(object):
     """Simple nested transaction context manager that uses a file lock.
 
-    This class can trigger actions when the lock is acquired for the
-    first time and released for the last.
+    Arguments:
+        lock (Lock): underlying lock for this transaction to be accquired on
+            enter and released on exit
+        acquire (callable or contextmanager): function to be called after lock
+            is acquired, or contextmanager to enter after acquire and leave
+            before release.
+        release (callable): function to be called before release. If
+            ``acquire`` is a contextmanager, this will be called *after*
+            exiting the nexted context and before the lock is released.
+        timeout (float): number of seconds to set for the timeout when
+            accquiring the lock (default no timeout)
 
     If the ``acquire_fn`` returns a value, it is used as the return value for
     ``__enter__``, allowing it to be passed as the ``as`` argument of a
     ``with`` statement.
 
     If ``acquire_fn`` returns a context manager, *its* ``__enter__`` function
-    will be called in ``__enter__`` after ``acquire_fn``, and its ``__exit__``
-    funciton will be called before ``release_fn`` in ``__exit__``, allowing you
-    to nest a context manager to be used along with the lock.
+    will be called after the lock is acquired, and its ``__exit__`` funciton
+    will be called before ``release_fn`` in ``__exit__``, allowing you to
+    nest a context manager inside this one.
 
     Timeout for lock is customizable.
 
     """
 
-    def __init__(self, lock, acquire_fn=None, release_fn=None,
-                 timeout=None):
+    def __init__(self, lock, acquire=None, release=None, timeout=None):
         self._lock = lock
         self._timeout = timeout
-        self._acquire_fn = acquire_fn
-        self._release_fn = release_fn
+        self._acquire_fn = acquire
+        self._release_fn = release
         self._as = None
 
     def __enter__(self):
@@ -383,13 +414,18 @@ def __enter__(self):
 
     def __exit__(self, type, value, traceback):
         suppress = False
-        if self._exit():
-            if self._as and hasattr(self._as, '__exit__'):
-                if self._as.__exit__(type, value, traceback):
-                    suppress = True
-            if self._release_fn:
-                if self._release_fn(type, value, traceback):
-                    suppress = True
+
+        def release_fn():
+            if self._release_fn is not None:
+                return self._release_fn(type, value, traceback)
+
+        if self._as and hasattr(self._as, '__exit__'):
+            if self._as.__exit__(type, value, traceback):
+                suppress = True
+
+        if self._exit(release_fn):
+            suppress = True
+
         return suppress
 
 
@@ -398,8 +434,8 @@ class ReadTransaction(LockTransaction):
     def _enter(self):
         return self._lock.acquire_read(self._timeout)
 
-    def _exit(self):
-        return self._lock.release_read()
+    def _exit(self, release_fn):
+        return self._lock.release_read(release_fn)
 
 
 class WriteTransaction(LockTransaction):
@@ -407,8 +443,8 @@ class WriteTransaction(LockTransaction):
     def _enter(self):
         return self._lock.acquire_write(self._timeout)
 
-    def _exit(self):
-        return self._lock.release_write()
+    def _exit(self, release_fn):
+        return self._lock.release_write(release_fn)
 
 
 class LockError(Exception):