Skip to content
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

[8.x] TaggedCache flush should also remove tags from cache #39299

Merged
merged 4 commits into from
Oct 22, 2021
Merged

[8.x] TaggedCache flush should also remove tags from cache #39299

merged 4 commits into from
Oct 22, 2021

Conversation

Plytas
Copy link
Contributor

@Plytas Plytas commented Oct 21, 2021

At the moment flushing a tagged cache removes standard keys, forever keys and the key/value pair that was actually cached, but it leaves the tag itself as it only resets it.

In our project we use renoki-co/laravel-eloquent-query-cache and with heavy cache tag usage it leads to junk keys accumulating in our redis instance to a point where we now have close to 5 million keys that will never expire:
image

IMO, cache tags (as well as standard keys) should have an expiration time as well, but I'm able to solve that in userland by setting expiration time to them manually after caching the value. But it doesn't solve the issue when you flush a tag.

This PR solves flushing issue by deleting the tags from cache instead of resetting them. I've added a small test that you can run without TaggedCache changes and see that the tag key actually persists after flush.

@GrahamCampbell GrahamCampbell changed the title TaggedCache flush should also remove tags from cache [8.x] TaggedCache flush should also remove tags from cache Oct 21, 2021
@taylorotwell
Copy link
Member

taylorotwell commented Oct 21, 2021

Are you using Redis? If so, can you just implement this change on the RedisTaggedCache (you can still add a method to TagSet)? That way we are making a smaller change.

@Plytas
Copy link
Contributor Author

Plytas commented Oct 21, 2021

I've reverted TaggedCache and implemented the change on RedisTaggedCache. I had to remove my newly added test because it was using ArrayStore, thus not being impacted by this change. Not sure if possible to write a test without mocking that would prove that tags are being removed.

Link to previous test for reference

@taylorotwell taylorotwell merged commit 0140cfe into laravel:8.x Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants