Skip to content

[Cache] Always select database for persistent redis connections #54579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 12, 2024
Merged

[Cache] Always select database for persistent redis connections #54579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/Symfony/Component/Cache/Tests/Traits/RedisTraitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,57 @@ public static function provideCreateConnection(): array
],
];
}

/**
* Due to a bug in phpredis, the persistent connection will keep its last selected database. So when re-using
* a persistent connection, the database has to be re-selected, too.
*
* @see https://github.com/phpredis/phpredis/issues/1920
*
* @group integration
*/
public function testPconnectSelectsCorrectDatabase()
{
if (!class_exists(\Redis::class)) {
throw new SkippedTestSuiteError('The "Redis" class is required.');
}
if (!getenv('REDIS_HOST')) {
throw new SkippedTestSuiteError('REDIS_HOST env var is not defined.');
}
if (!\ini_get('redis.pconnect.pooling_enabled')) {
throw new SkippedTestSuiteError('The bug only occurs when pooling is enabled.');
}

// Limit the connection pool size to 1:
if (false === $prevPoolSize = ini_set('redis.pconnect.connection_limit', 1)) {
throw new SkippedTestSuiteError('Unable to set pool size');
}

try {
$mock = self::getObjectForTrait(RedisTrait::class);

$dsn = 'redis://'.getenv('REDIS_HOST');

$cacheKey = 'testPconnectSelectsCorrectDatabase';
$cacheValueOnDb1 = 'I should only be on database 1';

// First connect to database 1 and set a value there so we can identify this database:
$db1 = $mock::createConnection($dsn, ['dbindex' => 1, 'persistent' => 1]);
self::assertInstanceOf(\Redis::class, $db1);
self::assertSame(1, $db1->getDbNum());
$db1->set($cacheKey, $cacheValueOnDb1);
self::assertSame($cacheValueOnDb1, $db1->get($cacheKey));

// Unset the connection - do not use `close()` or we will lose the persistent connection:
unset($db1);

// Now connect to database 0 and see that we do not actually ended up on database 1 by checking the value:
$db0 = $mock::createConnection($dsn, ['dbindex' => 0, 'persistent' => 1]);
self::assertInstanceOf(\Redis::class, $db0);
self::assertSame(0, $db0->getDbNum()); // Redis is lying here! We could actually be on any database!
self::assertNotSame($cacheValueOnDb1, $db0->get($cacheKey)); // This value should not exist if we are actually on db 0
} finally {
ini_set('redis.pconnect.connection_limit', $prevPoolSize);
}
}
}
20 changes: 4 additions & 16 deletions src/Symfony/Component/Cache/Traits/RedisTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,10 @@ public static function createConnection(string $dsn, array $options = [])
}

if ((null !== $auth && !$redis->auth($auth))
|| ($params['dbindex'] && !$redis->select($params['dbindex']))
// Due to a bug in phpredis we must always select the dbindex if persistent pooling is enabled
// @see https://github.com/phpredis/phpredis/issues/1920
// @see https://github.com/symfony/symfony/issues/51578
|| (($params['dbindex'] || ('pconnect' === $connect && '0' !== \ini_get('redis.pconnect.pooling_enabled'))) && !$redis->select($params['dbindex']))
) {
$e = preg_replace('/^ERR /', '', $redis->getLastError());
throw new InvalidArgumentException('Redis connection failed: '.$e.'.');
Expand Down Expand Up @@ -403,9 +406,6 @@ public static function createConnection(string $dsn, array $options = [])
return $redis;
}

/**
* {@inheritdoc}
*/
protected function doFetch(array $ids)
{
if (!$ids) {
Expand Down Expand Up @@ -439,17 +439,11 @@ protected function doFetch(array $ids)
return $result;
}

/**
* {@inheritdoc}
*/
protected function doHave(string $id)
{
return (bool) $this->redis->exists($id);
}

/**
* {@inheritdoc}
*/
protected function doClear(string $namespace)
{
if ($this->redis instanceof \Predis\ClientInterface) {
Expand Down Expand Up @@ -511,9 +505,6 @@ protected function doClear(string $namespace)
return $cleared;
}

/**
* {@inheritdoc}
*/
protected function doDelete(array $ids)
{
if (!$ids) {
Expand Down Expand Up @@ -548,9 +539,6 @@ protected function doDelete(array $ids)
return true;
}

/**
* {@inheritdoc}
*/
protected function doSave(array $values, int $lifetime)
{
if (!$values = $this->marshaller->marshall($values, $failed)) {
Expand Down
Loading