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.38k stars 452 forks source link

Multiple driver errors caused by invalid return type of `driverRead()` #862

Closed ShockedPlot7560 closed 2 years ago

ShockedPlot7560 commented 2 years ago

Configuration

Describe the bug

I don't really know the nature of the bug, but it seems to come from the File driver Phpfastcache\Drivers\Files\Driver::driverRead()

To Reproduce Code to reproduce the issue:

/** @var ExtendedCacheItemPoolInterface */
$cacheManager = $this->container->get(CacheManager::class);
$cacheString = "SimplyString";
$cacheItem = $cacheManager->getItem($cacheString);

Driver used : files

Stacktrace

TypeError: Phpfastcache\Drivers\Files\Driver::driverRead(): Return value must be of type ?array, bool returned in /var/www/html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Files/Driver.php:74
Stack trace:
#0 /var/www/html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(132): Phpfastcache\Drivers\Files\Driver->driverRead(Object(Phpfastcache\Drivers\Files\Item))
#1 /var/www/html/src/core/middleware/MaintenanceMiddleware.php(32): Phpfastcache\Drivers\Files\Driver->getItem('MaintenanceAuto...')
...
#7 {main}
github-actions[bot] commented 2 years ago

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

aemla commented 2 years ago

I have the same issue with Memcached driver.

PHP Fatal error:  Uncaught TypeError: Phpfastcache\Drivers\Memcached\Driver::driverRead(): Return value must be of type ?array, string returned in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php:154
Stack trace:
#0 /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(132): Phpfastcache\Drivers\Memcached\Driver->driverRead()
#1 /public_html/controller/Controller.php(61): Phpfastcache\Drivers\Memcached\Driver->getItem()
...
#4 {main}
  thrown in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php on line 154
Geolim4 commented 2 years ago

This behavior is very surprising as all tests would have been broken if such behavior would happens.

Did you decorated Phpfastcache services or something ?

If you see in /tests lot of call to getItem are made, especially with Files driver.

(See ReadWriteOperations.test.php)

Geolim4 commented 2 years ago

Can you please post your full code implementation below ?

Because this is the very first time report me this kind of issue that should be impossible to produce, especially with LOT of tests calling getItem().

Geolim4 commented 2 years ago

I have the same issue with Memcached driver.

* **PhpFastCache version:** 9.1.0

* **PHP version:** 8.0.17

* **Operating system:** Ubuntu
PHP Fatal error:  Uncaught TypeError: Phpfastcache\Drivers\Memcached\Driver::driverRead(): Return value must be of type ?array, string returned in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php:154
Stack trace:
#0 /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(132): Phpfastcache\Drivers\Memcached\Driver->driverRead()
#1 /public_html/controller/Controller.php(61): Phpfastcache\Drivers\Memcached\Driver->getItem()
...
#4 {main}
  thrown in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php on line 154

Can't reproduce it neither, I'm gonna need more information with your full code implementation. For confidentiality you can send me sensible code at contact at geolim4.com image

ShockedPlot7560 commented 2 years ago

Hello, The error also seems very strange to me and never happens in local but only in production.
I will detail below the architecture likely to make a bug and will take the simplest case which brings me this error :

Before anything, I use the PHP-DI container for dependency injection that I build like this:

$buildeur = new ContainerBuilder();
$buildeur->addDefinitions($pathToConfigFile);
$buildeur->enableCompilation(__ROOT_FOLDER . "/tmp");
$container = $buildeur->build();

The configuration file is of type:

<?php

use DI\Container;
use Phpfastcache\CacheManager;
use Phpfastcache\Config\ConfigurationOption;

require __ROOT_FOLDER . "/vendor/autoload.php";

return [
    CacheManager::class => function (Container $container) {
        $instance = $container->get("ENV") === "dev" ? "Devnull" : "files";
        CacheManager::setDefaultConfig(new ConfigurationOption([
            "path" => $container->get("tmp.path")
        ]));
        return CacheManager::getInstance($instance);
    }
];

After that, so the compilation of the container has been defined, and phpfastcache and DI share the same tmp/ directory.
A case that I have a problem with is going to be below. Or, the code that seems to be problematic anyway, a very trivial value retrieval and saving code.

//I have simplified the operation and removed the portions of code that are not necessary and do not interfere
$pdo = $container->get(PDO::class);
/** @var ExtendedCacheItemPoolInterface */
$cacheManager = $container->get(CacheManager::class);
$cacheString = "Maintenance";
$cacheItem = $cacheManager->getItem($cacheString);
if ($config["maintenance"] !== true && $pdo !== null) {
    $cacheItem->set(false)->expiresAfter(3600 * 24);
    $cacheManager->save($cacheItem);
} else {
    if($pdo === null){
        if($cacheItem->get() === false || $cacheItem->get() === null){
            // send notification to discord
        }
        if(!$cacheItem->isHit()){
            $cacheItem->set(true)->expiresAfter(3600 * 24);
        }
        $cacheManager->save($cacheItem);
    }
}
Geolim4 commented 2 years ago

