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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saving an item that is already in the pool after its expiration time fails #897

Closed
1 task done
marcos-guerrero opened this issue Feb 16, 2023 · 13 comments
Closed
1 task done

Comments

@marcos-guerrero
Copy link

marcos-guerrero commented Feb 16, 2023

What type of issue is this?

Incorrect/unexpected/unexplainable behavior

Operating system + version

ubuntu 22.04

PHP version

8.2.3

Connector/Database version (if applicable)

redis_version:7.0.8

Phpfastcache version

9.1.2 馃敹

Describe the issue you're facing

I'm using Redis Driver with defaultTtl configured, so usually I don't use expiresAfter() nor expiresAt() Item API methods

Once the item is in the cache pool (because I just saved it with save() ItemPool method or because I retrieved it with getItem() method) , I sleep for defaultTtl + 1 seconds , just to be sure that the item is expired. Then, I save it again.

  • if I don't set an explicit ttl (relying on the defaultTtl) the result of save method is FALSE an the item is not saved
  • if I set an explicit ttl with expiresAfter() everything goes as expected : the result of save() method is TRUE and the item is saved again to redis

Expected behavior

The expected behaviour is the item to be saved no matter if explicit ttl is set or not

Code sample (optional)

<?php

require_once __DIR__ . '/composer/vendor/autoload.php';

use Phpfastcache\CacheManager;
use Phpfastcache\Drivers\Redis\Config as RedisConfig;
use Phpfastcache\Core\Item\ExtendedCacheItemInterface;
use Phpfastcache\Core\Pool\ExtendedCacheItemPoolInterface;

class Cache
{
    private ExtendedCacheItemPoolInterface $cacheInstance;

    public function __construct(array $config)
    {
        $this->cacheInstance = CacheManager::getInstance('Redis', new RedisConfig($config));
    }

    private function isItemInCache(ExtendedCacheItemInterface $cacheItem) : bool
    {
        return $cacheItem->isHit() && !$cacheItem->isExpired();
    }

    public function get(string $key, mixed &$value) : bool
    {
        //obtenemos el item
        $item = $this->cacheInstance->getItem($key);
        //comprobamos si est谩 o no
        if (!$this->isItemInCache($item))
        {
            return false;
        }
        //dejamos el valor en la variable
        $value = $item->get();
        return true;
    }

    public function set(string $key, mixed $value, int $ttl = 0) : bool
    {
        //obtenemos el item
        $item = $this->cacheInstance->getItem($key);
        //ponemos el valor
        $item->set($value);
        //si nos ponen un tiempo de ttl positivo, se lo ajustamos
        if ($ttl > 0)
        {
            $item = $item->expiresAfter($ttl);
        }
        //devolvemos el resultado del save
        return $this->cacheInstance->save($item);
    }
}


$config = [
    'host' => 'redis',
    'defaultTtl' => 2, 
];

$cache = new Cache($config);

$key = 'THEKEY';

echo 'SET: ' . ($cache->set($key, 'dfklasdfhasdkfasdf') ? 'OK' : 'FAIL') . PHP_EOL;

sleep($config['defaultTtl'] + 1);

echo 'SET AGAIN: ' . ($cache->set($key, 'sdfsadwerw3w4') ? 'OK' : 'FAIL') . PHP_EOL;

Suggestion to fix the issue (optional)

No response

References (optional)

No response

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

No response

Have you searched in our Wiki before posting ?

  • I have searched over the Wiki
@Geolim4
Copy link
Member

Geolim4 commented Feb 16, 2023

Hello,

That's an expected behavior to be honest, not a bug.

The item object is statically still present in the Phpfastcache internal storage.

So you're basically just trying to write an expired item to Redis which return false, as expected.

You have two options:

  • Use the "Detach API" and reconfigure your cache item:
  • Disable useStaticItemCaching, see options in Wiki

So, It's not a bug, it's a feature :D

@marcos-guerrero
Copy link
Author

Thank you very much for your attention and fast answer.
Ok, I'll study the options and decide what to do, but for me maybe the best option is to set ttl everytime I save an item, because relying on defaultTtl is not giving the behaviour I expected

@Geolim4
Copy link
Member

Geolim4 commented Feb 16, 2023

The default ttl only work for new cache items, that does not exist in the internal storage.
If an object with the specified key exists in the cache storage or in the backend, the default values are not applied.

@marcos-guerrero
Copy link
Author

marcos-guerrero commented Feb 16, 2023

The default ttl only work for new cache items, that does not exist in the internal storage. If an object with the specified key exists in the cache storage or in the backend, the default values are not applied.

Thank you again for your help, yes, I understand, that's why I'm thinking of not using defaulTtl anymore and substitute it with an explicit expiresAfter() that works in new and already existing items

@Geolim4
Copy link
Member

Geolim4 commented Feb 16, 2023

The static storage is intended to avoid querying the original backend if the item has been already fetched using getItem* methods. If you disable useStaticItemCaching, the core will ignore static storage and will always query the original backend. Therefore if the item has expired and the backend returns nothing, you will get a new * not hit * fresh item with default TTL applied (900s)

@marcos-guerrero
Copy link
Author

The static storage is intended to avoid querying the original backend if the item has been already fetched using getItem* methods. If you disable useStaticItemCaching, the core will ignore static storage and will always query the original backend. Therefore if the item has expired and the backend returns nothing, you will get a new * not hit * fresh item with default TTL applied (900s)

I understand that disabling static storage is easy and no code changes are required, but sure the performance will be affected, so having the chance to tweak my set($key, $value, $ttl) method and call expiresAfter() always (with default or explicit ttl) maybe is the solution with minimum code changes and not sacrifice performance... Don't you think so?

@Geolim4
Copy link
Member

Geolim4 commented Feb 16, 2023

Disabling static storage may slightly reduce performances for applications that critically need fresh cache item.
In your case, I think you should, indeed, always specify the desired expiration time to be POLA-compliant :P

@marcos-guerrero
Copy link
Author

POLA-compliant

Sorry, I don't catch it ...

@Geolim4
Copy link
Member

Geolim4 commented Feb 16, 2023

POLA-compliant

Sorry, I don't catch it ...

Principe Of Last Astonishment, look on Wikipedia.
Basically it means, always be as much explicit as you can, in order to avoid any undesired behavior from your code :)

@marcos-guerrero
Copy link
Author

Ok! Interesting, thank you very much! I understand
Last but not least, I want to thank you for your software, phpfastcache rocks! It made our migration from apcu and memcached to redis very easy, and the tags feature is very very useful!

@Geolim4
Copy link
Member

Geolim4 commented Feb 18, 2023

Ok! Interesting, thank you very much! I understand Last but not least, I want to thank you for your software, phpfastcache rocks! It made our migration from apcu and memcached to redis very easy, and the tags feature is very very useful!

Thank you :)
Everything's good for you now ?

@marcos-guerrero
Copy link
Author

Ok! Interesting, thank you very much! I understand Last but not least, I want to thank you for your software, phpfastcache rocks! It made our migration from apcu and memcached to redis very easy, and the tags feature is very very useful!

Thank you :) Everything's good for you now ?

Aye! Everything is ok for me now!

@Geolim4
Copy link
Member

Geolim4 commented Feb 20, 2023

Great :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants