bug #51578 [Cache] always select database for persistent redis connections

Author: uncaught

Tests/Traits/RedisTraitTest.php

@@ -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);
+        }
+    }
 }

Traits/RedisTrait.php

@@ -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.'.');
@@ -403,9 +406,6 @@ public static function createConnection(string $dsn, array $options = [])
         return $redis;
     }
 
-    /**
-     * {@inheritdoc}
-     */
     protected function doFetch(array $ids)
     {
         if (!$ids) {
@@ -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) {
@@ -511,9 +505,6 @@ protected function doClear(string $namespace)
         return $cleared;
     }
 
-    /**
-     * {@inheritdoc}
-     */
     protected function doDelete(array $ids)
     {
         if (!$ids) {
@@ -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)) {