From 2167cbf72cdc94d573e1e7af843b0a5dc296e958 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Fri, 25 Nov 2022 10:57:33 +0100 Subject: Track locks by (dev, ino); close file handlers between tests (#34122) --- lib/spack/llnl/util/lock.py | 34 ++++++++++++++++------------------ lib/spack/spack/test/conftest.py | 16 ++++++++++++++-- lib/spack/spack/test/llnl/util/lock.py | 8 +++++++- 3 files changed, 37 insertions(+), 21 deletions(-) (limited to 'lib') diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py index 9c3bcd7a91..cefaa8c253 100644 --- a/lib/spack/llnl/util/lock.py +++ b/lib/spack/llnl/util/lock.py @@ -9,7 +9,6 @@ import socket 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 @@ class OpenFileTracker(object): 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 @@ class OpenFileTracker(object): # 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 @@ class Lock(object): """ 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 diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 0a7a21116a..0449cd8cf8 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -17,7 +17,6 @@ import stat import sys import tempfile import xml.etree.ElementTree -from typing import Dict # novm import py import pytest @@ -26,6 +25,7 @@ import archspec.cpu.microarchitecture 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 @@ repo: 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 diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py index bf812bcc1d..468a4faa3a 100644 --- a/lib/spack/spack/test/llnl/util/lock.py +++ b/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() -- cgit v1.2.3-60-g2f50