From bff1de69a577b8118b4eb75f987d3f701cff19a2 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Tue, 12 Jul 2022 17:03:48 +0200 Subject: file_cache.py: idempotent remove without races (#31477) There's a race condition in `remove()` as the lockfile is removed after releasing the lock, which is a problem when another process acquires a write lock during deletion. Also simplify life a bit in multiprocessing when a file is possibly removed multiple times, which currently is an error on the second deletion, so the proposed fix is to make remove(...) idempotent and not error when deleting non-existing cache entries. Don't tests for existence of lockfile, cause windows/linux behavior is different --- lib/spack/spack/test/util/file_cache.py | 14 ++++++++++++-- lib/spack/spack/util/file_cache.py | 9 +++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/lib/spack/spack/test/util/file_cache.py b/lib/spack/spack/test/util/file_cache.py index 106a411b69..815b21fb38 100644 --- a/lib/spack/spack/test/util/file_cache.py +++ b/lib/spack/spack/test/util/file_cache.py @@ -55,9 +55,12 @@ def test_write_and_remove_cache_file(file_cache): file_cache.remove('test.yaml') - # After removal both the file and the lock file should not exist + # After removal the file should not exist assert not os.path.exists(file_cache.cache_path('test.yaml')) - assert not os.path.exists(file_cache._lock_path('test.yaml')) + + # Whether the lock file exists is more of an implementation detail, on Linux they + # continue to exist, on Windows they don't. + # assert os.path.exists(file_cache._lock_path('test.yaml')) @pytest.mark.skipif(sys.platform == 'win32', @@ -94,3 +97,10 @@ def test_cache_write_readonly_cache_fails(file_cache): with pytest.raises(CacheError, match='Insufficient permissions to write'): file_cache.write_transaction(filename) + + +@pytest.mark.regression('31475') +def test_delete_is_idempotent(file_cache): + """Deleting a non-existent key should be idempotent, to simplify life when + running delete with multiple processes""" + file_cache.remove('test.yaml') diff --git a/lib/spack/spack/util/file_cache.py b/lib/spack/spack/util/file_cache.py index cf5ed8310e..611f428881 100644 --- a/lib/spack/spack/util/file_cache.py +++ b/lib/spack/spack/util/file_cache.py @@ -3,6 +3,7 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) +import errno import os import shutil @@ -170,13 +171,17 @@ class FileCache(object): return sinfo.st_mtime def remove(self, key): + file = self.cache_path(key) lock = self._get_lock(key) try: lock.acquire_write() - os.unlink(self.cache_path(key)) + os.unlink(file) + except OSError as e: + # File not found is OK, so remove is idempotent. + if e.errno != errno.ENOENT: + raise finally: lock.release_write() - lock.cleanup() class CacheError(SpackError): -- cgit v1.2.3-70-g09d2