PHPSocialNetwork / phpfastcache

A high-performance backend cache system. It is intended for use in speeding up dynamic web applications by alleviating database load. Well implemented, it can drops the database load to almost nothing, yielding faster page load times for users, better resource utilization. It is simple yet powerful.
https://www.phpfastcache.com
MIT License
2.36k stars 452 forks source link

getItemsByTag() - empty after one item has expired #893

Closed MaxChri closed 1 year ago

MaxChri commented 1 year ago

What type of issue is this?

Incorrect/unexpected/unexplainable behavior

Operating system + version

Debian 11

PHP version

8.1

Connector/Database version (if applicable)

Memcached, Files

Phpfastcache version

9.1.2 ✅

Describe the issue you're facing

I create multiple keys with different expiration values using expiresAfter(). I'm using the addTags() function to search for the keys if necessary (e.g to manage them in a CMS). When a first item key expires, all other keys with the same tag name are immediately gone and are not callable anymore with getItemsByTag().

So basically if you have a 5 seconds key and another 10 seconds key (both initialized at the same time) with the same tag name, both items will be returned by getItemsByTag(). But if the 5 seconds key has expired, the 10 seconds key is also gone when calling getItemsByTag(). The 10 seconds key is still physically there but cannot be found by the previously declared tag anymore. So a 10 seconds timer would be gone within 5 seconds in this scenario. If the 5 seconds timer would not exist, the 10 seconds timer would of course counting down until 0.

Expected behavior

All keys with the same tag name should be still callable, even if previous keys with the same tag has been expired.

Code sample (optional)

No response

Suggestion to fix the issue (optional)

No response

References (optional)

No response

Do you have anything more you want to share? (optional)

In the video you can see, that when the statistics key expires, the player object key is also gone while it should have 5 seconds left. At the same time, a new statistics key will be created (which is not important). https://i.gyazo.com/ad584c00607032c41a83db2378ff51d8.mp4

Have you searched in our Wiki before posting ?

github-actions[bot] commented 1 year ago

Hello curious contributor ! Since it seems to be your first contribution, make sure that you've been:

Geolim4 commented 1 year ago

Hello,

I'm not sure how you get this behavior since the cache already benefits from the cache item with the longest date: https://github.com/PHPSocialNetwork/phpfastcache/blob/master/lib/Phpfastcache/Core/Pool/TaggableCacheItemPoolTrait.php#L430

Also the tags are tested against this behavior too: https://github.com/PHPSocialNetwork/phpfastcache/blob/master/tests/ItemTags.test.php

MaxChri commented 1 year ago

Hello,

I'm not sure how you get this behavior since the cache already benefits from the cache item with the longest date: https://github.com/PHPSocialNetwork/phpfastcache/blob/master/lib/Phpfastcache/Core/Pool/TaggableCacheItemPoolTrait.php#L430

Also the tags are tested against this behavior too: https://github.com/PHPSocialNetwork/phpfastcache/blob/master/tests/ItemTags.test.php

Hey Geo,

If you have multiple keys which have the same tag name and one key gets expired, all keys with the same tag name will be not be displayed anymore (even if they have been not expired) when using getItemsByTag(). This bug doesn't occour if the keys have different tag names. It also doesn't matter if you add just one tag or multiple. As long as one tag gets expired, all other keys with the same tags are gone as well.

In the code example you can see that key_1 getting removed at the same time of key_2, because key_2 getting expired after 5 seconds, while key_1 should have still 5 seconds to live. The cache item key_1 is still there in the pool but cannot longer be called with getItemsByTag().

Simple test:

// instance
$InstanceCache = CacheManager::getInstance('memcached', new Config([
    'host' =>'127.0.0.1',
    'port' => 11211,
    // 'sasl_user' => false, // optional
    // 'sasl_password' => false // optional
]));

// key_1
$CachedString1 = $InstanceCache->getItem("key_1");
if (is_null($CachedString1->get())) {
    $CachedString1->set("data1")->expiresAfter(10);
    $CachedString1->addTag("query");
    $InstanceCache->save($CachedString1);
}

// key_2
$CachedString2 = $InstanceCache->getItem("key_2");
if (is_null($CachedString2->get())) {
    $CachedString2->set("data2")->expiresAfter(5);
    $CachedString2->addTag("query");
    $InstanceCache->save($CachedString2);
}

// application layer
$keys = $InstanceCache->getItemsByTag("query");
$aCacheObjects = null;
foreach ($keys as $key) {
    $oCacheItem = new stdClass();
    $oCacheItem->key = $key->getKey();
    $oCacheItem->ttl = $key->getTtl();
    $oCacheItem->tags = $key->getTags();
    $aCacheObjects[] = $oCacheItem;
}

// render
echo "<pre>";
print_r($aCacheObjects);
echo "</pre>";
Geolim4 commented 1 year ago

Interesting, if you set the 5 sec key first then the 10 sec, the result are valid. By your example, you does the opposite and there's, indeed an issue.

Geolim4 commented 1 year ago

That's why I doubted it first, because my tests are doing the first use case, not yours. I'm investigating.

Geolim4 commented 1 year ago

Can you please open lib/Phpfastcache/Core/Pool/TaggableCacheItemPoolTrait.php

Search for:


            $tagsItem->set(\array_merge((array)$data, [$item->getKey() => $expTimestamp]))
                ->expiresAt($item->getExpirationDate());

and replace it with:

            $tagsItem->set(\array_merge((array)$data, [$item->getKey() => $expTimestamp]));

            if (!$tagsItem->isHit() || $tagsItem->getExpirationDate() < $item->getExpirationDate()) {
                $tagsItem->expiresAt($item->getExpirationDate());
            }

and tell me if it's working on your side :)

MaxChri commented 1 year ago

I have replaced the code and it works perfectly now. Thank you very much for your quick help, I really appreciate it! :)

Geolim4 commented 1 year ago

Okay, I'll push a fix tonight, but don't expect a release immediately, maybe this weekend or later 😉

Geolim4 commented 1 year ago

I will make an anti-regression test as well, since this behavior was difficult to catch without your specific case.

Geolim4 commented 1 year ago

Versions 9.1.3 and 8.1.4 have been released.

Happy caching !