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 conditions causing notices if directory does not exist #8302

Merged
merged 2 commits into from Jul 25, 2022

Conversation

kkmuffme
Copy link
Contributor

No description provided.

@kkmuffme kkmuffme force-pushed the fix-cache-directory-race-conditions branch from 2dde842 to 233863d Compare July 21, 2022 09:38
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Jul 21, 2022

Ready for code review & merge.

Circle CI reported an error for a file I didn't modify, so I fixed that too, so tests wouldn't fail.

@AndrolGenhald
Copy link
Collaborator

It looks like this more reduces the probability of running into the race condition rather than actually fixing it. Can we just handle the error returned from failing operations instead?

@kkmuffme
Copy link
Contributor Author

It looks like this more reduces the probability of running into the race condition rather than actually fixing it.

Race conditions in an independent multi-process environment are an unfixable issue in software in general, unless you want to slow everything down with locks (and even then you can get race conditions)

Can we just handle the error returned from failing operations instead?

There already is an error, natively by PHP if any of that fails. And unfortunately, those errors are not actionable, bc there's nothing the user can do about it (well, the user could wait until 1 psalm process is finished before running the next, therefore having to wait ages longer)

This PR fixed the race conditions we had in our use case, which is why I PRed it.

I'm open for any suggestions on how to improve/reduce that code.
Just not handling it and giving an error - like it does already now by default - is not a workable solution.

@AndrolGenhald
Copy link
Collaborator

There already is an error, natively by PHP if any of that fails. And unfortunately, those errors are not actionable, bc there's nothing the user can do about it

Do we actually care about the errors though? Why not just ignore them? If all we care about is making sure a file is removed, instead of

if (file_exists($file)) {
    unlink($file);
}

why not simply

@unlink($file);

?

I'd prefer to actually use try/catch and only catch errors due to the file not existing, but it doesn't seem like that's possible for this. Just ignoring all errors seems like it could be better than leaving the race condition there but reducing its likelihood.

@kkmuffme
Copy link
Contributor Author

Do we actually care about the errors though? Why not just ignore them? If all we care about is making sure a file is removed,
why not simply

@unlink($file);

Bc this will snooze ALL errors. e.g. if unlink fails due to permission problems we will never know about it, bc @ snoozes that error. But we definitely need to know about this error, bc this can be fixed by the user.

@AndrolGenhald
Copy link
Collaborator

Bc this will snooze ALL errors. e.g. if unlink fails due to permission problems we will never know about it, bc @ snoozes that error. But we definitely need to know about this error, bc this can be fixed by the user.

Good point, I wasn't sure if there were any errors we would care about, but a permission error is a good example of one we definitely want to show to the user.

There's some discussion about the issue here, and a closed bug report here, unfortunately it doesn't seem as if there are any immediate plans to improve this in PHP. We might be able to use an example from the stackoverflow post to create a unlinkIfNotExists function though, which should handle most of the problem. There would still need to be some extra logic to handle the case that a new file is created in a directory while it's being removed.

@kkmuffme
Copy link
Contributor Author

  • error_get_last() => possible to do, however by default it ends up in the stderr anyway, so I don't really see the benefit of throwing it again, when it's there already by default (could do try/catch, but then we just add a bunch of code to get what PHP does by default anyway. Since we cannot snooze anything, since we even need to know about race conditions of unlink, in case we need to further improve this)

  • separate function: this issue exists solely in this 1 function, so adding a separate function would just make the code slower

  • I totally agree with you, that this does not fix the race condition issue 100%, however I wasn't able to reproduce the race condition anymore with this. If I (or anybody else) encounters a race condition again with this new code, I'm happy to improve it.
    I just think it's not worth to micro-optimize this now, when there are 1000 of open issues I could work on and PR instead. Better we wait until someone encounters an issue with this again, and improve it then, as this PR should fix it for 99.99% of cases.

@AndrolGenhald
Copy link
Collaborator

I suppose this is good enough for now then, maybe we can come back to it if PHP ever gives us better error handling for unlink. @orklah do you want to look over this before merging?

@orklah
Copy link
Collaborator

orklah commented Jul 25, 2022

Seems fine :)

Thanks!

@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jul 25, 2022
@orklah orklah merged commit d7cd84c into vimeo:4.x Jul 25, 2022
@kkmuffme kkmuffme deleted the fix-cache-directory-race-conditions branch July 29, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants