[11.x] `Cache::flexible` improvements #52891

timacdonald · 2024-09-23T22:43:22Z

Bugfix

Addresses a potential bug in Cache::flexible where the main key has been forgotten, via Cache::forget('foo'), but the TTL key, i.e., foo:created, remains in the cache.

When Cache::flexible is called again, null is returned rather than calling the Closure $callback and refreshing the value in the cache.

There is a race condition in this solution where the $value is intentionally null and the value expires after we retrieve it from the cache but before we check $this->missing($key). I don't really see any real-world problem with refreshing the cache in that situation though. The value just expired after all. Heck, I'd even call it a feature.

Cleanup flexible TTL keys

The database and file driver may leave behind TTL keys when the main key has been forgotten. Both the database and file driver will now attempt to cleanup the flexible TTL keys as well.

TTL key rename

Because we now implicitly clean up flexible TTL keys in the database and file driver, I felt it best we rename the TTL key, as ...:created could easily be a user land key that we accidentally delete.

We now namespace the key illuminate:cache:flexible:created:{key}.

I suppose this is a breaking change, but I think removing the possibility of accidentally deleting user-land keys is more breaking than having the cache value refreshed early for this one off change.

Standardise key names

While I was in here making this change I also updated the defer key and the cache lock name to match the illuminate:cache:flexible:* pattern.

fixes #52847

timacdonald · 2024-09-24T03:07:28Z

tests/Cache/CacheFileStoreTest.php

-        $files = $this->mockFilesystem();
-        $hash = sha1('foobar');
-        $cache_dir = substr($hash, 0, 2).'/'.substr($hash, 2, 2);
+        $files = new Filesystem;
        $store = new FileStore($files, __DIR__);
        $store->put('foobar', 'Hello Baby', 10);
-        $files->expects($this->once())->method('exists')->with($this->equalTo(__DIR__.'/'.$cache_dir.'/'.$hash))->willReturn(true);
-        $files->expects($this->once())->method('delete')->with($this->equalTo(__DIR__.'/'.$cache_dir.'/'.$hash));
+
+        $this->assertFileExists($store->path('foobar'));
+
        $store->forget('foobar');
+
+        $this->assertFileDoesNotExist($store->path('foobar'));


I'm modified this test so it really does check that the file is cleaned up. There was an issue with the mock because of the new key being deleted. Mockery just burnt my time trying to make it work.

This test is 1000% better without the mocks anyway.

Tklaversma · 2024-09-25T09:15:19Z

@timacdonald Maybe it's just me, but I think this is still not working as expected when using Redis (I didn't test other drivers). When I do Cache::forget('foo'), it still leaves illuminate:cache:flexible:created:foo in the database.

Although after re-creating the same cache item, it replaces the existing illuminate:cache:flexible:created:foo (which is a good thing), I think it's still best to remove it when Cache::forget() is being called. I can imagine a scenario where Cache::flexible() is used extensively with very high TTLs and the database is still filled with unnecessary data after forget is called.

timacdonald · 2024-09-25T22:43:48Z

@Tklaversma, we have decided to leave the flexible keys to naturally expire for the cache drivers that respect a TTL.

As you mentioned, they would be replaced correctly when called again, so functionally there is no issue, and otherwise they would naturally expire over time.

This is similar to what happens with cache tags. It is not an ideal solution but the direction we are heading for now.

We manually clean out the database and file drivers because they won't naturally expire over time.

Tklaversma · 2024-09-26T06:18:49Z

@timacdonald I understand, but can't there be a middle ground for Cache::flexible() (and maybe Cache::tags() as well), where we introduce an extra parameter on Cache::forget() for example, that will also remove these illuminate:cache:flexible:created:... keys? Heck, maybe even introduce an additional function called Cache::forceForget().

In my particular case I don't want these remnants, because I have many cache items with high TTL's. I do have workarounds, but in the light of "keeping your database clean", having such an option to be able to delete those keys as well, would be great addition. And I don't think I'm the only one in this matter.

