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

TypeError Phpfastcache\Drivers\Files\Driver::driverRead(): Return value must be of type ?array, bool returned #907

Closed Jankyz closed 8 months ago

Jankyz commented 8 months ago

What type of issue is this?

Exception/Error/Warning/Notice/Deprecation

Operating system + version

Linux 5.10.198-187.748.amzn2.x86_64

PHP version

8.0.26

Connector/Database version (if applicable)

No response

Phpfastcache version

9.1.3 ✅

Describe the issue you're facing

I'm being swamped with errors of this kind

TypeError Phpfastcache\Drivers\Files\Driver::driverRead(): Return value must be of type ?array, bool returned

I use Files as a driver I think that this method has a problem with return from this

    return $this->decode($content);

inside we have

return $value ? \unserialize($value, ['allowed_classes' => true]) : null;

and when data cannot be unserialized then the function returns false

example of invalid serialized object

$content = 'a:1:{s:5:"Test";s:15:"serialize here!";}';

Expected behavior

return should be array|null

github-actions[bot] commented 8 months ago

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

Geolim4 commented 8 months ago

Hello,

That's weird, unserialize only returns false upon invalid string passed through.

I'll take a look, but you are may facing corrupted stored cache files.

Cheers,Georges

Jankyz commented 8 months ago

it looks like Phpfastcache can serialize objects and save them (no errors), but it has a problem when we want to get the same objects and unserialize them...

Jankyz commented 8 months ago

Georges what about allowing people to choose between two methods in driver config

Geolim4 commented 8 months ago

Georges what about allowing people to choose between two methods in driver config

* serialize

* json_encode
  ?

Because unlike unserialize, json_decode does not allow de-serializing object with custom class names.

jdalsem commented 8 months ago

Just ran into this myself. It was indeed a corrupted cache file in my situation. Any reason why you are not handling this? The decode function could be returning null if the unserialize function returns false

In case the passed string is not unserializeable, false is returned and E_NOTICE is issued.

Geolim4 commented 8 months ago

Just ran into this myself. It was indeed a corrupted cache file in my situation. Any reason why you are not handling this? The decode function could be returning null if the unserialize function returns false

In case the passed string is not unserializeable, false is returned and E_NOTICE is issued.

I'm not handling this case because if it happens it is a server issue rather than a Phpfastcache issue. So if a corrupted cache occurs the idea is to let you know be aware that there's an issue somewhere. No to handle it silently like nothing happened :)

jdalsem commented 8 months ago

I'm not handling this case because if it happens it is a server issue rather than a Phpfastcache issue. So if a corrupted cache occurs the idea is to let you know be aware that there's an issue somewhere. No to handle it silently like nothing happened :)

I understand, but you typehinted your functions, thus you are creating a coding issue. If you want this problem to be handled upstream, why not throw an exception?

Geolim4 commented 8 months ago

I understand, but you typehinted your functions, thus you are creating a coding issue. If you want this problem to be handled upstream, why not throw an exception?

I'm not creating a coding issue, your server is causing coding issue by storing corrupted files expecting Phpfastcache to silently handle the issue and get over it. If an exception is thrown or a PHP error appears, the result is still the same: An error somewhere in your application prevent Phpfastcache from running smoothly. Badly unserialize is one of the functions that should've thrown on invalid (like json_decode does now), but it still doesn't and return scalar on error instead of throwing exception.

So until now I felt no necessity to catch this kind of error because it is very rare and outside of Phpfastcache scope, at least in your case. 9.2 is coming in 2024, I'll add an exception on this block of code, but again, it is not a coding issue, just a server issue.

jdalsem commented 8 months ago

I'm not creating a coding issue

If your return types do not match, it is a coding issue. (mismatch between driverRead and decode return types)

If an exception is thrown or a PHP error appears, the result is still the same

With an exception i can determine upstream if it is a cache contents issue. A php coding error should not be used to identify this.

An error somewhere in your application

Corruption can happen for many reasons. In my experience it is almost never an application or phpfastcache issue, but just some weird stuff happening on disk.

The biggest problem i have is that i can not 'repair' the corrupted data, as the code crashes on a PHP issue. With an exception thrown i might be able to catch and delete the cache contents (hopefully that will not trigger the same issue).

Geolim4 commented 8 months ago

For now I consider that issue as an exclusive server-side issue. I can make a better error handling if you'd like so, but it is a low priority until next month since the 9.2 will be shipped with some new features, driver updates and internal performances improvements.

That block of code never thrown any errors until now even in the worst conditions with huge stress tests configured on the CI :/

jdalsem commented 8 months ago

I can make a better error handling if you'd like so

That would be nice.

That block of code never thrown any errors until now even in the worst conditions with huge stress tests configured on the CI :/

I think (for now) it is just a coïncidence it happend. We are using phpfastcache multiple years without any issue related to corrupted cache contents, so there is no need to rush anything now.

Geolim4 commented 8 months ago

Will be implemented as of v9.2.