From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 7 Jul 2014 03:36:24 +0000 (-0700)
Subject: Lowered the default OPT_READ_TIMEOUT for Redis
X-Git-Tag: 1.31.0-rc.0~15057^2
X-Git-Url: https://git.cyclocoop.org/%28%28?a=commitdiff_plain;h=e3227eeda60330d9ac28fcd1788bceae23e60b61;p=lhc%2Fweb%2Fwiklou.git

Lowered the default OPT_READ_TIMEOUT for Redis

* This should probably be low for the case of a server that is
  quick to accept connections but slow to do anything.
* Made the client class handle blocking operations sanely
  by automatically boosting/lowering the timeout value.

Change-Id: Idea083b843f7eb558d2daf249deea853c9ec43ae
---

diff --git a/includes/clientpool/RedisConnectionPool.php b/includes/clientpool/RedisConnectionPool.php
index 36d2731cbe..d2c504ae1a 100644
--- a/includes/clientpool/RedisConnectionPool.php
+++ b/includes/clientpool/RedisConnectionPool.php
@@ -101,7 +101,7 @@ class RedisConnectionPool {
 			$options['connectTimeout'] = 1;
 		}
 		if ( !isset( $options['readTimeout'] ) ) {
-			$options['readTimeout'] = 31; // handles up to 30 second blocking commands
+			$options['readTimeout'] = 1;
 		}
 		if ( !isset( $options['persistent'] ) ) {
 			$options['persistent'] = false;
@@ -120,7 +120,7 @@ class RedisConnectionPool {
 	 *                      Optional, default is 1 second.
 	 *   - readTimeout    : The timeout for operation reads, in seconds.
 	 *                      Commands like BLPOP can fail if told to wait longer than this.
-	 *                      Optional, default is 60 seconds.
+	 *                      Optional, default is 1 second.
 	 *   - persistent     : Set this to true to allow connections to persist across
 	 *                      multiple web requests. False by default.
 	 *   - password       : The authentication password, will be sent to Redis in clear text.
@@ -342,6 +342,16 @@ class RedisConnectionPool {
 		return true;
 	}
 
+	/**
+	 * Adjust or reset the connection handle read timeout value
+	 *
+	 * @param Redis $conn
+	 * @param integer $timeout Optional
+	 */
+	public function resetTimeout( Redis $conn, $timeout = null ) {
+		$conn->setOption( Redis::OPT_READ_TIMEOUT, $timeout ?: $this->readTimeout );
+	}
+
 	/**
 	 * Make sure connections are closed for sanity
 	 */
@@ -401,17 +411,34 @@ class RedisConnRef {
 	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] )
+		) {
+			$this->pool->resetTimeout( $conn, $arguments[1] + 1 );
+		} elseif ( $lname === 'brpoplpush' && isset( $arguments[2] ) ) {
+			$this->pool->resetTimeout( $conn, $arguments[2] + 1 );
+		}
+
 		$conn->clearLastError();
-		$res = call_user_func_array( array( $conn, $name ), $arguments );
-		if ( preg_match( '/^ERR operation not permitted\b/', $conn->getLastError() ) ) {
-			$this->pool->reauthenticateConnection( $this->server, $conn );
-			$conn->clearLastError();
+		try {
 			$res = call_user_func_array( array( $conn, $name ), $arguments );
-			wfDebugLog( 'redis', "Used automatic re-authentication for method '$name'." );
+			if ( preg_match( '/^ERR operation not permitted\b/', $conn->getLastError() ) ) {
+				$this->pool->reauthenticateConnection( $this->server, $conn );
+				$conn->clearLastError();
+				$res = call_user_func_array( array( $conn, $name ), $arguments );
+				wfDebugLog( 'redis', "Used automatic re-authentication for method '$name'." );
+			}
+		} catch ( RedisException $e ) {
+			$this->pool->resetTimeout( $conn ); // restore
+			throw $e;
 		}
 
 		$this->lastError = $conn->getLastError() ?: $this->lastError;
 
+		$this->pool->resetTimeout( $conn ); // restore
+
 		return $res;
 	}