Skip to content

Commit 57cb40b

Browse files
albertchtalbertcht
authored
Merge pull request #315 from binaryfire/fix/redis-exception-swallowing
fix: exception swallowing in `Redis::__call()`
2 parents 23545a0 + f24b3b2 commit 57cb40b

File tree

2 files changed

+282
-16
lines changed

2 files changed

+282
-16
lines changed

‎src/redis/src/Redis.php‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,16 @@ public function __call($name, $arguments)
2828
$hasContextConnection = Context::has($this->getContextKey());
2929
$connection = $this->getConnection($hasContextConnection);
3030

31-
$hasError = false;
3231
$start = (float) microtime(true);
32+
$result = null;
33+
$exception = null;
3334

3435
try {
3536
/** @var RedisConnection $connection */
3637
$connection = $connection->getConnection();
3738
$result = $connection->{$name}(...$arguments);
38-
} catch (Throwable $exception) {
39-
$hasError = true;
40-
throw $exception;
39+
} catch (Throwable $e) {
40+
$exception = $e;
4141
} finally {
4242
$time = round((microtime(true) - $start) * 1000, 2);
4343
$connection->getEventDispatcher()?->dispatch(
@@ -47,31 +47,30 @@ public function __call($name, $arguments)
4747
$time,
4848
$connection,
4949
$this->poolName,
50-
$result ?? null,
51-
$exception ?? null,
50+
$result,
51+
$exception,
5252
)
5353
);
5454

5555
if ($hasContextConnection) {
56-
return $hasError ? null : $result; // @phpstan-ignore-line
57-
}
58-
59-
// Release connection.
60-
if ($this->shouldUseSameConnection($name)) {
56+
// Connection is already in context, don't release
57+
} elseif ($exception === null && $this->shouldUseSameConnection($name)) {
58+
// On success with same-connection command: store in context for reuse
6159
if ($name === 'select' && $db = $arguments[0]) {
6260
$connection->setDatabase((int) $db);
6361
}
64-
// Should storage the connection to coroutine context, then use defer() to release the connection.
6562
Context::set($this->getContextKey(), $connection);
6663
defer(function () {
6764
$this->releaseContextConnection();
6865
});
69-
70-
return $hasError ? null : $result; // @phpstan-ignore-line
66+
} else {
67+
// Release the connection
68+
$connection->release();
7169
}
70+
}
7271

73-
// Release the connection after command executed.
74-
$connection->release();
72+
if ($exception !== null) {
73+
throw $exception;
7574
}
7675

7776
return $result;

