File delete fix on high load

[sartor]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌

On high load projects sometimes there is an error trying to delete file. This happens because of not atomic check if file exists and actual delete:

if (!is_file($file)) {
    return true;
}

return @unlink($file);

In old yii2 file cache result of key delete not throw any error.
This PR tries to return false only if unlink call error caused by any reason except file already deleted
I can't do simple test to prove fix, but I've checked it by calling first unlink just after is_file() check

[what-the-diff]

• Added a check to see if the file was already deleted by another process on high load.
• If it is, then return true and clear last error message so that we don't get an exception thrown in our codebase for this case.

[codecov]

Codecov Report

Base: 95.74% // Head: 93.19% // Decreases project coverage by -2.54% ⚠️

Coverage data is based on head (10ff8f1) compared to base (6c0eda6).
Patch coverage: 42.85% of modified lines in pull request are covered.

❗ Current head 10ff8f1 differs from pull request most recent head b59ca98. Consider uploading reports for the commit b59ca98 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #50      +/-   ##
============================================
- Coverage     95.74%   93.19%   -2.55%     
- Complexity       66       68       +2     
============================================
  Files             1        1              
  Lines           141      147       +6     
============================================
+ Hits            135      137       +2     
- Misses            6       10       +4     

Impacted Files	Coverage Δ
src/FileCache.php	93.19% <42.85%> (-2.55%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rustamwin]

Need tests.

[sartor]

Need tests.

Any ideas how it can be done?

[samdark]

It's hard to test race conditions. I think we can skip these for the case. @sartor would you please add a line for CHANGELOG?

[vjik]

.

[vjik]

Seems, similar problem may occur in method removeCacheFiles():

cache-file/src/FileCache.php

Line 391 in 6c0eda6

if (!$expiredOnly && !@rmdir($fullPath)) {

cache-file/src/FileCache.php

Line 395 in 6c0eda6

} elseif ((!$expiredOnly || @filemtime($fullPath) < time()) && !@unlink($fullPath)) {

[samdark]

Yes, likely.

[sartor]

Yes. I'm using on prod my fork for a week, and fixed line weren't produced any new errors.
But there are similar errors left in other places. I'll fix them later in another PR