I'm curious to your thought about this. Let me know 👍🏻

timacdonald · 2024-09-26T06:26:57Z

I don't think we have plans for this but will certainly keep an eye feedback around all this.

Tklaversma · 2024-09-26T06:55:02Z

@timacdonald Ok, readying a PR. I found out that you already did delete those illuminate:cache:flexible:created:... keys, but only for the database and file stores. I'll submit my PR that will align the other stores with this functionality as well.

See laravel#52891 Signed-off-by: Rood Fruit B.V <development@roodfruit.studio>

Tklaversma · 2024-09-26T07:21:59Z

See #52936.

mra-dev · 2024-10-02T17:24:35Z

This PR is actually breaking my CLI App when using Cache with file-driver

In this line:
#diff-4a1de3db154fb3881008aacba13f82699df89754a68b9d913077177d36821f63R251
Note: I checked it & by deleting that line. everything went back to normal.

There's actually no file named that. but it tries to delete it and the holy Exception comes up.

Mustn't there be an exists() check and then delete it ???

Exception (Full):

[2024-10-01 xx:xx:xx] local.ERROR: unlink(/home/storage/framework/cache/data/2f/a0/2fa0e....95e779): No such file or directory {"exception":"[object] (Exception: unlink(/home/storage/framework/cache/data/2f/a0/2fa0...e779): No such file or directory at /home/vendor/laravel/framework/src/Illuminate/Filesystem/Filesystem.php:308)

timacdonald · 2024-10-02T22:28:38Z

@mra-dev, do you have a custom implementation of the filesystem in your application? I can't seem to replicate that behaviour locally.

I tried with the following in a fresh Laravel application.

Cache::driver('file')->forget('key');

Cache::driver('file')->put('key', 'value', ttl: 5);
Cache::driver('file')->forget('key');

I've opened a PR with a passing test for this behaviour: #53018

Could you provide a way for me to replicate the issue to ensure we have an appropriate test and fix in place?

mra-dev · 2024-10-02T23:13:37Z

@timacdonald I'm using pure Laravel (nothing custom)
Situation: I'm processing incoming "messages" in Laravel's Queue.
Each has to be checked in a check-wall which involves "rememberForever" & "forget" (no flexible, just simple cache access)
In versions prior to 11.24, everything's were fine.
After upgrading, i got the Exception i mentioned above.

The problem is that i'm in CLI. Inside web side, everything's fine.

mra-dev · 2024-10-02T23:24:46Z

Oh, and by the way, there are cases which may there be "race condition" or "concurrency" or in "Fibers".
Means: with a 20~80 ms between each cache access or forget (which was not a problem before)

timacdonald · 2024-10-02T23:53:45Z

Thanks for the additional info.

Can you see why my test in my PR would be passing without adding any changes?

I've tried this on both the web and the CLI and everything is working as expected. Not seeing any logs or exceptions.

// console.php

Artisan::command('dev', function () {
    Cache::driver('file')->forget('key');

    Cache::driver('file')->put('key', 'value', ttl: 5);
    Cache::driver('file')->forget('key');

    echo 'ok';
});

// web.php

Route::get('/', function () {
    Cache::driver('file')->forget('key');

    Cache::driver('file')->put('key', 'value', ttl: 5);
    Cache::driver('file')->forget('key');

    return 'ok';
});

Wondering if there is some php.ini configuration needed to make this throw an exception?

We will get it fixed for you. Just want to make sure we have a test in place for future developers modifying the code.

mra-dev · 2024-10-03T00:21:51Z

One potential thing could be write-protection that Laravel does on the file.
And when another process with another user or group, tries to do operations on it; it throws the error.

One another weak possibility could be the Queue's long running process not knowing PHP's "clearstatcache" !!!?

But, 100% sure that this line corrupts the APP.

Maybe it isn't created at all in normal cache puts ??! (i mean the flexible cache file)

mra-dev · 2024-10-03T00:26:06Z

And, yes. sorry for double message bug i have.
I checked it in normal circumstances (so raw) in a queue job. (it works like your tests)
This thing happens when using "rememberForever" & "forget" together in long running processes (Queues) + using Events in conjunction with it.

mra-dev · 2024-10-03T00:57:28Z

They key thing i remembered to point out:
Even cache folders permissions were right: 755
and then i saw a 2 prefixed for "/storage" folder; like: 2755

mra-dev · 2024-10-03T09:18:13Z

I tried to replicate this in a similar situation.
But it seems this happens in certain scenarios. (Fibers in Foreach & vice versa)
Can't we just put and ENV key to indicate if it should delete "flexible" keys too and set false by default. (It's not a good idea to just downgrade to not having this weird BUG)

