Skip to content

Commit

Permalink
FileCache: Delete the new cache file on exception (#34623)
Browse files Browse the repository at this point in the history
The code in FileCache for write_transaction attempts to delete the temporary file when an exception occurs under the context by calling shutil.rmtree. However, rmtree only operates on directories while the rest of FileCache uses normal files. This causes an empty file to be written to the cache key when unneeded.

Use os.remove instead which operates on normal files.

Co-authored-by: Harmen Stoppels <harmenstoppels@gmail.com>
  • Loading branch information
2 people authored and alalazo committed Feb 7, 2023
1 parent 2f3f4ad commit 390112f
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
21 changes: 21 additions & 0 deletions lib/spack/spack/test/util/file_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,27 @@ def test_write_and_read_cache_file(file_cache):
assert text == "foobar\n"


@pytest.mark.skipif(sys.platform == "win32", reason="Locks not supported on Windows")
def test_failed_write_and_read_cache_file(file_cache):
"""Test failing to write then attempting to read a cached file."""
with pytest.raises(RuntimeError, match=r"^foobar$"):
with file_cache.write_transaction("test.yaml") as (old, new):
assert old is None
assert new is not None
raise RuntimeError("foobar")

# Cache dir should have exactly one (lock) file
assert os.listdir(file_cache.root) == [".test.yaml.lock"]

# File does not exist
assert not file_cache.init_entry("test.yaml")

# Attempting to read will cause a file not found error
with pytest.raises((IOError, OSError), match=r"test\.yaml"):
with file_cache.read_transaction("test.yaml"):
pass


def test_write_and_remove_cache_file(file_cache):
"""Test two write transactions on a cached file. Then try to remove an
entry from it.
Expand Down
3 changes: 1 addition & 2 deletions lib/spack/spack/util/file_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,7 @@ def __exit__(cm, type, value, traceback):
cm.tmp_file.close()

if value:
# remove tmp on exception & raise it
shutil.rmtree(cm.tmp_filename, True)
os.remove(cm.tmp_filename)

else:
rename(cm.tmp_filename, cm.orig_filename)
Expand Down

0 comments on commit 390112f

Please sign in to comment.