laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.5k stars 11.02k forks source link

RedisStore throw an unserialization notice when a key used by an atomic lock is checked. #44216

Closed juanparati closed 2 years ago

juanparati commented 2 years ago

Description:

I observed that Laravel raises a "PHP Notice" when a key used by an atomic lock is verified with the "has" method.

Steps To Reproduce:

  1. Remove the 'lock_connection' from the redis configuration into cache.php config file.
  2. Run artisan cache:clear
  3. In Laravel Tinker type:
\Cache::lock('testlock', 20)->get();
\Cache::has('testlock');

Received result

PHP Notice:  unserialize(): Error at offset 0 of 16 bytes in /home/vagrant/testproject/code/vendor/laravel/framework/src/Illuminate/Cache/RedisStore.php on line 345
true

Expected result

Do not raise notice.

Notes

The result is received correctly however with a notice. I am not sure if to use the "has" method in order to verify if a lock was acquire without to previously acquire the lock is good practice.

driesvints commented 2 years ago

Does this also happens on the latest Laravel release?

juanparati commented 2 years ago

Hi Dries, I can reproduce with 9.30, however I was doing some research and I think I found the reason.

It seems that my cache.php config file has missing the 'lock_connection' parameter (My cache.php config file is a legacy one from an old Laravel version). When this parameter is missing then I can reproduce this issue (I edited the original bug report).

I observed that when the parameter is present the behavior is also different, so when 'lock_connection' => 'default' is present the "has" method will not find the key used in the lock and the notice is not raised.

Example:

Psy Shell v0.11.8 (PHP 8.0.17 — cli) by Justin Hileman
>>> \Cache::lock('testlock1', 20)->get();
=> true

>>> \Cache::has('testlock1');
=> false

If I remove the "lock_connection" from the configuration the "has" method will be able to find the key used. Example:

Psy Shell v0.11.8 (PHP 8.0.17 — cli) by Justin Hileman
>>> \Cache::lock('testlock1', 20)->get();
=> true

>>> \Cache::has('testlock1');
PHP Notice:  unserialize(): Error at offset 0 of 16 bytes in /home/vagrant/test/laravel/vendor/laravel/framework/src/Illuminate/Cache/RedisStore.php on line 345
=> true
juanparati commented 2 years ago

It's logic that "has" method cannot find the key used in the atomic lock because the "lock_connection" indicates a different "connection" for the locks.

driesvints commented 2 years ago

Ow I see. Yeah just don't remove that parameter from your config please.