Fix stupid lock bug.

- Code simplification ignored case where exception was raised.
- If LockError was raised, read and write counts were incremented erroneously.
- updated lock test.

3 files changed, 53 insertions, 41 deletions

diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py
index dcca37687e..ac3684bd55 100644
--- a/lib/spack/llnl/util/lock.py
+++ b/lib/spack/llnl/util/lock.py
@@ -99,11 +99,13 @@ class Lock(object):
         the POSIX lock, False if it is a nested transaction.
 
         """
-        self._reads += 1
-        if self._reads == 1 and self._writes == 0:
-            self._lock(fcntl.LOCK_SH, timeout)
+        if self._reads == 0 and self._writes == 0:
+            self._lock(fcntl.LOCK_SH, timeout)   # can raise LockError.
+            self._reads += 1
             return True
-        return False
+        else:
+            self._reads += 1
+            return False
 
 
     def acquire_write(self, timeout=_default_timeout):
@@ -117,11 +119,13 @@ class Lock(object):
         the POSIX lock, False if it is a nested transaction.
 
         """
-        self._writes += 1
-        if self._writes == 1:
-            self._lock(fcntl.LOCK_EX, timeout)
+        if self._writes == 0:
+            self._lock(fcntl.LOCK_EX, timeout)   # can raise LockError.
+            self._writes += 1
             return True
-        return False
+        else:
+            self._writes += 1
+            return False
 
 
     def release_read(self):
@@ -136,11 +140,13 @@ class Lock(object):
         """
         assert self._reads > 0
 
-        self._reads -= 1
-        if self._reads == 0 and self._writes == 0:
-            self._unlock()
+        if self._reads == 1 and self._writes == 0:
+            self._unlock()      # can raise LockError.
+            self._reads -= 1
             return True
-        return False
+        else:
+            self._reads -= 1
+            return False
 
 
     def release_write(self):
@@ -155,11 +161,13 @@ class Lock(object):
         """
         assert self._writes > 0
 
-        self._writes -= 1
-        if self._writes == 0 and self._reads == 0:
-            self._unlock()
+        if self._writes == 1 and self._reads == 0:
+            self._unlock()      # can raise LockError.
+            self._writes -= 1
             return True
-        return False
+        else:
+            self._writes -= 1
+            return False
 
 
 class LockError(Exception):
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index e6944ce40c..2957257b1a 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -606,6 +606,7 @@ class Package(object):
         spack.install_layout.remove_install_directory(self.spec)
         spack.installed_db.remove(self.spec)
 
