summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorHarmen Stoppels <harmenstoppels@gmail.com>2022-07-12 17:03:48 +0200
committerMassimiliano Culpo <massimiliano.culpo@gmail.com>2022-07-20 08:10:41 +0200
commitbff1de69a577b8118b4eb75f987d3f701cff19a2 (patch)
tree58f3aefd2ab0b3245078f5bf8d90e4f3d87feee4
parent44f360556d8258e840d8f280ae21fe86fbc13004 (diff)
downloadspack-bff1de69a577b8118b4eb75f987d3f701cff19a2.tar.gz
spack-bff1de69a577b8118b4eb75f987d3f701cff19a2.tar.bz2
spack-bff1de69a577b8118b4eb75f987d3f701cff19a2.tar.xz
spack-bff1de69a577b8118b4eb75f987d3f701cff19a2.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):