I think you are facing race condition issue with Files driver. Try to enable the option preventCacheSlams (only for Files driver), see Wiki.

Also I suggest you a better approach with the CacheManager.

Instead of calling:

        CacheManager::setDefaultConfig(new ConfigurationOption([
            "path" => $container->get("tmp.path")
        ]));

Try to do:

        return CacheManager::getInstance($instance, [
            'path' => $container->get("tmp.path"),
            'preventCacheSlams' => true,
                ]);
                // OR

Also try a more performant driver like Redis if you can, this will be more appropriated for heavy cache usage.

ShockedPlot7560 commented 2 years ago

I'll try your answer, however regarding the Redis driver, it takes, I guess a lot of memory space?

Geolim4 commented 2 years ago

Not at all, redis is one of the lightweight memory backend I know. It is popular because of its efficiency and few ressources needed ๐Ÿ˜

But first try to enable the option I suggested above.

ShockedPlot7560 commented 2 years ago

Update after testing in production (where it crashes). The error is always launched having set the configuration as requested. I will try to switch to the Redis driver to see if it still happens or not.

Geolim4 commented 2 years ago

How many request per second do you have ?

Can you invit me to talk on discord maybe ? :)

ShockedPlot7560 commented 2 years ago

This can go up I think to 10/15 requests per second at most. They are then limited by the capacity of my proxy and the traffic.

Of course, here is my discord: ShockedPlot7560 # 9999

Geolim4 commented 2 years ago

Added you, so we can discuss more clearly about your issue !

Geolim4 commented 2 years ago

I have the same issue with Memcached driver.

* **PhpFastCache version:** 9.1.0

* **PHP version:** 8.0.17

* **Operating system:** Ubuntu
PHP Fatal error:  Uncaught TypeError: Phpfastcache\Drivers\Memcached\Driver::driverRead(): Return value must be of type ?array, string returned in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php:154
Stack trace:
#0 /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(132): Phpfastcache\Drivers\Memcached\Driver->driverRead()
#1 /public_html/controller/Controller.php(61): Phpfastcache\Drivers\Memcached\Driver->getItem()
...
#4 {main}
  thrown in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php on line 154

I need more information @aemla. What is the string returned exactly ? Can you dump me the content of that string please ? Because unlike @ShockedPlot7560 I can't reproduce your issue.

aemla commented 2 years ago

I have the same issue with Memcached driver.

* **PhpFastCache version:** 9.1.0

* **PHP version:** 8.0.17

* **Operating system:** Ubuntu
PHP Fatal error:  Uncaught TypeError: Phpfastcache\Drivers\Memcached\Driver::driverRead(): Return value must be of type ?array, string returned in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php:154
Stack trace:
#0 /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(132): Phpfastcache\Drivers\Memcached\Driver->driverRead()
#1 /public_html/controller/Controller.php(61): Phpfastcache\Drivers\Memcached\Driver->getItem()
...
#4 {main}
  thrown in /public_html/vendor/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Memcached/Driver.php on line 154

I need more information @aemla. What is the string returned exactly ? Can you dump me the content of that string please ? Because unlike @ShockedPlot7560 I can't reproduce your issue.

It can be reproduced by running it in a loop. I wrote a little example, it doesn't make any sense, but it throws the same error. ๐Ÿ˜ In my original code, I am getting data from a database. I am getting this error only with the Memcached driver. Files driver works fine.

Dump of that string: string(0) ""

    function test()
    {
        $array = [
            ['userId' => 1, 'data' => 'test'],
            ['userId' => 2, 'data' => 'test'],
            ['userId' => 3, 'data' => 'test'],
            ['userId' => 4, 'data' => 'test'],
            ['userId' => 5, 'data' => 'test'],
            ['userId' => 6, 'data' => 'test'],
            ['userId' => 7, 'data' => 'test'],
            ['userId' => 8, 'data' => 'test'],
            ['userId' => 9, 'data' => 'test'],
            ['userId' => 10, 'data' => 'test'],
            ['userId' => 11, 'data' => 'test'],
            ['userId' => 12, 'data' => 'test'],
            ['userId' => 13, 'data' => 'test'],
            ['userId' => 14, 'data' => 'test'],
            ['userId' => 15, 'data' => 'test'],
            ['userId' => 16, 'data' => 'test'],
            ['userId' => 17, 'data' => 'test'],
            ['userId' => 18, 'data' => 'test'],
            ['userId' => 19, 'data' => 'test'],
            ['userId' => 20, 'data' => 'test'],
            ['userId' => 21, 'data' => 'test'],
            ['userId' => 22, 'data' => 'test'],
        ];

        $cache = CacheManager::getInstance('Memcached');

        // The error only occurs when writing to the cache, so let's clear it before running the loop.
        $cache->clear();

        foreach ($array as $row) {
            $this->getFromCache($cache, $row['userId'], $row['data']);
        }
    }

    function getFromCache(ExtendedCacheItemPoolInterface $cache, $userId, $data)
    {
        $cachedString = $cache->getItem('dataById=' . $userId);
        if (!$cachedString->isHit()) {
            $cachedString->set($data)->expiresAfter(300);
            $cache->save($cachedString);

            return $cachedString->get();
        } else {
            return $cachedString->get();
        }
    }
