Track locks by (dev, ino); close file handlers between tests (#34122)

lib/spack/llnl/util/lock.py

@@ -9,7 +9,6 @@
 import sys
 import time
 from datetime import datetime
-from typing import Dict, Tuple  # novm
 
 import llnl.util.tty as tty
 from llnl.util.lang import pretty_seconds
@@ -81,7 +80,7 @@ class OpenFileTracker(object):
 
     def __init__(self):
         """Create a new ``OpenFileTracker``."""
-        self._descriptors = {}  # type: Dict[Tuple[int, int], OpenFile]
+        self._descriptors = {}
 
     def get_fh(self, path):
         """Get a filehandle for a lockfile.
@@ -103,7 +102,7 @@ def get_fh(self, path):
         try:
             # see whether we've seen this inode/pid before
             stat = os.stat(path)
-            key = (stat.st_ino, pid)
+            key = (stat.st_dev, stat.st_ino, pid)
             open_file = self._descriptors.get(key)
 
         except OSError as e:
@@ -129,32 +128,32 @@ def get_fh(self, path):
 
             # if we just created the file, we'll need to get its inode here
             if not stat:
-                inode = os.fstat(fd).st_ino
-                key = (inode, pid)
+                stat = os.fstat(fd)
+                key = (stat.st_dev, stat.st_ino, pid)
 
             self._descriptors[key] = open_file
 
         open_file.refs += 1
         return open_file.fh
 
-    def release_fh(self, path):
-        """Release a filehandle, only closing it if there are no more references."""
-        try:
-            inode = os.stat(path).st_ino
-        except OSError as e:
-            if e.errno != errno.ENOENT:  # only handle file not found
-                raise
-            inode = None  # this will not be in self._descriptors
-
-        key = (inode, os.getpid())
+    def release_by_stat(self, stat):
+        key = (stat.st_dev, stat.st_ino, os.getpid())
         open_file = self._descriptors.get(key)
-        assert open_file, "Attempted to close non-existing lock path: %s" % path
+        assert open_file, "Attempted to close non-existing inode: %s" % stat.st_inode
 
         open_file.refs -= 1
         if not open_file.refs:
             del self._descriptors[key]
             open_file.fh.close()
 
+    def release_by_fh(self, fh):
+        self.release_by_stat(os.fstat(fh.fileno()))
+
+    def purge(self):
+        for key in list(self._descriptors.keys()):
+            self._descriptors[key].fh.close()
+            del self._descriptors[key]
+
 
 #: Open file descriptors for locks in this process. Used to prevent one process
 #: from opening the sam file many times for different byte range locks
@@ -432,8 +431,7 @@ def _unlock(self):
 
         """
         fcntl.lockf(self._file, fcntl.LOCK_UN, self._length, self._start, os.SEEK_SET)
-
-        file_tracker.release_fh(self.path)
+        file_tracker.release_by_fh(self._file)
         self._file = None
         self._reads = 0
         self._writes = 0

lib/spack/spack/test/conftest.py

@@ -17,7 +17,6 @@
 import sys
 import tempfile
 import xml.etree.ElementTree
-from typing import Dict  # novm
 
 import py
 import pytest
@@ -26,6 +25,7 @@
 import archspec.cpu.schema
 
 import llnl.util.lang
+import llnl.util.lock
 import llnl.util.tty as tty
 from llnl.util.filesystem import copy_tree, mkdirp, remove_linked_tree, working_dir
 
@@ -1640,7 +1640,6 @@ def mock_clone_repo(tmpdir_factory):
 class MockBundle(object):
     has_code = False
     name = "mock-bundle"
-    versions = {}  # type: Dict
 
 
 @pytest.fixture
@@ -1692,6 +1691,19 @@ def mock_test_stage(mutable_config, tmpdir):
     yield tmp_stage
 
 
+@pytest.fixture(autouse=True)
+def inode_cache():
+    llnl.util.lock.file_tracker.purge()
+    yield
+    # TODO: it is a bug when the file tracker is non-empty after a test,
+    # since it means a lock was not released, or the inode was not purged
+    # when acquiring the lock failed. So, we could assert that here, but
+    # currently there are too many issues to fix, so look for the more
+    # serious issue of having a closed file descriptor in the cache.
+    assert not any(f.fh.closed for f in llnl.util.lock.file_tracker._descriptors.values())
+    llnl.util.lock.file_tracker.purge()
+
+
 @pytest.fixture(autouse=True)
 def brand_new_binary_cache():
     yield

lib/spack/spack/test/llnl/util/lock.py

@@ -687,7 +687,9 @@ def test_upgrade_read_to_write_fails_with_readonly_file(private_lock_path):
         # upgrade to write here
         with pytest.raises(lk.LockROFileError):
             lock.acquire_write()
-        lk.file_tracker.release_fh(lock.path)
+
+        # TODO: lk.file_tracker does not release private_lock_path
+        lk.file_tracker.release_by_stat(os.stat(private_lock_path))
 
 
 class ComplexAcquireAndRelease(object):
@@ -1313,6 +1315,7 @@ def test_downgrade_write_okay(tmpdir):
         lock.downgrade_write_to_read()
         assert lock._reads == 1
         assert lock._writes == 0
+        lock.release_read()
 
 
 def test_downgrade_write_fails(tmpdir):
@@ -1323,6 +1326,7 @@ def test_downgrade_write_fails(tmpdir):
         msg = "Cannot downgrade lock from write to read on file: lockfile"
         with pytest.raises(lk.LockDowngradeError, match=msg):
             lock.downgrade_write_to_read()
+        lock.release_read()
 
 
 @pytest.mark.parametrize(
@@ -1362,6 +1366,7 @@ def test_upgrade_read_okay(tmpdir):
         lock.upgrade_read_to_write()
         assert lock._reads == 0
         assert lock._writes == 1
+        lock.release_write()
 
 
 def test_upgrade_read_fails(tmpdir):
@@ -1372,3 +1377,4 @@ def test_upgrade_read_fails(tmpdir):
         msg = "Cannot upgrade lock from read to write on file: lockfile"
         with pytest.raises(lk.LockUpgradeError, match=msg):
             lock.upgrade_read_to_write()
+        lock.release_write()