[BUG]: Redis Cache/Storage Adapter race condition

[Aphexx]

Describe the bug
\Phalcon\Cache\Adapter\Redis has race condition that returns false in cases when value was deleted on redis server after this->has(key) check and before this->doGet(key). Also this->has(key) making excessive request to redis server.

cphalcon/phalcon/Storage/Adapter/AbstractAdapter.zep

Line 148 in ce1392e

if (true !== this->has(key)) {

To Reproduce

        $redisCache = new \Phalcon\Cache\Adapter\Redis(new \Phalcon\Storage\SerializerFactory(), [
            'index' => 3,
            'persistent' => false,
            'host' =>  '/tmp/redis-sock/redis.sock',
            'port' => null,
            'lifetime' => 3600,
            'prefix' => '_c_'
        ]);
        for($i=0;$i<200000;$i++) {
            $v = $redisCache->get('test2', 'default_value');
            if($v === 'default_value') {
                //correct behavior
                var_dump(['OK', $i, $v]);
                $redisCache->set('test2', 'value', 1);
            }else if($v === 'value') {
                //correct behavior
            } else {
                //incorrect behaviour. Must never get here
                var_dump(['ERROR', $i, $v]);
                die;
            }
        }

Will eventualy return false :

Expected behavior
Must return "value" if value exists in cache and "default_value" in case cache miss..

Solution
\Redis->get() returns bool false in case key not exists in Redis. We must remove this->has(key) request from \Phalcon\Cache\Adapter\Redis::get(), and add check for false. This will return $defaultValue for non existing keys, and will not create excessive request via this->has(key)

    public function get(string $key, $defaultValue =null) {
        $v = $this->doGet($key);
        if($v === false){
            return $defaultValue;
        }
        return $this->getUnserializedData($v, $defaultValue);
    }

Details

• Phalcon version: 5.1.4
• PHP Version: 8.1.14
• Operating System: Linux
• Installation type: pecl
• Zephir version (if any): no
• Server: Nginx

[Aphexx]

Other cache adapters can be optimized replacing this->has(key) check in get() with adapter-specific check. For example apcu:

    public function get(string $key, $defaultValue=null) {
        $success = null;
        $v = apcu_fetch($this->getPrefixedKey($key), $success);
        if(!$success) {
            return $defaultValue;
        }
        return $v;
    }

libmemcached (in current implementation actually doing 2 requests in $cache->get($key))

    public function get(string $key, $defaultValue=null) {
        $connection = $this->adapter();
        $v = $connection->get(key);
        if(\Memcached::RES_NOTFOUND  === $connection->getResultCode()) {
            return $defaultValue;
        }
        return $v;
    }

[niden]

What do we do if the actual stored value is false ?

$key = 'somekey';

$value = $this->doGet($key); // value is `false`
if (false === $value) {
    return $defaultValue; // null
}

return $value;

The reason we did it that way was just for the above scenario.

Thoughts?

[Aphexx]

Redis is not supporting storing boolean or int types, it stores everything as string. If the actual value will be false, it will be stored like "b:0;" string (depending of serializer adapter) and further passed to this->getUnserializedData and unserialized to php false.

Answering your question @niden, it will work properly with actual false value:

$key = 'somekey';

$serialized_value = $this->doGet($key); // if stored value is `false` will return string "b:0;" 
if (false === $value) {
    return $defaultValue; // null
}
$value = $this->getUnserializedData($serialized_value, $defaultValue); // "b:0;" will be unserialized to false
return $value;

[niden]

That I did not know :)

Thank you for reporting this. I will get on it shortly.