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

uncaught · nicolas-grekas · commit ce4f815abf8e · 2024-04-12T13:35:21.000+02:00
diff --git a/src/Symfony/Component/Cache/Tests/Traits/RedisTraitTest.php b/src/Symfony/Component/Cache/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);
+        }
+    }
 }
diff --git a/src/Symfony/Component/Cache/Traits/RedisTrait.php b/src/Symfony/Component/Cache/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)) {

Original file line number	Diff line number	Diff line change
`@@ -283,7 +283,10 @@ public static function createConnection(string $dsn, array $options = [])`
`283`	`283`	`}`
`284`	`284`
`285`	`285`	`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']))`
`287`	`290`	`) {`
`288`	`291`	`$e = preg_replace('/^ERR /', '', $redis->getLastError());`
`289`	`292`	`throw new InvalidArgumentException('Redis connection failed: '.$e.'.');`
`@@ -403,9 +406,6 @@ public static function createConnection(string $dsn, array $options = [])`
`403`	`406`	`return $redis;`
`404`	`407`	`}`
`405`	`408`
`406`		`- /**`
`407`		`- * {@inheritdoc}`
`408`		`- */`
`409`	`409`	`protected function doFetch(array $ids)`
`410`	`410`	`{`
`411`	`411`	`if (!$ids) {`
`@@ -439,17 +439,11 @@ protected function doFetch(array $ids)`
`439`	`439`	`return $result;`
`440`	`440`	`}`
`441`	`441`
`442`		`- /**`
`443`		`- * {@inheritdoc}`
`444`		`- */`
`445`	`442`	`protected function doHave(string $id)`
`446`	`443`	`{`
`447`	`444`	`return (bool) $this->redis->exists($id);`
`448`	`445`	`}`
`449`	`446`
`450`		`- /**`
`451`		`- * {@inheritdoc}`
`452`		`- */`
`453`	`447`	`protected function doClear(string $namespace)`
`454`	`448`	`{`
`455`	`449`	`if ($this->redis instanceof \Predis\ClientInterface) {`
`@@ -511,9 +505,6 @@ protected function doClear(string $namespace)`
`511`	`505`	`return $cleared;`
`512`	`506`	`}`
`513`	`507`
`514`		`- /**`
`515`		`- * {@inheritdoc}`
`516`		`- */`
`517`	`508`	`protected function doDelete(array $ids)`
`518`	`509`	`{`
`519`	`510`	`if (!$ids) {`
`@@ -548,9 +539,6 @@ protected function doDelete(array $ids)`
`548`	`539`	`return true;`
`549`	`540`	`}`
`550`	`541`
`551`		`- /**`
`552`		`- * {@inheritdoc}`
`553`		`- */`
`554`	`542`	`protected function doSave(array $values, int $lifetime)`
`555`	`543`	`{`
`556`	`544`	`if (!$values = $this->marshaller->marshall($values, $failed)) {`