Skip to content

Commit

Permalink
Ensure proper cache initialization before writing
Browse files Browse the repository at this point in the history
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 pytest-dev#12167.
  • Loading branch information
tamird committed Mar 29, 2024
1 parent 12e061e commit f2dfbb4
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 23 deletions.
46 changes: 26 additions & 20 deletions src/_pytest/cacheprovider.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
14 changes: 11 additions & 3 deletions testing/test_cacheprovider.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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, "w") as f:
assert f.read() == filename
f.write("filename")


def test_cachedir_tag(pytester: Pytester) -> None:
Expand Down

0 comments on commit f2dfbb4

Please sign in to comment.