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

Cache dir and temp dir are not guaranteed to be on the same mount #4

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

joepvandijken
Copy link

The problem is when for example you have a container and have the cache directory bind mounted thus being on a different file system than the temp directory of the container. This makes the os.rename fail. The proposed fix in this merge request just creates the temp directory inside the cache directory next to the values directory. This should not use any additional resources because the tempfile would eventually get in this directory anyway. The only problem I see here is that I don't know a simple way to create a test for this case.

@youtux
Copy link
Collaborator

youtux commented Apr 1, 2021

Right, it makes sense. The only thing I don't like too much is that there is no automatic garbage collection for this folder, which can eventually grow big with time (if the process is killed while filling the temporary file).

Can you think of a solution for that? If not, we should probably merge this anyway.

About the test, I guess we would need a more capable testing infrastructure, where we can define these kind of scenarios. But yeah, not going to happen now, so this feature can stay untested :).

Please also leave comments in the code explaining why we use a self.temp_dir.

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.

2 participants