Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialize cache directory in isolation #12168

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Mar 29, 2024

Creating and initializing the cache directory is interruptible; this
avoids 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.

Closes #12167.

@tamird tamird force-pushed the fix-gitignore-missing branch 13 times, most recently from 1710bb5 to 62f895d Compare March 29, 2024 19:18
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @tamird, appreciate the PR!

Left a few comments, please take a look.

Also, please include a changelog entry in changelog/12167.trivial.rst:

cache: ensure supporting files (``CACHEDIR.TAG``, ``.gitignore``, etc) are always created in the cache directory, even in the event of test session being interrupted.

src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Show resolved Hide resolved
testing/test_cacheprovider.py Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Show resolved Hide resolved
@tamird tamird force-pushed the fix-gitignore-missing branch 6 times, most recently from 78fc304 to 2374cf7 Compare April 1, 2024 11:06
@tamird tamird changed the title Ensure proper cache initialization before writing Initialize cache before writing data Apr 1, 2024
@tamird tamird changed the title Initialize cache before writing data Initialize cache directory in isolation Apr 1, 2024
@tamird tamird force-pushed the fix-gitignore-missing branch 5 times, most recently from d42c4fc to c24b40b Compare April 1, 2024 17:42
src/_pytest/cacheprovider.py Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
changelog/12167.trivial.rst Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
@tamird
Copy link
Contributor Author

tamird commented Apr 2, 2024

TBH I'm starting to see this change as -0: we are changing how the supporting files are being created for little reason (as the original reason for the issue is no longer valid). I fear we will be introducing subtle bugs for little gain.

The original reason is as valid as it ever was - I saw this symptom on my local machine. It wasn't communicated properly to start, but validity hasn't changed.

@nicoddemus
Copy link
Member

The original reason is as valid as it ever was - I saw this symptom on my local machine

An interruption between mkdir and writing the files? This seems highly unlikely to happen, those lines are next to each other -- the current code will solve this indeed, but I fear we will be introducing subtle issues and breaking test suites for minimal/marginal gains, hence -0.

But if @bluetech prefers, then definitely move forward with this.

@nicoddemus nicoddemus requested review from nicoddemus and removed request for nicoddemus April 2, 2024 12:22
@nicoddemus
Copy link
Member

I left a few comments which are good to apply, but I will remove myself from reviewer and defer the final decision here to @bluetech. 👍

Regardless of the outcome, thanks @tamird for the PR, we appreciate it. 👍

src/_pytest/cacheprovider.py Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
@tamird tamird force-pushed the fix-gitignore-missing branch 2 times, most recently from 4f938d3 to 97ec412 Compare April 3, 2024 10:34
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at first glance the proposed mechanism is broken in most linux deployments simply by degrading to copytree on anything that has TMP on tmpfs

src/_pytest/cacheprovider.py Outdated Show resolved Hide resolved
Creating and initializing the cache directory is interruptible; this
avoids 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.

Closes pytest-dev#12167.
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given how temporarydirectory actually is used in our case,
a followup at a later point might want to replace it with a simpler mechanism

good work

@tamird
Copy link
Contributor Author

tamird commented Apr 6, 2024

@nicoddemus could you have a look? I believe this is blocked on your approval.

@nicoddemus
Copy link
Member

It is not blocked, I will defer merging to @bluetech or @RonnyPfannschmidt. 👍

@tamird
Copy link
Contributor Author

tamird commented Apr 6, 2024

Screenshot 2024-04-06 at 13 33 27

@nicoddemus nicoddemus removed their request for review April 6, 2024 13:34
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh my bad, did not realize that.

Relutanctly approving, as I mentioned I'm -0 on this -- need to mark as "Approve" to pass the merge requirements.

@tamird
Copy link
Contributor Author

tamird commented Apr 6, 2024

Now I just need someone to click merge 😄

@bluetech bluetech merged commit 5acc3f8 into pytest-dev:main Apr 6, 2024
24 checks passed
@tamird tamird deleted the fix-gitignore-missing branch April 6, 2024 20:27
@tamird
Copy link
Contributor Author

tamird commented Apr 8, 2024

Turns out the behavior I was seeing which prompted me to send this PR was in fact caused by mkdir not properly initializing the cache directory. Plugins (such as pytest-insta) use this function; if they are the first user, then the directory does not get initialized properly. Anyway, this PR fixes that, but we should probably add a test -- I'll send another PR shortly.

@tamird tamird mentioned this pull request Apr 8, 2024
@tamird
Copy link
Contributor Author

tamird commented Apr 8, 2024

#12199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

race condition between creation of .pytest_cache and .pytest_cache/.gitignore
5 participants