mra-dev · 2024-10-03T22:40:02Z

@timacdonald

timacdonald · 2024-10-03T23:31:09Z

Can you please confirm in your application that the changes here fixes your problem with certainty?

I have no way to confirm my fix even works so I'm shooting in the dark.

https://github.com/laravel/framework/pull/53018/files#diff-4a1de3db154fb3881008aacba13f82699df89754a68b9d913077177d36821f63

mra-dev · 2024-10-04T00:49:31Z

@timacdonald I'm gonna check it right away. and shoot a video for it.

mra-dev · 2024-10-04T01:04:16Z

✅ I'm confirming that this is the solution.
😍 Everything's working as before and expected. (no more unlink exception)
thanks @timacdonald

P.S: I tested it by manually changing code in /vendor/laravel/framework/src/Illuminate/Cache/FileStore.php

// From this: 

public function forget($key)
{
    if ($this->files->exists($file = $this->path($key))) {
        $this->files->delete($this->path("illuminate:cache:flexible:created:{$key}"));

        return $this->files->delete($file);
    }

    return false;
}

// To this:

public function forget($key)
{
    if ($this->files->exists($file = $this->path("illuminate:cache:flexible:created:{$key}"))) {
        $this->files->delete($file);
    }
    
    if ($this->files->exists($file = $this->path($key))) {
        return $this->files->delete($file);
    }

    return false;
}

mra-dev · 2024-10-04T01:07:34Z

BTW; @timacdonald Do you think using declare(strict_types=1); would cause the errors to pop, even when using @unlink() ?!

timacdonald · 2024-10-04T01:41:55Z

Are you modifying the files in vendor/laravel/framework to apply declare(strict_types=1);?

That flag only applies only to the files it is within.

mra-dev · 2024-10-05T04:11:56Z

Are you modifying the files in vendor/laravel/framework to apply declare(strict_types=1);?

@timacdonald
No No. I just wanted to fill all the blank holes with information.
I just applied the part you specifically said. (one comment above)
And boom; Everything went back to normal.

mra-dev · 2024-10-06T21:58:10Z

@timacdonald Should I create an issue for this, or has it already been fixed?

timacdonald · 2024-10-06T22:38:08Z

I'll fix this in #53018. Will have it opened today.

Handle stray flexible TTL keys

681854f

timacdonald marked this pull request as draft September 23, 2024 22:59

timacdonald added 7 commits September 24, 2024 09:17

Ensure forgetting keys cleans up flexible TTL keys

8426698

Remove redundant has check

415e4b4

Ensure lazy database drivers clean up ttl keys

0133660

Rename ttl key

e7452dd

Fix tests

29d4537

Fix tests

8078060

fix test

ad7ecec

timacdonald commented Sep 24, 2024

View reviewed changes

timacdonald changed the title ~~[11.x] Handle stray flexible TTL keys~~ [11.x] Cache::flexible improvements Sep 24, 2024

timacdonald added 2 commits September 24, 2024 14:20

Rename cache lock key

e917c4c

Rename defer key

bfd90aa

timacdonald force-pushed the cache-flexible branch from be64ca2 to bfd90aa Compare September 24, 2024 04:20

timacdonald marked this pull request as ready for review September 24, 2024 04:25

timacdonald mentioned this pull request Sep 24, 2024

