fix race conditions causing notices if directory does not exist

[kkmuffme]

No description provided.

[kkmuffme]

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]

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]

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]

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]

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]

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]

• 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]

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]

Seems fine :)

Thanks!