From d0cfffb0df102808f9e4abf75abe5cbbfb9001c0 Mon Sep 17 00:00:00 2001 From: Petrisor Ciprian Daniel Date: Thu, 2 Jan 2025 20:42:57 +0200 Subject: [PATCH 1/3] Fix predis command error checking --- Store/RedisStore.php | 39 ++++++++++++++----------------- Tests/Store/CombinedStoreTest.php | 5 +++- Tests/Store/PredisStoreTest.php | 10 ++++++-- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/Store/RedisStore.php b/Store/RedisStore.php index a7bf7fe..503d306 100644 --- a/Store/RedisStore.php +++ b/Store/RedisStore.php @@ -11,7 +11,7 @@ namespace Symfony\Component\Lock\Store; -use Predis\Response\ServerException; +use Predis\Response\Error; use Relay\Relay; use Symfony\Component\Lock\Exception\InvalidTtlException; use Symfony\Component\Lock\Exception\LockConflictedException; @@ -250,10 +250,10 @@ private function evaluate(string $script, string $resource, array $args): mixed } $result = $this->redis->evalSha($scriptSha, array_merge([$resource], $args), 1); + } - if (null !== $err = $this->redis->getLastError()) { - throw new LockStorageException($err); - } + if (null !== $err = $this->redis->getLastError()) { + throw new LockStorageException($err); } return $result; @@ -273,10 +273,10 @@ private function evaluate(string $script, string $resource, array $args): mixed } $result = $client->evalSha($scriptSha, array_merge([$resource], $args), 1); + } - if (null !== $err = $client->getLastError()) { - throw new LockStorageException($err); - } + if (null !== $err = $client->getLastError()) { + throw new LockStorageException($err); } return $result; @@ -284,26 +284,21 @@ private function evaluate(string $script, string $resource, array $args): mixed \assert($this->redis instanceof \Predis\ClientInterface); - try { - return $this->redis->evalSha($scriptSha, 1, $resource, ...$args); - } catch (ServerException $e) { - // Fallthrough only if we need to load the script - if (self::NO_SCRIPT_ERROR_MESSAGE !== $e->getMessage()) { - throw new LockStorageException($e->getMessage(), $e->getCode(), $e); + $result = $this->redis->evalSha($scriptSha, 1, $resource, ...$args); + if ($result instanceof Error && self::NO_SCRIPT_ERROR_MESSAGE === $result->getMessage()) { + $result = $this->redis->script('LOAD', $script); + if ($result instanceof Error) { + throw new LockStorageException($result->getMessage()); } - } - try { - $this->redis->script('LOAD', $script); - } catch (ServerException $e) { - throw new LockStorageException($e->getMessage(), $e->getCode(), $e); + $result = $this->redis->evalSha($scriptSha, 1, $resource, ...$args); } - try { - return $this->redis->evalSha($scriptSha, 1, $resource, ...$args); - } catch (ServerException $e) { - throw new LockStorageException($e->getMessage(), $e->getCode(), $e); + if ($result instanceof Error) { + throw new LockStorageException($result->getMessage()); } + + return $result; } private function getUniqueToken(Key $key): string diff --git a/Tests/Store/CombinedStoreTest.php b/Tests/Store/CombinedStoreTest.php index b7cdd7a..6a19805 100644 --- a/Tests/Store/CombinedStoreTest.php +++ b/Tests/Store/CombinedStoreTest.php @@ -39,7 +39,10 @@ protected function getClockDelay(): int public function getStore(): PersistingStoreInterface { - $redis = new \Predis\Client(array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => 6379])); + $redis = new \Predis\Client( + array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => 6379]), + ['exceptions' => false], + ); try { $redis->connect(); diff --git a/Tests/Store/PredisStoreTest.php b/Tests/Store/PredisStoreTest.php index 3569e3d..74a72b5 100644 --- a/Tests/Store/PredisStoreTest.php +++ b/Tests/Store/PredisStoreTest.php @@ -20,7 +20,10 @@ class PredisStoreTest extends AbstractRedisStoreTestCase { public static function setUpBeforeClass(): void { - $redis = new \Predis\Client(array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null])); + $redis = new \Predis\Client( + array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null]), + ['exceptions' => false], + ); try { $redis->connect(); } catch (\Exception $e) { @@ -30,7 +33,10 @@ public static function setUpBeforeClass(): void protected function getRedisConnection(): \Predis\Client { - $redis = new \Predis\Client(array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null])); + $redis = new \Predis\Client( + array_combine(['host', 'port'], explode(':', getenv('REDIS_HOST')) + [1 => null]), + ['exceptions' => false], + ); $redis->connect(); return $redis; From 3b4e407251c2febb68004d7a786b717998cfcb6d Mon Sep 17 00:00:00 2001 From: PatNowak Date: Mon, 13 Jan 2025 09:29:13 +0100 Subject: [PATCH 2/3] [Lock] Make sure RedisStore will also support Valkey --- Store/RedisStore.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Store/RedisStore.php b/Store/RedisStore.php index 503d306..23f6289 100644 --- a/Store/RedisStore.php +++ b/Store/RedisStore.php @@ -29,7 +29,7 @@ class RedisStore implements SharedLockStoreInterface { use ExpiringStoreTrait; - private const NO_SCRIPT_ERROR_MESSAGE = 'NOSCRIPT No matching script. Please use EVAL.'; + private const NO_SCRIPT_ERROR_MESSAGE_PREFIX = 'NOSCRIPT No matching script.'; private bool $supportTime; @@ -234,7 +234,7 @@ private function evaluate(string $script, string $resource, array $args): mixed $this->redis->clearLastError(); $result = $this->redis->evalSha($scriptSha, array_merge([$resource], $args), 1); - if (self::NO_SCRIPT_ERROR_MESSAGE === $err = $this->redis->getLastError()) { + if (null !== ($err = $this->redis->getLastError()) && str_starts_with($err, self::NO_SCRIPT_ERROR_MESSAGE_PREFIX)) { $this->redis->clearLastError(); if ($this->redis instanceof \RedisCluster) { @@ -263,7 +263,7 @@ private function evaluate(string $script, string $resource, array $args): mixed $client = $this->redis->_instance($this->redis->_target($resource)); $client->clearLastError(); $result = $client->evalSha($scriptSha, array_merge([$resource], $args), 1); - if (self::NO_SCRIPT_ERROR_MESSAGE === $err = $client->getLastError()) { + if (null !== ($err = $this->redis->getLastError()) && str_starts_with($err, self::NO_SCRIPT_ERROR_MESSAGE_PREFIX)) { $client->clearLastError(); $client->script('LOAD', $script); @@ -285,7 +285,7 @@ private function evaluate(string $script, string $resource, array $args): mixed \assert($this->redis instanceof \Predis\ClientInterface); $result = $this->redis->evalSha($scriptSha, 1, $resource, ...$args); - if ($result instanceof Error && self::NO_SCRIPT_ERROR_MESSAGE === $result->getMessage()) { + if ($result instanceof Error && str_starts_with($result->getMessage(), self::NO_SCRIPT_ERROR_MESSAGE_PREFIX)) { $result = $this->redis->script('LOAD', $script); if ($result instanceof Error) { throw new LockStorageException($result->getMessage()); From 4f6e8b0e03e4a76095f7d058d72e72d30d5f59e5 Mon Sep 17 00:00:00 2001 From: PatNowak Date: Thu, 16 Jan 2025 13:07:48 +0100 Subject: [PATCH 3/3] Issue 59387-2: make check with prefix more robust --- Store/RedisStore.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Store/RedisStore.php b/Store/RedisStore.php index 23f6289..5d82844 100644 --- a/Store/RedisStore.php +++ b/Store/RedisStore.php @@ -29,7 +29,7 @@ class RedisStore implements SharedLockStoreInterface { use ExpiringStoreTrait; - private const NO_SCRIPT_ERROR_MESSAGE_PREFIX = 'NOSCRIPT No matching script.'; + private const NO_SCRIPT_ERROR_MESSAGE_PREFIX = 'NOSCRIPT'; private bool $supportTime;