Merge "clientpool: refactor Redis authentication error handling"

diff --git a/includes/libs/redis/RedisConnRef.php b/includes/libs/redis/RedisConnRef.php
index d330d3c..ede35fa 100644 (file)
--- a/includes/libs/redis/RedisConnRef.php
+++ b/includes/libs/redis/RedisConnRef.php
@@ -42,6 +42,27 @@ class RedisConnRef implements LoggerAwareInterface {
         */
        protected $logger;
 
+       /**
+        * No authentication errors.
+        *
+        * @var constant
+        */
+       const AUTH_NO_ERROR = 200;
+
+       /**
+        * Temporary authentication error; recovered by reauthenticating.
+        *
+        * @var constant
+        */
+       const AUTH_ERROR_TEMPORARY = 201;
+
+       /**
+        * Authentication error was permanent and could not be recovered.
+        *
+        * @var constant
+        */
+       const AUTH_ERROR_PERMANENT = 202;
+
        /**
         * @param RedisConnectionPool $pool
         * @param string $server
@@ -77,41 +98,145 @@ class RedisConnRef implements LoggerAwareInterface {
                $this->lastError = null;
        }
 
+       /**
+        * Magic __call handler for most Redis functions.
+        *
+        * @param string $name
+        * @param array $arguments
+        * @return mixed $res
+        * @throws RedisException
+        */
        public function __call( $name, $arguments ) {
-               $conn = $this->conn; // convenience
-
                // Work around https://github.com/nicolasff/phpredis/issues/70
                $lname = strtolower( $name );
-               if ( ( $lname === 'blpop' || $lname == 'brpop' )
-                       && is_array( $arguments[0] ) && isset( $arguments[1] )
+               if (
+                       ( $lname === 'blpop' || $lname === 'brpop' || $lname === 'brpoplpush' )
+                       && count( $arguments ) > 1
                ) {
-                       $this->pool->resetTimeout( $conn, $arguments[1] + 1 );
-               } elseif ( $lname === 'brpoplpush' && isset( $arguments[2] ) ) {
-                       $this->pool->resetTimeout( $conn, $arguments[2] + 1 );
+                       // Get timeout off the end since it is always required and argument length can vary
+                       $timeout = end( $arguments );
+                       // Only give the additional one second buffer if not requesting an infinite timeout
+                       $this->pool->resetTimeout( $this->conn, ( $timeout > 0 ? $timeout + 1 : $timeout ) );
                }
 
-               $conn->clearLastError();
+               return $this->tryCall( $name, $arguments );
+       }
+
+       /**
+        * Do the method call in the common try catch handler.
+        *
+        * @param string $method
+        * @param array $arguments
+        * @return mixed $res
+        * @throws RedisException
+        */
+       private function tryCall( $method, $arguments ) {
+               $this->conn->clearLastError();
                try {
-                       $res = call_user_func_array( [ $conn, $name ], $arguments );
-                       if ( preg_match( '/^ERR operation not permitted\b/', $conn->getLastError() ) ) {
-                               $this->pool->reauthenticateConnection( $this->server, $conn );
-                               $conn->clearLastError();
-                               $res = call_user_func_array( [ $conn, $name ], $arguments );
-                               $this->logger->info(
-                                       "Used automatic re-authentication for method '$name'.",
-                                       [ 'redis_server' => $this->server ]
-                               );
+                       $res = call_user_func_array( [ $this->conn, $method ], $arguments );
+                       $authError = $this->checkAuthentication();
+                       if ( $authError === self::AUTH_ERROR_TEMPORARY ) {
+                               $res = call_user_func_array( [ $this->conn, $method ], $arguments );
                        }
-               } catch ( RedisException $e ) {
-                       $this->pool->resetTimeout( $conn ); // restore
-                       throw $e;
+                       if ( $authError === self::AUTH_ERROR_PERMANENT ) {
+                               throw new RedisException( "Failure reauthenticating to Redis." );
+                       }
+               } finally {
+                       $this->postCallCleanup();
                }
 
-               $this->lastError = $conn->getLastError() ?: $this->lastError;
+               return $res;
+       }
 
-               $this->pool->resetTimeout( $conn ); // restore
+       /**
+        * Key Scan
+        * Handle this explicity due to needing the iterator passed by reference.
+        * See: https://github.com/phpredis/phpredis#scan
+        *
+        * @param int &$iterator
+        * @param string $pattern
+        * @param int $count
+        * @return array $res
+        */
+       public function scan( &$iterator, $pattern = null, $count = null ) {
+               return $this->tryCall( 'scan', [ &$iterator, $pattern, $count ] );
+       }
 
-               return $res;
+       /**
+        * Set Scan
+        * Handle this explicity due to needing the iterator passed by reference.
+        * See: https://github.com/phpredis/phpredis#sScan
+        *
+        * @param string $key
+        * @param int &$iterator
+        * @param string $pattern
+        * @param int $count
+        * @return array $res
+        */
+       public function sScan( $key, &$iterator, $pattern = null, $count = null ) {
+               return $this->tryCall( 'sScan', [ $key, &$iterator, $pattern, $count ] );
+       }
+
+       /**
+        * Hash Scan
+        * Handle this explicity due to needing the iterator passed by reference.
+        * See: https://github.com/phpredis/phpredis#hScan
+        *
+        * @param string $key
+        * @param int &$iterator
+        * @param string $pattern
+        * @param int $count
+        * @return array $res
+        */
+       public function hScan( $key, &$iterator, $pattern = null, $count = null ) {
+               return $this->tryCall( 'hScan', [ $key, &$iterator, $pattern, $count ] );
+       }
+
+       /**
+        * Sorted Set Scan
+        * Handle this explicity due to needing the iterator passed by reference.
+        * See: https://github.com/phpredis/phpredis#hScan
+        *
+        * @param string $key
+        * @param int &$iterator
+        * @param string $pattern
+        * @param int $count
+        * @return array $res
+        */
+       public function zScan( $key, &$iterator, $pattern = null, $count = null ) {
+               return $this->tryCall( 'zScan', [ $key, &$iterator, $pattern, $count ] );
+       }
+
+       /**
+        * Handle authentication errors and automatically reauthenticate.
+        *
+        * @return constant self::AUTH_NO_ERROR, self::AUTH_ERROR_TEMPORARY, or self::AUTH_ERROR_PERMANENT
+        */
+       private function checkAuthentication() {
+               if ( preg_match( '/^ERR operation not permitted\b/', $this->conn->getLastError() ) ) {
+                       if ( !$this->pool->reauthenticateConnection( $this->server, $this->conn ) ) {
+                               return self::AUTH_ERROR_PERMANENT;
+                       }
+                       $this->conn->clearLastError();
+                       $this->logger->info(
+                               "Used automatic re-authentication for Redis.",
+                               [ 'redis_server' => $this->server ]
+                       );
+                       return self::AUTH_ERROR_TEMPORARY;
+               }
+               return self::AUTH_NO_ERROR;
+       }
+
+       /**
+        * Post Redis call cleanup.
+        *
+        * @return void
+        */
+       private function postCallCleanup() {
+               $this->lastError = $this->conn->getLastError() ?: $this->lastError;
+
+               // Restore original timeout in the case of blocking calls.
+               $this->pool->resetTimeout( $this->conn );
        }
 
        /**