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.

lib/spack/llnl/util/lock.py

@@ -99,10 +99,12 @@ def acquire_read(self, timeout=_default_timeout):
         the POSIX lock, False if it is a nested transaction.
 
         """
+        if self._reads == 0 and self._writes == 0:
+            self._lock(fcntl.LOCK_SH, timeout)   # can raise LockError.
             self._reads += 1
-        if self._reads == 1 and self._writes == 0:
-            self._lock(fcntl.LOCK_SH, timeout)
             return True
+        else:
+            self._reads += 1
             return False
 
 
@@ -117,10 +119,12 @@ def acquire_write(self, timeout=_default_timeout):
         the POSIX lock, False if it is a nested transaction.
 
         """
+        if self._writes == 0:
+            self._lock(fcntl.LOCK_EX, timeout)   # can raise LockError.
             self._writes += 1
-        if self._writes == 1:
-            self._lock(fcntl.LOCK_EX, timeout)
             return True
+        else:
+            self._writes += 1
             return False
 
 
@@ -136,10 +140,12 @@ def release_read(self):
         """
         assert self._reads > 0
 
+        if self._reads == 1 and self._writes == 0:
+            self._unlock()      # can raise LockError.
             self._reads -= 1
-        if self._reads == 0 and self._writes == 0:
-            self._unlock()
             return True
+        else:
+            self._reads -= 1
             return False
 
 
@@ -155,10 +161,12 @@ def release_write(self):
         """
         assert self._writes > 0
 
+        if self._writes == 1 and self._reads == 0:
+            self._unlock()      # can raise LockError.
             self._writes -= 1
-        if self._writes == 0 and self._reads == 0:
-            self._unlock()
             return True
+        else:
+            self._writes -= 1
             return False
 
 

lib/spack/spack/package.py

@@ -606,6 +606,7 @@ def remove_prefix(self):
         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 @@ def real_work():
                     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 @@ def real_work():
         # 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)
 

lib/spack/spack/test/lock.py

@@ -41,14 +41,6 @@
 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 @@ def tearDown(self):
          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 @@ def timeout_read(self, barrier):
     # 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 @@ def test_write_lock_timeout_on_write_3(self):
     # 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 @@ def p3(barrier):
             barrier.wait() # ---------------------------------------- 13
             lock.release_read()
 
-        order_processes(p1, p2, p3)
+        self.multiproc_test(p1, p2, p3)