summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorHarmen Stoppels <harmenstoppels@gmail.com>2022-07-12 17:03:48 +0200
committerGitHub <noreply@github.com>2022-07-12 15:03:48 +0000
commitea91e8f4ca1790598906a0363d9f90852f9a74d3 (patch)
tree7201cd8aff1037607ad92debd9c1ab3c1345f3c6
parent74e2625dcf61639fa6d63892115eb4ab35b85e58 (diff)
downloadspack-ea91e8f4ca1790598906a0363d9f90852f9a74d3.tar.gz
spack-ea91e8f4ca1790598906a0363d9f90852f9a74d3.tar.bz2
spack-ea91e8f4ca1790598906a0363d9f90852f9a74d3.tar.xz
spack-ea91e8f4ca1790598906a0363d9f90852f9a74d3.zip
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
-rw-r--r--lib/spack/spack/test/util/file_cache.py14
-rw-r--r--lib/spack/spack/util/file_cache.py9
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):