‎tests/Redis/RedisTest.php‎

Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,267 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Hypervel\Tests\Redis;
6+
7+
use Exception;
8+
use Hyperf\Pool\PoolOption;
9+
use Hyperf\Redis\Event\CommandExecuted;
10+
use Hyperf\Redis\Pool\PoolFactory;
11+
use Hyperf\Redis\Pool\RedisPool;
12+
use Hypervel\Context\Context;
13+
use Hypervel\Foundation\Testing\Concerns\RunTestsInCoroutine;
14+
use Hypervel\Redis\Redis;
15+
use Hypervel\Redis\RedisConnection;
16+
use Hypervel\Tests\TestCase;
17+
use Mockery;
18+
use Psr\EventDispatcher\EventDispatcherInterface;
19+
use Redis as PhpRedis;
20+
use Throwable;
21+
22+
/**
23+
* @internal
24+
* @coversNothing
25+
*/
26+
class RedisTest extends TestCase
27+
{
28+
use RunTestsInCoroutine;
29+
30+
protected function tearDown(): void
31+
{
32+
parent::tearDown();
33+
Context::destroy('redis.connection.default');
34+
}
35+
36+
public function testSuccessfulCommandReleasesConnection(): void
37+
{
38+
$mockRedisConnection = $this->createMockRedisConnection();
39+
$mockRedisConnection->shouldReceive('release')->once();
40+
41+
$redis = $this->createRedis($mockRedisConnection);
42+
43+
$result = $redis->get('key');
44+
45+
$this->assertEquals('value', $result);
46+
}
47+
48+
public function testSuccessfulCommandWithContextConnectionDoesNotReleaseConnection(): void
49+
{
50+
$mockRedisConnection = $this->createMockRedisConnection();
51+
$mockRedisConnection->shouldReceive('release')->never();
52+
53+
// Pre-set context connection
54+
Context::set('redis.connection.default', $mockRedisConnection);
55+
56+
$redis = $this->createRedis($mockRedisConnection);
57+
58+
$result = $redis->get('key');
59+
60+
$this->assertEquals('value', $result);
61+
}
62+
63+
public function testSuccessfulMultiCommandStoresConnectionInContext(): void
64+
{
65+
$mockRedisConnection = $this->createMockRedisConnection('multi', true);
66+
// Connection will be released via defer() when coroutine ends
67+
$mockRedisConnection->shouldReceive('release')->once();
68+
69+
$redis = $this->createRedis($mockRedisConnection);
70+
71+
$result = $redis->multi();
72+
73+
$this->assertTrue($result);
74+
$this->assertSame($mockRedisConnection, Context::get('redis.connection.default'));
75+
}
76+
77+
public function testSuccessfulSelectCommandStoresConnectionInContextAndSetsDatabase(): void
78+
{
79+
$mockRedisConnection = $this->createMockRedisConnection('select', true);
80+
// Connection will be released via defer() when coroutine ends
81+
$mockRedisConnection->shouldReceive('release')->once();
82+
$mockRedisConnection->shouldReceive('setDatabase')->with(2)->once();
83+
84+
$redis = $this->createRedis($mockRedisConnection);
85+
86+
$result = $redis->select(2);
87+
88+
$this->assertTrue($result);
89+
$this->assertSame($mockRedisConnection, Context::get('redis.connection.default'));
90+
}
91+
92+
public function testExceptionPropagatesToCaller(): void
93+
{
94+
$expectedException = new Exception('Redis error');
95+
96+
$mockRedisConnection = $this->createMockRedisConnection('get', null, $expectedException);
97+
$mockRedisConnection->shouldReceive('release')->once();
98+
99+
$redis = $this->createRedis($mockRedisConnection);
100+
101+
$this->expectException(Exception::class);
102+
$this->expectExceptionMessage('Redis error');
103+
104+
$redis->get('key');
105+
}
106+
107+
public function testExceptionWithContextConnectionDoesNotReleaseConnection(): void
108+
{
109+
$expectedException = new Exception('Redis error');
110+
111+
$mockRedisConnection = $this->createMockRedisConnection('get', null, $expectedException);
112+
$mockRedisConnection->shouldReceive('release')->never();
113+
114+
// Pre-set context connection
115+
Context::set('redis.connection.default', $mockRedisConnection);
116+
117+
$redis = $this->createRedis($mockRedisConnection);
118+
119+
try {
120+
$redis->get('key');
121+
$this->fail('Expected exception was not thrown');
122+
} catch (Exception $e) {
123+
$this->assertEquals('Redis error', $e->getMessage());
124+
}
125+
}
126+
127+
public function testExceptionWithSameConnectionCommandReleasesConnectionInsteadOfStoring(): void
128+
{
129+
$expectedException = new Exception('Multi failed');
130+
131+
$mockRedisConnection = $this->createMockRedisConnection('multi', null, $expectedException);
132+
// On error, connection should be released, NOT stored in context
133+
$mockRedisConnection->shouldReceive('release')->once();
134+
135+
$redis = $this->createRedis($mockRedisConnection);
136+
137+
try {
138+
$redis->multi();
139+
$this->fail('Expected exception was not thrown');
140+
} catch (Exception $e) {
141+
$this->assertEquals('Multi failed', $e->getMessage());
142+
}
143+
144+
// Connection should NOT be stored in context on error
145+
$this->assertNull(Context::get('redis.connection.default'));
146+
}
147+
148+
public function testEventDispatchedOnSuccess(): void
149+
{
150+
$mockEventDispatcher = Mockery::mock(EventDispatcherInterface::class);
151+
$mockEventDispatcher->shouldReceive('dispatch')
152+
->once()
153+
->with(Mockery::on(function (CommandExecuted $event) {
154+
return $event->command === 'get'
155+
&& $event->parameters === ['key']
156+
&& $event->result === 'value'
157+
&& $event->throwable === null;
158+
}));
159+
160+
$mockRedisConnection = $this->createMockRedisConnection('get', 'value', null, $mockEventDispatcher);
161+
$mockRedisConnection->shouldReceive('release')->once();
162+
163+
$redis = $this->createRedis($mockRedisConnection);
164+
165+
$redis->get('key');
166+
}
167+
168+
public function testEventDispatchedOnErrorWithExceptionInfo(): void
169+
{
170+
$expectedException = new Exception('Redis error');
171+
172+
$mockEventDispatcher = Mockery::mock(EventDispatcherInterface::class);
173+
$mockEventDispatcher->shouldReceive('dispatch')
174+
->once()
175+
->with(Mockery::on(function (CommandExecuted $event) use ($expectedException) {
176+
return $event->command === 'get'
177+
&& $event->parameters === ['key']
178+
&& $event->result === null
179+
&& $event->throwable === $expectedException;
180+
}));
181+
182+
$mockRedisConnection = $this->createMockRedisConnection('get', null, $expectedException, $mockEventDispatcher);
183+
$mockRedisConnection->shouldReceive('release')->once();
184+
185+
$redis = $this->createRedis($mockRedisConnection);
186+
187+
try {
188+
$redis->get('key');
189+
} catch (Exception) {
190+
// Expected
191+
}
192+
}
193+
194+
public function testPipelineCommandStoresConnectionInContext(): void
195+
{
196+
$mockRedisConnection = $this->createMockRedisConnection('pipeline', true);
197+
// Connection will be released via defer() when coroutine ends
198+
$mockRedisConnection->shouldReceive('release')->once();
199+
200+
$redis = $this->createRedis($mockRedisConnection);
201+
202+
$result = $redis->pipeline();
203+
204+
$this->assertTrue($result);
205+
$this->assertSame($mockRedisConnection, Context::get('redis.connection.default'));
206+
}
207+
208+
public function testRegularCommandDoesNotStoreConnectionInContext(): void
209+
{
210+
$mockRedisConnection = $this->createMockRedisConnection();
211+
$mockRedisConnection->shouldReceive('release')->once();
212+
213+
$redis = $this->createRedis($mockRedisConnection);
214+
215+
$redis->get('key');
216+
217+
$this->assertNull(Context::get('redis.connection.default'));
218+
}
219+
220+
/**
221+
* Create a mock Redis connection with configurable behavior.
222+
*/
223+
protected function createMockRedisConnection(
224+
string $command = 'get',
225+
mixed $returnValue = 'value',
226+
?Throwable $exception = null,
227+
?EventDispatcherInterface $eventDispatcher = null
228+
): RedisConnection&Mockery\MockInterface {
229+
$mockPhpRedis = Mockery::mock(PhpRedis::class);
230+
231+
if ($exception !== null) {
232+
$mockPhpRedis->shouldReceive($command)
233+
->andThrow($exception);
234+
} else {
235+
$mockPhpRedis->shouldReceive($command)
236+
->andReturn($returnValue);
237+
}
238+
239+
$mockRedisConnection = Mockery::mock(RedisConnection::class);
240+
$mockRedisConnection->shouldReceive('shouldTransform')->andReturnSelf();
241+
$mockRedisConnection->shouldReceive('getConnection')->andReturn($mockRedisConnection);
242+
$mockRedisConnection->shouldReceive('getEventDispatcher')->andReturn($eventDispatcher);
243+
244+
// Forward the command call to the mock PHP Redis
245+
$mockRedisConnection->shouldReceive($command)
246+
->andReturnUsing(function (...$args) use ($mockPhpRedis, $command) {
247+
return $mockPhpRedis->{$command}(...$args);
248+
});
249+
250+
return $mockRedisConnection;
251+
}
252+
253+
/**
254+
* Create a Redis instance with the given mock connection.
255+
*/
256+
protected function createRedis(RedisConnection $mockConnection): Redis
257+
{
258+
$mockPool = Mockery::mock(RedisPool::class);
259+
$mockPool->shouldReceive('get')->andReturn($mockConnection);
260+
$mockPool->shouldReceive('getOption')->andReturn(Mockery::mock(PoolOption::class));
261+
262+
$mockPoolFactory = Mockery::mock(PoolFactory::class);
263+
$mockPoolFactory->shouldReceive('getPool')->with('default')->andReturn($mockPool);
264+
265+
return new Redis($mockPoolFactory);
266+
}
267+
}

0 commit comments

Comments
 (0)