laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.13k stars 10.87k forks source link

Cache: Inconsistent behaviour regarding locks when clearing cache #52344

Closed pxlrbt closed 1 month ago

pxlrbt commented 1 month ago

Laravel Version

11.19.0

PHP Version

8.3

Database Driver & Version

Redis

Description

There is an inconsistency when clearing the cache between the Redis store and the file store. The file store also clears locks, while the redis store doesn't. This affects unique queued jobs. While you can reset the job locks through artisan cache:clear when using the file store, this doesn't work with the redis store, leaving you confused why no jobs are pushed to the queue.

Steps To Reproduce

When CACHE_STORE is set to redis Laravel behaves like this:

$lock = cache()->lock('test');
$lock->get(); // true
$lock->get(); // false

cache()->flush(); // or artisan cache:clear

$lock->get(); // false

When CACHE_STORE is set to file Laravel behaves like this:

$lock = cache()->lock('test');
$lock->get(); // true
$lock->get(); // false

cache()->flush(); // or artisan cache:clear

$lock->get(); // true
github-actions[bot] commented 1 month ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

rust17 commented 1 month ago

Hi everyone, Could you review this PR and check if the way to reproduce the issue is acceptable? Thanks.

Katalam commented 1 month ago

That is kinda by design. We have two configs for the redis cache

We have this https://github.com/laravel/framework/blob/cfe5f84ba23b5a47f1232a3f2bc1ea8cd23ca572/config/cache.php#L74-L78 That will determine which connection is used for the cache. Be advised that there is a difference in the default values of these two entries

And then we have this https://github.com/laravel/framework/blob/cfe5f84ba23b5a47f1232a3f2bc1ea8cd23ca572/config/database.php#L153-L169

The cache:clear will use FLUSHDB (https://redis.io/docs/latest/commands/flushdb/) as shown here https://github.com/laravel/framework/blob/cfe5f84ba23b5a47f1232a3f2bc1ea8cd23ca572/src/Illuminate/Cache/RedisStore.php#L248 This will only flush the current database, because we don't want to interfere with other databases inside redis. You can change that behaviour by either change the database for locks in redis or change the behaviour of the cache clear command in your own application and replace the RedisConnection with your own class here https://github.com/laravel/framework/blob/cfe5f84ba23b5a47f1232a3f2bc1ea8cd23ca572/src/Illuminate/Redis/Connections/PhpRedisConnection.php#L496-L505. be advised, there are other things that will impact that change.

pxlrbt commented 1 month ago

Yes, I found that solutions with two Redis connections after hours of debugging. I'm okay with it being by design, but what threw me off was the different behaviour between drivers.

I just checked the config/cache.php and there are separate connections for DB and Redis, but not for the others. At least for the File Driver it's just a config change. Never used the others, so not sure whether it's even possible to have the same behaviour for all the drivers

rust17 commented 1 month ago

It seems odd, in order to keep consistency, the best solution may be to set REDIS_CACHE_DB to 0. From multiple testing code executions, I've noticed that when REDIS_CACHE_DB is set to 1, the actual redis database that executing the commands is as bellow:

Cache::store('redis')->lock('foo')->forceRelease(); // actual database is 0

$lock = Cache::store('redis')->lock('foo'); // actual database is 0
$this->assertTrue($lock->get()); // actual database is 0
$this->assertFalse($lock->get()); // actual database is 0

Cache::store('redis')->flush(); // actual database is 1

$this->assertTrue($lock->get()); // actual database is 0

When REDIS_CACHE_DB is set to 0, the actual Redis database that executing the commands is as follows:

Cache::store('redis')->lock('foo')->forceRelease(); // actual database is 0

$lock = Cache::store('redis')->lock('foo'); // actual database is 0
$this->assertTrue($lock->get()); // actual database is 0
$this->assertFalse($lock->get()); // actual database is 0

Cache::store('redis')->flush(); // actual database is 0

$this->assertTrue($lock->get()); // actual database is 0

The inconsistency arises because only the redis instance executing the flush command is different, preventing the lock from being released.

Katalam commented 1 month ago

But that is indeed how it is supposed to work. In order to separate the locks from the actual values be use different databases, these values are saved inside database 1. The cache clear command is intended to clear the values from database 1. In order to flush all locks we need to have a separate logic that will flush database 0 for us. You can do: Redis::command("flushall") do achieve that type of behavior. We want to prevent that an actual value key is colliding with a lock key. And to do that, we use different databases. The different behavior in different drivers comes from the actual implementation and it is kinda expected that different drivers work slightly differently. I think we can try to add an extra layer for the Redis Connection to also flush the lock database while cache:clear

Katalam commented 1 month ago

While I wrote the answer, I though about it again and have to say that dropping the locks is kinda an expected behaviour, because we also drop the data at the moment

rust17 commented 1 month ago

Oh, I get it now 😯, I initially thought it was a bug.

crynobone commented 1 month ago

Thabks @Katalam

pxlrbt commented 1 month ago

@crynobone Not sure why this was closed. It's not really solved.

I think we should find a consensus on whether it's expected to clear locks when clearing the cache or not. Since it might break existing workflows, maybe the best idea is to introduce a new command and mention it in the docs?

crynobone commented 1 month ago

I think we should find a consensus on whether it's expected to clear locks when clearing the cache or not.

issues should focus on confirmed bug. Feel free to continue the discussion or submit a PR to the documentation etc.