From e7eff684307058d3f07eb7150b68f5738ee9c537 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Fri, 29 Mar 2024 15:51:51 +0000 Subject: [PATCH] Ensure proper cache initialization before writing Writing cache data is interruptible; this prevents a pathological case where interrupting a cache write can cause the cache directory to never be properly initialized with its supporting files. Unify `Cache.mkdir` with `Cache.set` while I'm here so the former also properly initializes the cache directory. Use json.dump instead of json.dumps + write for a tiny perf win. Fixes #12167. --- src/_pytest/cacheprovider.py | 46 ++++++++++++++++++++--------------- testing/test_cacheprovider.py | 14 ++++++++--- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/_pytest/cacheprovider.py b/src/_pytest/cacheprovider.py index 81703ddac44..fa97373c09c 100755 --- a/src/_pytest/cacheprovider.py +++ b/src/_pytest/cacheprovider.py @@ -21,6 +21,7 @@ from .reports import CollectReport from _pytest import nodes from _pytest._io import TerminalWriter +from _pytest.compat import assert_never from _pytest.config import Config from _pytest.config import ExitCode from _pytest.config import hookimpl @@ -123,6 +124,10 @@ def warn(self, fmt: str, *, _ispytest: bool = False, **args: object) -> None: stacklevel=3, ) + def _mkdir(self, path: Path) -> None: + self._ensure_cache_dir_and_supporting_files() + path.mkdir(exist_ok=True, parents=True) + def mkdir(self, name: str) -> Path: """Return a directory path object with the given name. @@ -141,7 +146,7 @@ def mkdir(self, name: str) -> Path: if len(path.parts) > 1: raise ValueError("name is not allowed to contain path separators") res = self._cachedir.joinpath(self._CACHE_PREFIX_DIRS, path) - res.mkdir(exist_ok=True, parents=True) + self._mkdir(res) return res def _getvaluepath(self, key: str) -> Path: @@ -178,20 +183,13 @@ def set(self, key: str, value: object) -> None: """ path = self._getvaluepath(key) try: - if path.parent.is_dir(): - cache_dir_exists_already = True - else: - cache_dir_exists_already = self._cachedir.exists() - path.parent.mkdir(exist_ok=True, parents=True) + self._mkdir(path.parent) except OSError as exc: self.warn( f"could not create cache path {path}: {exc}", _ispytest=True, ) return - if not cache_dir_exists_already: - self._ensure_supporting_files() - data = json.dumps(value, ensure_ascii=False, indent=2) try: f = path.open("w", encoding="UTF-8") except OSError as exc: @@ -201,19 +199,27 @@ def set(self, key: str, value: object) -> None: ) else: with f: - f.write(data) - - def _ensure_supporting_files(self) -> None: - """Create supporting files in the cache dir that are not really part of the cache.""" - readme_path = self._cachedir / "README.md" - readme_path.write_text(README_CONTENT, encoding="UTF-8") + json.dump(value, f, ensure_ascii=False, indent=2) - gitignore_path = self._cachedir.joinpath(".gitignore") - msg = "# Created by pytest automatically.\n*\n" - gitignore_path.write_text(msg, encoding="UTF-8") + def _ensure_cache_dir_and_supporting_files(self) -> None: + """Create the cache dir and its supporting files.""" + self._cachedir.mkdir(exist_ok=True, parents=True) - cachedir_tag_path = self._cachedir.joinpath("CACHEDIR.TAG") - cachedir_tag_path.write_bytes(CACHEDIR_TAG_CONTENT) + files: Iterable[tuple[str, str | bytes]] = ( + ("README.md", README_CONTENT), + (".gitignore", "# Created by pytest automatically.\n*\n"), + ("CACHEDIR.TAG", CACHEDIR_TAG_CONTENT), + ) + for file, content in files: + path = self._cachedir.joinpath(file) + if path.exists(): + continue + if isinstance(content, str): + path.write_text(content) + elif isinstance(content, bytes): + path.write_bytes(content) + else: + assert_never(content) class LFPluginCollWrapper: diff --git a/testing/test_cacheprovider.py b/testing/test_cacheprovider.py index c020b77f978..c7e52474b5c 100644 --- a/testing/test_cacheprovider.py +++ b/testing/test_cacheprovider.py @@ -1282,9 +1282,13 @@ def test_preserve_keys_order(pytester: Pytester) -> None: assert list(read_back.items()) == [("z", 1), ("b", 2), ("a", 3), ("d", 10)] -def test_does_not_create_boilerplate_in_existing_dirs(pytester: Pytester) -> None: +def test_does_not_create_overwrite_boilerplate(pytester: Pytester) -> None: from _pytest.cacheprovider import Cache + for filename in ("README.md", ".gitignore"): + with open(filename, "w") as f: + f.write(filename) + pytester.makeini( """ [pytest] @@ -1296,8 +1300,12 @@ def test_does_not_create_boilerplate_in_existing_dirs(pytester: Pytester) -> Non cache.set("foo", "bar") assert os.path.isdir("v") # cache contents - assert not os.path.exists(".gitignore") - assert not os.path.exists("README.md") + + # unchanged + for filename in ("README.md", ".gitignore"): + with open(filename) as f: + assert f.read() == filename + f.write("filename") def test_cachedir_tag(pytester: Pytester) -> None: