From f24b3b2ef9dc36f488bee23145d531a3ce7d2847 Mon Sep 17 00:00:00 2001 From: Raj Siva-Rajah <5361908+binaryfire@users.noreply.github.com> Date: Fri, 26 Dec 2025 01:38:03 +0000 Subject: [PATCH] Fix exception swallowing in Redis::__call() The previous implementation had a bug where exceptions thrown during Redis command execution were silently swallowed when: 1. A context connection existed (hasContextConnection=true) 2. The command required same-connection handling (multi/pipeline/select) This happened because `return` statements inside a `finally` block override any pending exception from the `catch` block. Changes: - Restructure try/catch/finally to capture exception without immediate re-throw - Move exception re-throw to after finally block completes - On error with same-connection commands: release connection instead of storing (a failed multi/pipeline/select has nothing to continue, storing would be wrong) - Add comprehensive test coverage for exception propagation and connection release logic across all scenarios The connection release behavior is now: - Success + context connection: don't release (managed by context) - Success + same-connection cmd: store in context, release via defer() - Success + normal cmd: release immediately - Error + context connection: don't release, propagate exception - Error + same-connection cmd: release immediately, propagate exception - Error + normal cmd: release immediately, propagate exception --- src/redis/src/Redis.php | 31 +++-- tests/Redis/RedisTest.php | 267 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 282 insertions(+), 16 deletions(-) create mode 100644 tests/Redis/RedisTest.php diff --git a/src/redis/src/Redis.php b/src/redis/src/Redis.php index c8da51f78..4beeb1ef4 100644 --- a/src/redis/src/Redis.php +++ b/src/redis/src/Redis.php @@ -28,16 +28,16 @@ public function __call($name, $arguments) $hasContextConnection = Context::has($this->getContextKey()); $connection = $this->getConnection($hasContextConnection); - $hasError = false; $start = (float) microtime(true); + $result = null; + $exception = null; try { /** @var RedisConnection $connection */ $connection = $connection->getConnection(); $result = $connection->{$name}(...$arguments); - } catch (Throwable $exception) { - $hasError = true; - throw $exception; + } catch (Throwable $e) { + $exception = $e; } finally { $time = round((microtime(true) - $start) * 1000, 2); $connection->getEventDispatcher()?->dispatch( @@ -47,31 +47,30 @@ public function __call($name, $arguments) $time, $connection, $this->poolName, - $result ?? null, - $exception ?? null, + $result, + $exception, ) ); if ($hasContextConnection) { - return $hasError ? null : $result; // @phpstan-ignore-line - } - - // Release connection. - if ($this->shouldUseSameConnection($name)) { + // Connection is already in context, don't release + } elseif ($exception === null && $this->shouldUseSameConnection($name)) { + // On success with same-connection command: store in context for reuse if ($name === 'select' && $db = $arguments[0]) { $connection->setDatabase((int) $db); } - // Should storage the connection to coroutine context, then use defer() to release the connection. Context::set($this->getContextKey(), $connection); defer(function () { $this->releaseContextConnection(); }); - - return $hasError ? null : $result; // @phpstan-ignore-line + } else { + // Release the connection + $connection->release(); } + } - // Release the connection after command executed. - $connection->release(); + if ($exception !== null) { + throw $exception; } return $result; diff --git a/tests/Redis/RedisTest.php b/tests/Redis/RedisTest.php new file mode 100644 index 000000000..46e88ee74 --- /dev/null +++ b/tests/Redis/RedisTest.php @@ -0,0 +1,267 @@ +createMockRedisConnection(); + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $result = $redis->get('key'); + + $this->assertEquals('value', $result); + } + + public function testSuccessfulCommandWithContextConnectionDoesNotReleaseConnection(): void + { + $mockRedisConnection = $this->createMockRedisConnection(); + $mockRedisConnection->shouldReceive('release')->never(); + + // Pre-set context connection + Context::set('redis.connection.default', $mockRedisConnection); + + $redis = $this->createRedis($mockRedisConnection); + + $result = $redis->get('key'); + + $this->assertEquals('value', $result); + } + + public function testSuccessfulMultiCommandStoresConnectionInContext(): void + { + $mockRedisConnection = $this->createMockRedisConnection('multi', true); + // Connection will be released via defer() when coroutine ends + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $result = $redis->multi(); + + $this->assertTrue($result); + $this->assertSame($mockRedisConnection, Context::get('redis.connection.default')); + } + + public function testSuccessfulSelectCommandStoresConnectionInContextAndSetsDatabase(): void + { + $mockRedisConnection = $this->createMockRedisConnection('select', true); + // Connection will be released via defer() when coroutine ends + $mockRedisConnection->shouldReceive('release')->once(); + $mockRedisConnection->shouldReceive('setDatabase')->with(2)->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $result = $redis->select(2); + + $this->assertTrue($result); + $this->assertSame($mockRedisConnection, Context::get('redis.connection.default')); + } + + public function testExceptionPropagatesToCaller(): void + { + $expectedException = new Exception('Redis error'); + + $mockRedisConnection = $this->createMockRedisConnection('get', null, $expectedException); + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $this->expectException(Exception::class); + $this->expectExceptionMessage('Redis error'); + + $redis->get('key'); + } + + public function testExceptionWithContextConnectionDoesNotReleaseConnection(): void + { + $expectedException = new Exception('Redis error'); + + $mockRedisConnection = $this->createMockRedisConnection('get', null, $expectedException); + $mockRedisConnection->shouldReceive('release')->never(); + + // Pre-set context connection + Context::set('redis.connection.default', $mockRedisConnection); + + $redis = $this->createRedis($mockRedisConnection); + + try { + $redis->get('key'); + $this->fail('Expected exception was not thrown'); + } catch (Exception $e) { + $this->assertEquals('Redis error', $e->getMessage()); + } + } + + public function testExceptionWithSameConnectionCommandReleasesConnectionInsteadOfStoring(): void + { + $expectedException = new Exception('Multi failed'); + + $mockRedisConnection = $this->createMockRedisConnection('multi', null, $expectedException); + // On error, connection should be released, NOT stored in context + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + try { + $redis->multi(); + $this->fail('Expected exception was not thrown'); + } catch (Exception $e) { + $this->assertEquals('Multi failed', $e->getMessage()); + } + + // Connection should NOT be stored in context on error + $this->assertNull(Context::get('redis.connection.default')); + } + + public function testEventDispatchedOnSuccess(): void + { + $mockEventDispatcher = Mockery::mock(EventDispatcherInterface::class); + $mockEventDispatcher->shouldReceive('dispatch') + ->once() + ->with(Mockery::on(function (CommandExecuted $event) { + return $event->command === 'get' + && $event->parameters === ['key'] + && $event->result === 'value' + && $event->throwable === null; + })); + + $mockRedisConnection = $this->createMockRedisConnection('get', 'value', null, $mockEventDispatcher); + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $redis->get('key'); + } + + public function testEventDispatchedOnErrorWithExceptionInfo(): void + { + $expectedException = new Exception('Redis error'); + + $mockEventDispatcher = Mockery::mock(EventDispatcherInterface::class); + $mockEventDispatcher->shouldReceive('dispatch') + ->once() + ->with(Mockery::on(function (CommandExecuted $event) use ($expectedException) { + return $event->command === 'get' + && $event->parameters === ['key'] + && $event->result === null + && $event->throwable === $expectedException; + })); + + $mockRedisConnection = $this->createMockRedisConnection('get', null, $expectedException, $mockEventDispatcher); + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + try { + $redis->get('key'); + } catch (Exception) { + // Expected + } + } + + public function testPipelineCommandStoresConnectionInContext(): void + { + $mockRedisConnection = $this->createMockRedisConnection('pipeline', true); + // Connection will be released via defer() when coroutine ends + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $result = $redis->pipeline(); + + $this->assertTrue($result); + $this->assertSame($mockRedisConnection, Context::get('redis.connection.default')); + } + + public function testRegularCommandDoesNotStoreConnectionInContext(): void + { + $mockRedisConnection = $this->createMockRedisConnection(); + $mockRedisConnection->shouldReceive('release')->once(); + + $redis = $this->createRedis($mockRedisConnection); + + $redis->get('key'); + + $this->assertNull(Context::get('redis.connection.default')); + } + + /** + * Create a mock Redis connection with configurable behavior. + */ + protected function createMockRedisConnection( + string $command = 'get', + mixed $returnValue = 'value', + ?Throwable $exception = null, + ?EventDispatcherInterface $eventDispatcher = null + ): RedisConnection&Mockery\MockInterface { + $mockPhpRedis = Mockery::mock(PhpRedis::class); + + if ($exception !== null) { + $mockPhpRedis->shouldReceive($command) + ->andThrow($exception); + } else { + $mockPhpRedis->shouldReceive($command) + ->andReturn($returnValue); + } + + $mockRedisConnection = Mockery::mock(RedisConnection::class); + $mockRedisConnection->shouldReceive('shouldTransform')->andReturnSelf(); + $mockRedisConnection->shouldReceive('getConnection')->andReturn($mockRedisConnection); + $mockRedisConnection->shouldReceive('getEventDispatcher')->andReturn($eventDispatcher); + + // Forward the command call to the mock PHP Redis + $mockRedisConnection->shouldReceive($command) + ->andReturnUsing(function (...$args) use ($mockPhpRedis, $command) { + return $mockPhpRedis->{$command}(...$args); + }); + + return $mockRedisConnection; + } + + /** + * Create a Redis instance with the given mock connection. + */ + protected function createRedis(RedisConnection $mockConnection): Redis + { + $mockPool = Mockery::mock(RedisPool::class); + $mockPool->shouldReceive('get')->andReturn($mockConnection); + $mockPool->shouldReceive('getOption')->andReturn(Mockery::mock(PoolOption::class)); + + $mockPoolFactory = Mockery::mock(PoolFactory::class); + $mockPoolFactory->shouldReceive('getPool')->with('default')->andReturn($mockPool); + + return new Redis($mockPoolFactory); + } +}