Geolim4 commented 2 years ago

I ran your code sample:

image

And I have no error at all, so I can't reproduce your bug :/

Geolim4 commented 2 years ago

What version of php-memcached version are you using ? Do you run multiple concurrent process ?

aemla commented 2 years ago

This error occurs only in 9.x, it works fine in 8.x. I am using LSMCD https://github.com/litespeedtech/lsmcd could this be the reason why it sometimes returns an empty string?

aemla commented 2 years ago

This issue can be fixed by adding a check for an empty string.

    /**
     * @param ExtendedCacheItemInterface $item
     * @return null|array
     */
    protected function driverRead(ExtendedCacheItemInterface $item): ?array
    {
        $val = $this->instance->get($item->getKey());

        if ($val === false || $val === โ€œโ€) {
            return null;
        }

        return $val;
    }
Geolim4 commented 2 years ago

"it works fine in 8.x" ?

That's interesting. Did you tried in v8.1.x or previous one ?

Geolim4 commented 2 years ago

I understand now, I typed driverRead with ?array as of v9.

Geolim4 commented 2 years ago

Ok ok, that make sense now.

v8 was workin well because I previously made a weak validation: https://github.com/PHPSocialNetwork/phpfastcache/blob/894421d73b8aa1eb839553cfcb3201be13ecda90/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php#L139

But as of v9 I introduced (very) strict types everywhere this one can be problematic under some circumstances.

Geolim4 commented 2 years ago

Just out of curiosity tell me if replacing:

        if ($val === false) {
            return null;
        }

by

        if (empty($val) || !\is_array($val)) {
            return null;
        }

fixes your issue ?

aemla commented 2 years ago

Just out of curiosity tell me if replacing:

        if ($val === false) {
            return null;
        }

by

        if (empty($val) || !\is_array($val)) {
            return null;
        }

fixes your issue ?

Yes, now it works fine.

Geolim4 commented 2 years ago

Roger that. I'll push a fix tonight and make a new release by the end of the week at late.

GrandFelix commented 1 year ago

Hi,

we are still getting the same error. We are using version 9.1.2 and php 8.1.

PHP Fatal error:  Uncaught TypeError: Phpfastcache\Drivers\Files\Driver::driverRead(): Return value must be of type ?array, bool returned in /var/www/prod/app/vendor8/phpfastcache/phpfastcache/lib/Phpfastcache/Drivers/Files/Driver.php:74
Stack trace:
#0 /var/www/prod/app/vendor8/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(143): Phpfastcache\Drivers\Files\Driver->driverRead()
#1 /var/www/prod/app/vendor8/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(106): Phpfastcache\Drivers\Files\Driver->getItem()
#2 /var/www/prod/app/vendor8/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/TaggableCacheItemPoolTrait.php(399): Phpfastcache\Drivers\Files\Driver->getItems()
#3 /var/www/prod/app/vendor8/phpfastcache/phpfastcache/lib/Phpfastcache/Core/Pool/CacheItemPoolTrait.php(386): Phpfastcache\Drivers\Files\Driver->driverWriteTags()
#4 /var/www/prod/app/library/App/Cache/Cache.php(76): Phpfastcache\Drivers\Files\Driver->save()

Any tip?

Geolim4 commented 1 year ago

You seem to have concurrency issue as the file exists on disk but looks empty, very weird.

Did you tried to enable preventCacheSlams option ?

GrandFelix commented 1 year ago

will try and I report back.

GrandFelix commented 1 year ago

We have tried it and we still get the same error. I think the problem is in decode() function when calling unserialize() which one return false when it cant unserialize a string.

Geolim4 commented 1 year ago

Then you have a serious problem of I/O with your disk this is is not supposed to happens unless the file are corrupted or partially written which is a sign of a failing disk. I have multiple tests with this driver on the CI including concurrent writes and I'm not encountering this kind of issue.

GrandFelix commented 1 year ago

The problem is that this happens on multiple different servers and I really doubt that all servers have disk issues ๐Ÿ˜•

GrandFelix commented 1 year ago

now we have tired to set setSecureFileManipulation() and it looks good.