[CACHE] Never hits cache with TagAwareAdapter #54097

AtCliffUnderline · 2024-02-28T09:30:24Z

Symfony version(s) affected

6.4.4

Description

A bit similar to #53825

As in the code below, there is fetching several keys with mget. Keys are already exists in database. First fetch of keys with mget retrieves them all and create good $bufferedItems.

But after that getTagVersion gets wrong tag (screenshot) and unseting them all (TagAwareAdapter.php:191). This bad tag have been stored by this adapter.

And after unset them all it creates items with hit: false.

How to reproduce

$cache = new TagAwareAdapter(
    new ChainAdapter(
        [
            new ArrayAdapter(300, false, 300),
            new RedisTagAwareAdapter(
                $this->redisConnectionPoolService->getClientByPoolName('cache'),
                'redis.tag.aware.symfony1',
                86400,
                new DefaultMarshaller(false),
            ),
        ],
        300,
    ),
);

$keys = ['key1', 'key2', 'key3'];
$tag = 'some_tag';
$notHitItems = [];
$items = $cache->getItems($keys);
foreach ($items as $item) {
    if (! $item->isHit() || ! is_array($item->get())) {
        $notHitItems[] = $item;
    }
}

foreach ($notHitItems as $item) {
    $item->set(['some_value']);
    $item->expiresAfter(3 * 24 * 60 * 60);
    $item->tag($tag);

    $cache->saveDeferred($item);
}

if (! empty($notHitItems)) {
    $cache->commit();
}

Possible Solution

No response

Additional Context

Chain adapter is not really needed here. But this is my production setup.
Seems kinda critical imho

The text was updated successfully, but these errors were encountered:

AtCliffUnderline · 2024-02-28T09:32:00Z

@nicolas-grekas Hello! Very sorry for bothering you. Could you have a look at this please? Could be cause of #53846

nicolas-grekas · 2024-02-28T17:23:26Z

Can you try to figure out what happens?

AtCliffUnderline · 2024-02-28T18:04:27Z

I'm trying, but tbh i'm a little bit confused with TagAwareAdapter code :)
Right now I understood that even invalidateTags is broken, it removes only tag itself from Redis, without removing KV pairs that belongs to it.

stof · 2024-02-28T18:07:35Z

I don't think invalidateTags is broken. See the phpdoc of the class which provides some explanation about its architecture.

AtCliffUnderline · 2024-02-28T18:14:21Z

Well, i compared with my current 5.4.34 and I agree that behaviour of invalidateTags is the same. But still something going wrong

AtCliffUnderline · 2024-02-29T08:45:59Z

Index: vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php b/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php
--- a/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php	
+++ b/vendor/symfony/cache/Adapter/AbstractTagAwareAdapter.php	(date 1709196146551)
@@ -56,7 +56,7 @@
                 $item->isHit = $isHit;
                 // Extract value, tags and meta data from the cache value
                 $item->value = $value['value'];
