Read-only locks should close fd before opening for write. (#1906)

- Fixes bad file descriptor error in lock acquire, #1904
- Fix bug introduced in previous PR #1857
- Backported fix from soon-to-be merged fine-grained DB locking branch.

lib/spack/llnl/util/lock.py

@@ -69,6 +69,14 @@ def _lock(self, op, timeout):
         start_time = time.time()
         while (time.time() - start_time) < timeout:
             try:
+                # If this is already open read-only and we want to
+                # upgrade to an exclusive write lock, close first.
+                if self._fd is not None:
+                    flags = fcntl.fcntl(self._fd, fcntl.F_GETFL)
+                    if op == fcntl.LOCK_EX and flags | os.O_RDONLY:
+                        os.close(self._fd)
+                        self._fd = None
+
                 if self._fd is None:
                     mode = os.O_RDWR if op == fcntl.LOCK_EX else os.O_RDONLY
                     self._fd = os.open(self._file_path, mode)

lib/spack/spack/test/lock.py

@@ -157,6 +157,35 @@ def test_write_lock_timeout_with_multiple_readers_3_2(self):
             self.acquire_read, self.acquire_read, self.acquire_read,
             self.timeout_write, self.timeout_write)
 
+    #
+    # Test that read can be upgraded to write.
+    #
+    def test_upgrade_read_to_write(self):
+        # ensure lock file exists the first time, so we open it read-only
+        # to begin wtih.
+        touch(self.lock_path)
+
+        lock = Lock(self.lock_path)
+        self.assertTrue(lock._reads == 0)
+        self.assertTrue(lock._writes == 0)
+
+        lock.acquire_read()
+        self.assertTrue(lock._reads == 1)
+        self.assertTrue(lock._writes == 0)
+
+        lock.acquire_write()
+        self.assertTrue(lock._reads == 1)
+        self.assertTrue(lock._writes == 1)
+
+        lock.release_write()
+        self.assertTrue(lock._reads == 1)
+        self.assertTrue(lock._writes == 0)
+
+        lock.release_read()
+        self.assertTrue(lock._reads == 0)
+        self.assertTrue(lock._writes == 0)
+        self.assertTrue(lock._fd is None)
+
     #
     # Longer test case that ensures locks are reusable. Ordering is
     # enforced by barriers throughout -- steps are shown with numbers.