Skip to content

Commit be95659

Browse files
uncaughtuncaught
committed
bug #51578 [Cache] always select database for persistent redis connections
1 parent 184597d commit be95659

File tree

2 files changed

+73
-23
lines changed

2 files changed

+73
-23
lines changed

‎src/Symfony/Component/Cache/Tests/Traits/RedisTraitTest.php

+55
Original file line numberDiff line numberDiff line change
@@ -74,4 +74,59 @@ public static function provideCreateConnection(): array
7474
],
7575
];
7676
}
77+
78+
/**
79+
* Due to a bug in phpredis, the persistent connection will keep its last selected database. So when re-using
80+
* a persistent connection, the database has to be re-selected, too.
81+
*
82+
* @see https://github.com/phpredis/phpredis/issues/1920
83+
* @see https://github.com/symfony/symfony/issues/51578
84+
*
85+
* @group integration
86+
*/
87+
public function testPconnectSelectsCorrectDatabase()
88+
{
89+
if (!class_exists(\Redis::class)) {
90+
throw new SkippedTestSuiteError('The "Redis" class is required.');
91+
}
92+
if (!getenv('REDIS_HOST')) {
93+
throw new SkippedTestSuiteError('REDIS_HOST env var is not defined.');
94+
}
95+
if (!\ini_get('redis.pconnect.pooling_enabled')) {
96+
throw new SkippedTestSuiteError('The bug only occurs when pooling is enabled.');
97+
}
98+
99+
// Limit the connection pool size to 1:
100+
$prevPoolSize = ini_set('redis.pconnect.connection_limit', 1);
101+
if (false === $prevPoolSize) {
102+
throw new SkippedTestSuiteError('Unable to set pool size');
103+
}
104+
105+
try {
106+
$mock = self::getObjectForTrait(RedisTrait::class);
107+
108+
$dsn = 'redis://'.getenv('REDIS_HOST');
109+
110+
$cacheKey = 'testPconnectSelectsCorrectDatabase';
111+
$cacheValueOnDb1 = 'I should only be on database 1';
112+
113+
// First connect to database 1 and set a value there so we can identify this database:
114+
$db1 = $mock::createConnection($dsn, ['dbindex' => 1, 'persistent' => 1]);
115+
self::assertInstanceOf(\Redis::class, $db1);
116+
self::assertSame(1, $db1->getDbNum());
117+
$db1->set($cacheKey, $cacheValueOnDb1);
118+
self::assertSame($cacheValueOnDb1, $db1->get($cacheKey));
119+
120+
// Unset the connection - do not use `close()` or we will lose the persistent connection:
121+
unset($db1);
122+
123+
// Now connect to database 0 and see that we do not actually ended up on database 1 by checking the value:
124+
$db0 = $mock::createConnection($dsn, ['dbindex' => 0, 'persistent' => 1]);
125+
self::assertInstanceOf(\Redis::class, $db0);
126+
self::assertSame(0, $db0->getDbNum()); // Redis is lying here! We could actually be on any database!
127+
self::assertNotSame($cacheValueOnDb1, $db0->get($cacheKey)); // This value should not exist if we are actually on db 0
128+
} finally {
129+
ini_set('redis.pconnect.connection_limit', $prevPoolSize); // Restore previous pool size
130+
}
131+
}
77132
}

‎src/Symfony/Component/Cache/Traits/RedisTrait.php

+18-23
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,10 @@ public static function createConnection(string $dsn, array $options = [])
283283
}
284284

285285
if ((null !== $auth && !$redis->auth($auth))
286-
|| ($params['dbindex'] && !$redis->select($params['dbindex']))
286+
// Due to a bug in phpredis we must always select the dbindex if persistent pooling is enabled
287+
// @see https://github.com/phpredis/phpredis/issues/1920
288+
// @see https://github.com/symfony/symfony/issues/51578
289+
|| (($params['dbindex'] || ('pconnect' === $connect && '0' !== \ini_get('redis.pconnect.pooling_enabled'))) && !$redis->select($params['dbindex']))
287290
) {
288291
$e = preg_replace('/^ERR /', '', $redis->getLastError());
289292
throw new InvalidArgumentException('Redis connection failed: '.$e.'.');
@@ -307,8 +310,10 @@ public static function createConnection(string $dsn, array $options = [])
307310
} elseif (is_a($class, \RedisArray::class, true)) {
308311
foreach ($hosts as $i => $host) {
309312
switch ($host['scheme']) {
310-
case 'tcp': $hosts[$i] = $host['host'].':'.$host['port']; break;
311-
case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port']; break;
313+
case 'tcp': $hosts[$i] = $host['host'].':'.$host['port'];
314+
break;
315+
case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port'];
316+
break;
312317
default: $hosts[$i] = $host['path'];
313318
}
314319
}
@@ -328,8 +333,10 @@ public static function createConnection(string $dsn, array $options = [])
328333
$initializer = static function () use ($class, $params, $hosts) {
329334
foreach ($hosts as $i => $host) {
330335
switch ($host['scheme']) {
331-
case 'tcp': $hosts[$i] = $host['host'].':'.$host['port']; break;
332-
case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port']; break;
336+
case 'tcp': $hosts[$i] = $host['host'].':'.$host['port'];
337+
break;
338+
case 'tls': $hosts[$i] = 'tls://'.$host['host'].':'.$host['port'];
339+
break;
333340
default: $hosts[$i] = $host['path'];
334341
}
335342
}
@@ -344,9 +351,12 @@ public static function createConnection(string $dsn, array $options = [])
344351
$redis->setOption(\Redis::OPT_TCP_KEEPALIVE, $params['tcp_keepalive']);
345352
}
346353
switch ($params['failover']) {
347-
case 'error': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_ERROR); break;
348-
case 'distribute': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE); break;
349-
case 'slaves': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE_SLAVES); break;
354+
case 'error': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_ERROR);
355+
break;
356+
case 'distribute': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE);
357+
break;
358+
case 'slaves': $redis->setOption(\RedisCluster::OPT_SLAVE_FAILOVER, \RedisCluster::FAILOVER_DISTRIBUTE_SLAVES);
359+
break;
350360
}
351361

352362
return $redis;
@@ -403,9 +413,6 @@ public static function createConnection(string $dsn, array $options = [])
403413
return $redis;
404414
}
405415

406-
/**
407-
* {@inheritdoc}
408-
*/
409416
protected function doFetch(array $ids)
410417
{
411418
if (!$ids) {
@@ -439,17 +446,11 @@ protected function doFetch(array $ids)
439446
return $result;
440447
}
441448

442-
/**
443-
* {@inheritdoc}
444-
*/
445449
protected function doHave(string $id)
446450
{
447451
return (bool) $this->redis->exists($id);
448452
}
449453

450-
/**
451-
* {@inheritdoc}
452-
*/
453454
protected function doClear(string $namespace)
454455
{
455456
if ($this->redis instanceof \Predis\ClientInterface) {
@@ -511,9 +512,6 @@ protected function doClear(string $namespace)
511512
return $cleared;
512513
}
513514

514-
/**
515-
* {@inheritdoc}
516-
*/
517515
protected function doDelete(array $ids)
518516
{
519517
if (!$ids) {
@@ -548,9 +546,6 @@ protected function doDelete(array $ids)
548546
return true;
549547
}
550548

551-
/**
552-
* {@inheritdoc}
553-
*/
554549
protected function doSave(array $values, int $lifetime)
555550
{
556551
if (!$values = $this->marshaller->marshall($values, $failed)) {

0 commit comments

Comments
 (0)