-                $item->metadata[CacheItem::METADATA_TAGS] = isset($value['tags']) ? array_combine($value['tags'], $value['tags']) : [];
+                $item->metadata[CacheItem::METADATA_TAGS] = isset($value['tags']) ? $value['tags'] : [];
                 if (isset($value['meta'])) {
                     // For compactness these values are packed, & expiry is offset to reduce size
                     $v = unpack('Ve/Nc', $value['meta']);
@@ -107,7 +107,6 @@
                     foreach (array_diff_key($oldTags, $value['tags']) as $removedTag) {
                         $value['tag-operations']['remove'][] = $getId($tagPrefix.$removedTag);
                     }
-                    $value['tags'] = array_keys($value['tags']);
 
                     $byLifetime[$ttl][$getId($key)] = $value;
                     $item->metadata = $item->newMetadata;

wdyt? Saving and retrieving tags as associative array . Comparing is correct after this because we not losing these 6 random_bytes which are version.

AtCliffUnderline · 2024-03-01T12:17:20Z

Can I up this? Still thinks this is pretty critical

tecbird · 2024-03-01T13:20:35Z

+1

Fahl-Design · 2024-03-01T13:21:18Z

+1

nicolas-grekas · 2024-03-01T15:38:04Z

I would need more insights to be of any help.

I tried your reproducer @AtCliffUnderline but I miss an observer to tell which behavior is wrong. If I dump($notHitItems), I don't see any difference between v6.4.3 and v6.4.4.

Note that your wiring is really strange: a RedisTagAwareAdapter inside a TagAwareAdapter?

Fahl-Design · 2024-03-01T18:47:19Z

@nicolas-grekas here is an other example, maybe even simpler to reproduce

I have the same issue with this config based on the docs of the chain adapter with redis tag aware i guess (should not matter at all, looks like a cache tag problem anyways)

services:
    redis.provider.one:
        class: Redis
        factory: [ 'Symfony\Component\Cache\Adapter\RedisTagAwareAdapter', 'createConnection' ]
        arguments:
            - '%env(CACHE_REDIS_DSN_DEFAULT)%/6'

    redis.provider.two:
        class: Redis
        factory: [ 'Symfony\Component\Cache\Adapter\RedisTagAwareAdapter', 'createConnection' ]
        arguments:
            - '%env(WEBSHOP_REDIS_DSN_FLATDATA)%'
            - { retry_interval: 1, timeout: 10 }

    'cache.adapter.redis.one':
        parent: 'cache.adapter.redis_tag_aware'
        arguments:
            $redis: '@redis.provider.one'
            $namespace: 'fancy_namespace'
      
    'cache.adapter.redis.two':
        parent: 'cache.adapter.redis_tag_aware'
        arguments:
            $redis: '@redis.provider.two'
            $namespace: 'other_fancy_namespace'

framework:
    cache:
        pools:
            fancy_cache_pool:
                default_lifetime: 31536000  # One year
                adapters:
                    - cache.adapter.redis.one
                    - cache.adapter.redis.two
                tags: true

If you run this twice, it is build twice (or more)

        $myCachedData = $this->cachePool->get(
            key: 'fancy_cache_key',
            callback: function (ItemInterface $item): string {
                dump($item->getKey() . ' was build ');
                $item->tag(['tag1', 'tag2']);

                return 'my cached value';
            });

you will see the dump "was build" every time, meaning no cache at all

clear your cache
now remove the tag
repeat and see expected 1 time "build"

        $myCachedData = $this->cachePool->get(
            key: 'fancy_cache_key_2',
            callback: function (ItemInterface $item): string {
                dump($item->getKey() . ' was build ');
                // $item->tag(['tag1', 'tag2']); <-- no tags

                return 'my cached value';
            });

cache works as expected

possible cause:
the use of "TAGS_PREFIX" is inconsistent or just not named correct, sometimes it is used as prefix, sometime suffix. Redis gets data filled with "tags" as suffix and never has data with "tags" as prefix (getTagVersions can not work, because no data in redis with tags as prefix)

v6.4

I hope this may help to fix this

nicolas-grekas · 2024-03-01T18:52:06Z

Can you please put the reproducer in a small app I could clone ?

Fahl-Design · 2024-03-01T18:57:18Z

Of cause, give me a minute or 10 ;)

Fahl-Design · 2024-03-01T19:21:47Z

@nicolas-grekas here we go https://github.com/Fahl-Design/cache-issue-reproducer

*edit: inside redis: "tags" is always set as a key suffix NOT prefix

https://github.com/Fahl-Design/cache-issue-reproducer edit: updated to make it more visible and symfony v7 as well can be reproduces in v6.4 and v7.0

nicolas-grekas · 2024-03-04T20:31:30Z

Thanks for the reproducer app @Fahl-Design

Now I'm confused. Is this issue new? For some reason, I had the understanding that this was introduced in 6.4.4, but it's not, right? The reproducer behaves the same with 7.0.0 or 7.0.4, or I missed something.

Could any of you tell me why you're putting a RedisTagAwareAdapter in a chain that's then given to TagAwareAdapter? This doesn't make any sense to me and that'd the other thing I'd need to clarify.

Fahl-Design · 2024-03-04T21:21:04Z

You'r welcome, I found this issue mainly by accident ;) I use 6.4 in an app and this was my first time using a tagged cache pool.
There is a 6.4 version in the reproducer version as well, after that i updated to 7.0 and still reproduce it.

Could any of you tell me why you're putting a RedisTagAwareAdapter in a chain that's then given to TagAwareAdapter? This doesn't make any sense to me and that'd the other thing I'd need to clarify.

maybe the yaml don't make it clear ;)

...
    redis.provider.two:
        class: Redis
        factory: [ 'Symfony\Component\Cache\Adapter\RedisTagAwareAdapter', 'createConnection' ]
...

redis.provider.two is just the \Redis instance https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Cache/Traits/RedisTrait.php#L87

I guess that's from my understanding of the docs
https://symfony.com/doc/current/components/cache/adapters/redis_adapter.html#working-with-tags
https://symfony.com/doc/current/cache.html#creating-a-cache-chain