+
     def do_fetch(self):
         """Creates a stage directory and downloads the taball for this package.
            Working directory will be set to the stage directory.
@@ -812,9 +813,6 @@ class Package(object):
                     log_install_path = spack.install_layout.build_log_path(self.spec)
                     install(log_path, log_install_path)
 
-                #Update the database once we know install successful
-                spack.installed_db.add(self.spec, spack.install_layout.path_for_spec(self.spec))
-
                 # On successful install, remove the stage.
                 if not keep_stage:
                     self.stage.destroy()
@@ -845,6 +843,10 @@ class Package(object):
         # Do the build.
         spack.build_environment.fork(self, real_work)
 
+        # note: PARENT of the build process adds the new package to
+        # the database, so that we don't need to re-read from file.
+        spack.installed_db.add(self.spec, spack.install_layout.path_for_spec(self.spec))
+
         # Once everything else is done, run post install hooks
         spack.hooks.post_install(self)
 
diff --git a/lib/spack/spack/test/lock.py b/lib/spack/spack/test/lock.py
index 2e7440bbbc..5664e71b03 100644
--- a/lib/spack/spack/test/lock.py
+++ b/lib/spack/spack/test/lock.py
@@ -41,14 +41,6 @@ from spack.util.multiproc import Barrier
 barrier_timeout = 5
 
 
-def order_processes(*functions):
-    """Order some processes using simple barrier synchronization."""
-    b = Barrier(len(functions), timeout=barrier_timeout)
-    procs = [Process(target=f, args=(b,)) for f in functions]
-    for p in procs: p.start()
-    for p in procs: p.join()
-
-
 class LockTest(unittest.TestCase):
 
     def setUp(self):
@@ -61,6 +53,16 @@ class LockTest(unittest.TestCase):
          shutil.rmtree(self.tempdir, ignore_errors=True)
 
 
+    def multiproc_test(self, *functions):
+        """Order some processes using simple barrier synchronization."""
+        b = Barrier(len(functions), timeout=barrier_timeout)
+        procs = [Process(target=f, args=(b,)) for f in functions]
+        for p in procs: p.start()
+        for p in procs:
+            p.join()
+            self.assertEqual(p.exitcode, 0)
+
+
     #
     # Process snippets below can be composed into tests.
     #
@@ -94,13 +96,13 @@ class LockTest(unittest.TestCase):
     # exclusive lock is held.
     #
     def test_write_lock_timeout_on_write(self):
-        order_processes(self.acquire_write, self.timeout_write)
+        self.multiproc_test(self.acquire_write, self.timeout_write)
 
     def test_write_lock_timeout_on_write_2(self):
-        order_processes(self.acquire_write, self.timeout_write, self.timeout_write)
+        self.multiproc_test(self.acquire_write, self.timeout_write, self.timeout_write)
 
     def test_write_lock_timeout_on_write_3(self):
-        order_processes(self.acquire_write, self.timeout_write, self.timeout_write, self.timeout_write)
+        self.multiproc_test(self.acquire_write, self.timeout_write, self.timeout_write, self.timeout_write)
 
 
     #
@@ -108,42 +110,42 @@ class LockTest(unittest.TestCase):
     # exclusive lock is held.
     #
     def test_read_lock_timeout_on_write(self):
-        order_processes(self.acquire_write, self.timeout_read)
+        self.multiproc_test(self.acquire_write, self.timeout_read)
 
     def test_read_lock_timeout_on_write_2(self):
-        order_processes(self.acquire_write, self.timeout_read, self.timeout_read)
+        self.multiproc_test(self.acquire_write, self.timeout_read, self.timeout_read)
 
     def test_read_lock_timeout_on_write_3(self):
-        order_processes(self.acquire_write, self.timeout_read, self.timeout_read, self.timeout_read)
+        self.multiproc_test(self.acquire_write, self.timeout_read, self.timeout_read, self.timeout_read)
 
 
     #
     # Test that exclusive locks time out when shared locks are held.
     #
     def test_write_lock_timeout_on_read(self):
-        order_processes(self.acquire_read, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.timeout_write)
 
     def test_write_lock_timeout_on_read_2(self):
-        order_processes(self.acquire_read, self.timeout_write, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.timeout_write, self.timeout_write)
 
     def test_write_lock_timeout_on_read_3(self):
-        order_processes(self.acquire_read, self.timeout_write, self.timeout_write, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.timeout_write, self.timeout_write, self.timeout_write)
 
 
     #
     # Test that exclusive locks time while lots of shared locks are held.
     #
     def test_write_lock_timeout_with_multiple_readers_2_1(self):
-        order_processes(self.acquire_read, self.acquire_read, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.acquire_read, self.timeout_write)
 
     def test_write_lock_timeout_with_multiple_readers_2_2(self):
-        order_processes(self.acquire_read, self.acquire_read, self.timeout_write, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.acquire_read, self.timeout_write, self.timeout_write)
 
     def test_write_lock_timeout_with_multiple_readers_3_1(self):
-        order_processes(self.acquire_read, self.acquire_read, self.acquire_read, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.acquire_read, self.acquire_read, self.timeout_write)
 
     def test_write_lock_timeout_with_multiple_readers_3_2(self):
-        order_processes(self.acquire_read, self.acquire_read, self.acquire_read, self.timeout_write, self.timeout_write)
+        self.multiproc_test(self.acquire_read, self.acquire_read, self.acquire_read, self.timeout_write, self.timeout_write)
 
 
     #
@@ -261,4 +263,4 @@ class LockTest(unittest.TestCase):
             barrier.wait() # ---------------------------------------- 13
             lock.release_read()
 
-        order_processes(p1, p2, p3)
+        self.multiproc_test(p1, p2, p3)