From: Aaron Schulz <aschulz@wikimedia.org>
Date: Thu, 21 Nov 2013 18:22:48 +0000 (-0800)
Subject: Fixes to RedisBagOStuff
X-Git-Tag: 1.31.0-rc.0~17806^2
X-Git-Url: http://git.cyclocoop.org/%24image?a=commitdiff_plain;h=2ceda41c5783ddc3cfb113db53aee8dbc5a7aa56;p=lhc%2Fweb%2Fwiklou.git

Fixes to RedisBagOStuff

* Avoid serializing scalars like the memcached client does.
  This means incr/decr can be fast and preserve the TTL.
* Properly call unwatch() in the cas() method.
* Made the expiry set on add() actually atomic.
* Also cleaned up the profiling calls.

bug: 56069
Change-Id: I3e0d1c4888062544c54aef32085f8ce608ff5423
---

diff --git a/includes/objectcache/RedisBagOStuff.php b/includes/objectcache/RedisBagOStuff.php
index 135e0e8302..adcf762068 100644
--- a/includes/objectcache/RedisBagOStuff.php
+++ b/includes/objectcache/RedisBagOStuff.php
@@ -55,7 +55,7 @@ class RedisBagOStuff extends BagOStuff {
 	 *     flap, for example if it is in swap death.
 	 */
 	function __construct( $params ) {
-		$redisConf = array( 'serializer' => 'php' );
+		$redisConf = array( 'serializer' => 'none' ); // manage that in this class
 		foreach ( array( 'connectTimeout', 'persistent', 'password' ) as $opt ) {
 			if ( isset( $params[$opt] ) ) {
 				$redisConf[$opt] = $params[$opt];
@@ -72,38 +72,38 @@ class RedisBagOStuff extends BagOStuff {
 	}
 
 	public function get( $key, &$casToken = null ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 		try {
-			$result = $conn->get( $key );
+			$result = $this->unserialize( $conn->get( $key ) );
 		} catch ( RedisException $e ) {
 			$result = false;
 			$this->handleException( $server, $conn, $e );
 		}
 		$casToken = $result;
+
 		$this->logRequest( 'get', $key, $server, $result );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
 	public function set( $key, $value, $expiry = 0 ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 		$expiry = $this->convertToRelative( $expiry );
 		try {
-			if ( !$expiry ) {
-				// No expiry, that is very different from zero expiry in Redis
-				$result = $conn->set( $key, $value );
+			if ( $expiry ) {
+				$result = $conn->setex( $key, $expiry, $this->serialize( $value ) );
 			} else {
-				$result = $conn->setex( $key, $expiry, $value );
+				// No expiry, that is very different from zero expiry in Redis
+				$result = $conn->set( $key, $this->serialize( $value ) );
 			}
 		} catch ( RedisException $e ) {
 			$result = false;
@@ -111,15 +111,14 @@ class RedisBagOStuff extends BagOStuff {
 		}
 
 		$this->logRequest( 'set', $key, $server, $result );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
 	public function cas( $casToken, $key, $value, $expiry = 0 ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 		$expiry = $this->convertToRelative( $expiry );
@@ -127,25 +126,18 @@ class RedisBagOStuff extends BagOStuff {
 			$conn->watch( $key );
 
 			if ( $this->get( $key ) !== $casToken ) {
-				wfProfileOut( __METHOD__ );
+				$conn->unwatch();
 				return false;
 			}
 
+			// multi()/exec() will fail atomically if the key changed since watch()
 			$conn->multi();
-
-			if ( !$expiry ) {
-				// No expiry, that is very different from zero expiry in Redis
-				$conn->set( $key, $value );
+			if ( $expiry ) {
+				$conn->setex( $key, $expiry, $this->serialize( $value ) );
 			} else {
-				$conn->setex( $key, $expiry, $value );
+				// No expiry, that is very different from zero expiry in Redis
+				$conn->set( $key, $this->serialize( $value ) );
 			}
-
-			/*
-			 * multi()/exec() (transactional mode) allows multiple values to
-			 * be set/get at once and will return an array of results, in
-			 * the order they were set/get. In this case, we only set 1
-			 * value, which should (in case of success) result in true.
-			 */
 			$result = ( $conn->exec() == array( true ) );
 		} catch ( RedisException $e ) {
 			$result = false;
@@ -153,15 +145,14 @@ class RedisBagOStuff extends BagOStuff {
 		}
 
 		$this->logRequest( 'cas', $key, $server, $result );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
 	public function delete( $key, $time = 0 ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 		try {
@@ -172,13 +163,14 @@ class RedisBagOStuff extends BagOStuff {
 			$result = false;
 			$this->handleException( $server, $conn, $e );
 		}
+
 		$this->logRequest( 'delete', $key, $server, $result );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
 	public function getMulti( array $keys ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		$batches = array();
 		$conns = array();
 		foreach ( $keys as $key ) {
@@ -204,7 +196,7 @@ class RedisBagOStuff extends BagOStuff {
 				}
 				foreach ( $batchResult as $i => $value ) {
 					if ( $value !== false ) {
-						$result[$batchKeys[$i]] = $value;
+						$result[$batchKeys[$i]] = $this->unserialize( $value );
 					}
 				}
 			} catch ( RedisException $e ) {
@@ -214,29 +206,32 @@ class RedisBagOStuff extends BagOStuff {
 
 		$this->debug( "getMulti for " . count( $keys ) . " keys " .
 			"returned " . count( $result ) . " results" );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
 	public function add( $key, $value, $expiry = 0 ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 		$expiry = $this->convertToRelative( $expiry );
 		try {
-			$result = $conn->setnx( $key, $value );
-			if ( $result && $expiry ) {
+			if ( $expiry ) {
+				$conn->multi();
+				$conn->setnx( $key, $this->serialize( $value ) );
 				$conn->expire( $key, $expiry );
+				$result = ( $conn->exec() == array( true, true ) );
+			} else {
+				$result = $conn->setnx( $key, $this->serialize( $value ) );
 			}
 		} catch ( RedisException $e ) {
 			$result = false;
 			$this->handleException( $server, $conn, $e );
 		}
+
 		$this->logRequest( 'add', $key, $server, $result );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
@@ -245,23 +240,22 @@ class RedisBagOStuff extends BagOStuff {
 	 * with WATCH or scripting, but this function is rarely used.
 	 */
 	public function replace( $key, $value, $expiry = 0 ) {
-		wfProfileIn( __METHOD__ );
+		$section = new ProfileSection( __METHOD__ );
+
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 		if ( !$conn->exists( $key ) ) {
-			wfProfileOut( __METHOD__ );
 			return false;
 		}
 
 		$expiry = $this->convertToRelative( $expiry );
 		try {
 			if ( !$expiry ) {
-				$result = $conn->set( $key, $value );
+				$result = $conn->set( $key, $this->serialize( $value ) );
 			} else {
-				$result = $conn->setex( $key, $expiry, $value );
+				$result = $conn->setex( $key, $expiry, $this->serialize( $value ) );
 			}
 		} catch ( RedisException $e ) {
 			$result = false;
@@ -269,10 +263,57 @@ class RedisBagOStuff extends BagOStuff {
 		}
 
 		$this->logRequest( 'replace', $key, $server, $result );
-		wfProfileOut( __METHOD__ );
 		return $result;
 	}
 
+	/**
+	 * Non-atomic implementation of incr().
+	 *
+	 * Probably all callers actually want incr() to atomically initialise
+	 * values to zero if they don't exist, as provided by the Redis INCR
+	 * command. But we are constrained by the memcached-like interface to
+	 * return null in that case. Once the key exists, further increments are
+	 * atomic.
+	 */
+	public function incr( $key, $value = 1 ) {
+		$section = new ProfileSection( __METHOD__ );
+
+		list( $server, $conn ) = $this->getConnection( $key );
+		if ( !$conn ) {
+			return false;
+		}
+		if ( !$conn->exists( $key ) ) {
+			return null;
+		}
+		try {
+			$result = $this->unserialize( $conn->incrBy( $key, $value ) );
+		} catch ( RedisException $e ) {
+			$result = false;
+			$this->handleException( $server, $conn, $e );
+		}
+
+		$this->logRequest( 'incr', $key, $server, $result );
+		return $result;
+	}
+
+	/**
+	 * @param mixed $data
+	 * @return string
+	 */
+	protected function serialize( $data ) {
+		// Ignore digit strings and ints so INCR/DECR work
+		return ( is_int( $data ) || ctype_digit( $data ) ) ? $data : serialize( $data );
+	}
+
+	/**
+	 * @param string $data
+	 * @return mixed
+	 */
+	protected function unserialize( $data ) {
+		// Ignore digit strings and ints so INCR/DECR work
+		return ( is_int( $data ) || ctype_digit( $data ) ) ? $data : unserialize( $data );
+	}
+
 	/**
 	 * Get a Redis object with a connection suitable for fetching the specified key
 	 * @return Array (server, RedisConnRef) or (false, false)