[11.x] Make Cache::forget compatible with Cache::flexible #52859

Closed

taylorotwell merged commit b05e6f8 into laravel:11.x Sep 24, 2024
30 checks passed

timacdonald deleted the cache-flexible branch September 24, 2024 22:24

Tklaversma pushed a commit to Tklaversma/framework that referenced this pull request Sep 26, 2024

[11.x] Align all Cache stores to work the same way regarding forget()

3dc21b4

See laravel#52891 Signed-off-by: Rood Fruit B.V <development@roodfruit.studio>

Tklaversma mentioned this pull request Sep 26, 2024

[11.x] Fixed issue where Cache::forget() works differently if keys created with Cache::flexible when using Redis, DynamoDB, Memcached or Apc #52936

Closed

timacdonald mentioned this pull request Oct 2, 2024

[11.x] Test for forgetting non-flexible keys for file driver #53018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[11.x] `Cache::flexible` improvements #52891

[11.x] `Cache::flexible` improvements #52891

timacdonald commented Sep 23, 2024 •

edited

Loading

timacdonald Sep 24, 2024

Tklaversma commented Sep 25, 2024 •

edited

Loading

timacdonald commented Sep 25, 2024

Tklaversma commented Sep 26, 2024 •

edited

Loading

timacdonald commented Sep 26, 2024

Tklaversma commented Sep 26, 2024

Tklaversma commented Sep 26, 2024

mra-dev commented Oct 2, 2024

timacdonald commented Oct 2, 2024 •

edited

Loading

mra-dev commented Oct 2, 2024

mra-dev commented Oct 2, 2024 •

edited

Loading

timacdonald commented Oct 2, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

timacdonald commented Oct 3, 2024 •

edited

Loading

mra-dev commented Oct 4, 2024

mra-dev commented Oct 4, 2024 •

edited

Loading

mra-dev commented Oct 4, 2024

timacdonald commented Oct 4, 2024

mra-dev commented Oct 5, 2024 •

edited

Loading

mra-dev commented Oct 6, 2024

timacdonald commented Oct 6, 2024

[11.x] Cache::flexible improvements #52891

[11.x] Cache::flexible improvements #52891

Conversation

timacdonald commented Sep 23, 2024 • edited Loading

Bugfix

Cleanup flexible TTL keys

TTL key rename

Standardise key names

timacdonald Sep 24, 2024

Choose a reason for hiding this comment

Tklaversma commented Sep 25, 2024 • edited Loading

timacdonald commented Sep 25, 2024

Tklaversma commented Sep 26, 2024 • edited Loading

timacdonald commented Sep 26, 2024

Tklaversma commented Sep 26, 2024

Tklaversma commented Sep 26, 2024

mra-dev commented Oct 2, 2024

timacdonald commented Oct 2, 2024 • edited Loading

mra-dev commented Oct 2, 2024

mra-dev commented Oct 2, 2024 • edited Loading

timacdonald commented Oct 2, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

mra-dev commented Oct 3, 2024

timacdonald commented Oct 3, 2024 • edited Loading

mra-dev commented Oct 4, 2024

mra-dev commented Oct 4, 2024 • edited Loading

mra-dev commented Oct 4, 2024

timacdonald commented Oct 4, 2024

mra-dev commented Oct 5, 2024 • edited Loading

mra-dev commented Oct 6, 2024

timacdonald commented Oct 6, 2024

[11.x] `Cache::flexible` improvements #52891

[11.x] `Cache::flexible` improvements #52891

timacdonald commented Sep 23, 2024 •

edited

Loading

Tklaversma commented Sep 25, 2024 •

edited

Loading

Tklaversma commented Sep 26, 2024 •

edited

Loading

timacdonald commented Oct 2, 2024 •

edited

Loading

mra-dev commented Oct 2, 2024 •

edited

Loading

timacdonald commented Oct 3, 2024 •

edited

Loading

mra-dev commented Oct 4, 2024 •

edited

Loading

mra-dev commented Oct 5, 2024 •

edited

Loading