isn't that how the pools should be set up to work?^^
I mean I don't get any autowiring issues and it doesn't look to bad

I attempt is to setup a cache pool with tags in a chain to fill a second redis instance for an other app.
It works fine and as expected as long as i don't use the tags

when you run bin/console app:cache-test --add-tags you can see the pool will not give you the cached value, and calls the get callback every time.

I had a look at the corresponding code but man, that's not easy to follow (and when tags are involved)
any ideas?

nicolas-grekas · 2024-03-05T10:03:12Z

OK, I think I get what we want to achieve. There are two aspects here:

Why does it not work as is?

My guess is that there is some bad interaction between RedisTagAwareAdapter and TagAwareAdapter. We could try to understand why and figure out a way around, but the wiring doesn't make sense in the first place to me. This leads to the second point;

What are you trying to achieve?

My guess is you're trying to have a cache that stores things locally (ArrayAdapter) on top of a remote cache storage (Redis), and you want that chain to be able to manage tags. You can achieve this immediately by not using RedisTagAwareAdapter in the chain. Just use a regular RedisAdapter.

This means the storage layout is going to use versioned tags instead of tag relationships (what RedisTagAwareAdapter manages). Tag relationships require more storage but save a few roundtrips. If you don't have an objective reason to use them, versioned tags are really good anyway.

Then remains the question of wiring an ArrayAdapter on top of a RedisTagAwareAdapter. I think we would need a TagAwareChainAdapter and TagAwareArrayAdapter for this. Or another better idea :)

Fahl-Design · 2024-03-05T10:13:59Z

@nicolas-grekas

I will open a new issue with a new title.
I'm not the author of this issue, so I cant edit this one.

In my case, and the reproducer code, there are no array adapters

nicolas-grekas · 2024-03-05T10:41:34Z

Yes, the array adapter is not the issue so it doesn't matter.

AtCliffUnderline · 2024-03-21T08:14:08Z

I apologize for not responding in this issue. Indeed, it makes no sense to use RedisTagAwareAdapter inside TagAwareAdapter, changing it to RedisAdapter made the problem go away, thanks

Sorry for going off-topic though, I really wanted to clarify, right now I'm in the process of upgrading from version 5.4 to version 6.4, and I can't understand at all, is it so necessary to make a second request to the pool (in my case to Redis) https://github.com/symfony/cache/blob/6.4/Adapter/TagAwareAdapter.php#L180 ?

This has added 4-5 ms of response time in my project as I am actively using cache. Trying to find what to do, is the only option for me right now is to use 6.1.11?

nicolas-grekas · 2024-03-21T08:27:35Z

There are two storage strategies with redis:

TagAwareAdapter + RedisAdapter, which requires this second request but has better scalability (needed only if you hit some Redis limits with the second approach)
RedisTagAwareAdapter, which requires only one roundtrip (at the expense of more storage requirement, which can be manageable depending on your needs)

Chose one, just don't mix both :)

Fahl-Design · 2024-03-21T09:16:25Z

@nicolas-grekas thanks for clarification, sounds good to me
would be a nice addition to prevent RedisTagAwareAdapter in a TagAwareAdapter Chain since is simply dose not work at all

carsonbot · 2024-09-22T13:03:42Z

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

carsonbot · 2024-10-06T13:05:57Z

Friendly ping? Should this still be open? I will close if I don't hear anything.

AtCliffUnderline added the Bug label Feb 28, 2024

carsonbot added Status: Needs Review Cache labels Feb 28, 2024

Fahl-Design mentioned this issue Mar 5, 2024

[Cache] Stored tag key change invalidate all cache #51655

Closed

Fahl-Design mentioned this issue Mar 5, 2024

[CACHE] TagAwareAdapter with ChainAdapter as Cache Pool and RedisTagAwareAdapter as pool adapters dose not work with tagged CacheItems #54160

Closed

nicolas-grekas mentioned this issue Apr 12, 2024

Cache pool keys colide when Chain Adapter pool is used as a parent pool #49607

Closed

carsonbot added the Stalled label Sep 22, 2024

AtCliffUnderline closed this as completed Oct 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CACHE] Never hits cache with TagAwareAdapter #54097

[CACHE] Never hits cache with TagAwareAdapter #54097

[CACHE] Never hits cache with TagAwareAdapter #54097

[CACHE] Never hits cache with TagAwareAdapter #54097

Comments

Symfony version(s) affected

Description

How to reproduce

Possible Solution

Additional Context

Why does it not work as is?

What are you trying to achieve?