-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[10.x] Use Lua script in Redis to flush cache tags atomically #48078
Conversation
e7c6ccf
to
174837a
Compare
Can you describe the cause of the issue and what steps you took to fix it? Please mark as ready for review when you have done so. |
When we call
Since these three operations are not atomic, a problem may arise: if new keys are added to the cache tag after step 2 but before step 3, these keys become orphaned and unmanageable by the tag key. Consequently, in the next call to To resolve this, I modified the logic:
|
@zengbo can't new keys be added after you gather them but before you invoke the Lua script here? |
@taylorotwell New keys can be added after they are gathered. The Lua script only deletes the previously gathered keys, not the tag key. Therefore, the new keys remain in the cache tag key and can be retrieved during the next flush call.. |
This means that the deletion of the key and the removal of the key's reference in the tag key are atomic operations. However, I do not delete the tag key. |
So how does the tag key ever get deleted? Can the removal of all tag entires and the key be combined into a single Lua script? |
|
redis.call('DEL', key) | ||
for j, arg in ipairs(ARGV) do | ||
local zkey = string.gsub(key, prefix, "") | ||
redis.call('ZREM', arg, zkey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this line violates:
Important: to ensure the correct execution of scripts, both in standalone and clustered deployments, all names of keys that a script accesses must be explicitly provided as input key arguments. The script should only access keys whose names are given as input arguments. Scripts should never access keys with programmatically-generated names or based on the contents of data structures stored in the database.
Because the key name is taken from ARGV
instead of KEYS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The KEYS
and ARGV
are not the same.
KEYS
represent cache entries within the cache tag, while ARGV
stands for cache tag keys.
I utilize key
for KEYS
and zkey
for ARGV
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arg
parameter is the key that ZREM
operates on and it is taken from ARGV
.
…istency and preventing partial updates during flushing operations
174837a
to
bd95dd2
Compare
@taylorotwell
Please test these, thanks. |
I personally don't have the knowledge to trust merging this sadly. I would prefer to either de-document cache tags or recommend their use with Memcached only. Cache tags and Redis have been a maintenance and complexity nightmare. |
@taylorotwell please reconsider fixing this concurrency issue since our application suffers a lot from inconsistent cache around resources. Since tagged cache does not guarantee cache to be flushed successfully each time, we are forced to clear entire cache store. It's not an option for high load applications as it stresses out the database too much. Opened a discussion #49159 |
@AidasK are you able to test the content of this PR to see if it fixes your issue? |
Hi @taylorotwell, thanks for taking a look at this issue again. Personally I have not tried this solution as in production we are using 9.x laravel cache class. Our use case allows us to have some memory leaks. You won't like it, but I suggest to use https://github.com/symfony/cache instead of developing your own solution. Under the hood they are already using lua scripts to make things work. Just look it up https://github.com/symfony/cache/blob/7.0/Adapter/RedisTagAwareAdapter.php Since it's too hard to maintain proper cache adapters, it's better to use symfony ones which are well developed and focus on futures which truly matters. Of course it is easier said than done, but in the long term perspective it should be worth the effort. For laravel community it won't have any breaking changes besides of clearing all the database once if storage format is different, haven't checked. |
symfony/cache tags, recahe-locking, optimistic-recache are awesome built-in & default features. |
Issue: #47903. Reproduce as follows:
composer create-project laravel/laravel example-app
php artisan make:controller CacheController
routes/api.php
The
App\Http\Controllers\CacheController
:The
routes/api.php
:To test on Mac iTerm:
Output "Inconsistent result found" indicates issue reproduction.
cache_tag_benchmark.tar.gz