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

Fix race condition creating parser cache directory (fixes #4483) #5603

Merged
merged 1 commit into from Apr 9, 2021

Conversation

AndrolGenhald
Copy link
Collaborator

I wasn't able to reproduce the error under normal conditions, but I set a breakpoint on the mkdir call, created the directory, then continued, and it seems to handle it correctly.

I wasn't able to reproduce the error under normal conditions, but I set a
breakpoint on the `mkdir` call, created the directory, then continued, and it
seems to handle it correctly.

private function createCacheDirectory(string $parser_cache_directory): void
{
if (!is_dir($parser_cache_directory)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't @mkdir(...) make more sense? After all, we only want the directory to exist. On the other hand this could just move race-condition elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically it could be a file for some reason, in which case an error should be thrown, but I wouldn't really be opposed to changing it to @mkdir(...), as it being a file seems incredibly unlikely.

I suppose you're right that this could just move the race condition somewhere else, I hadn't considered that. I haven't really looked into how the caching works, hopefully different processes aren't going to be using the same $file_cache_key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mkdir could also fail due to lack of write permission, which also seems unlikely, but more likely than the directory path being a file.

@muglug muglug merged commit 9f0d139 into vimeo:master Apr 9, 2021
@muglug
Copy link
Collaborator

muglug commented Apr 9, 2021

Thanks!

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.

None yet

3 participants