Bug 20595 - Fixed incr()/decr() functions for some caches.

* Preserve TTL in objectcache incr() method for APC, XCache and DBA.
  Do not serialize numeric values, since it breaks the counters.
* Also gave some DBABagOStuff functions explicit visibility.

Change-Id: I13856832f418c843afc8e42a6d77686331f6cb41

includes/objectcache/APCBagOStuff.php

@@ -27,7 +27,6 @@
  * @ingroup Cache
  */
 class APCBagOStuff extends BagOStuff {
-
 	/**
 	 * @param $key string
 	 * @return mixed
@@ -36,7 +35,11 @@ class APCBagOStuff extends BagOStuff {
 		$val = apc_fetch( $key );
 
 		if ( is_string( $val ) ) {
-			$val = unserialize( $val );
+			if ( $this->isInteger( $val ) ) {
+				$val = intval( $val );
+			} else {
+				$val = unserialize( $val );
+			}
 		}
 
 		return $val;
@@ -49,7 +52,11 @@ class APCBagOStuff extends BagOStuff {
 	 * @return bool
 	 */
 	public function set( $key, $value, $exptime = 0 ) {
-		apc_store( $key, serialize( $value ), $exptime );
+		if ( !$this->isInteger( $value ) ) {
+			$value = serialize( $value );
+		}
+
+		apc_store( $key, $value, $exptime );
 
 		return true;
 	}
@@ -65,6 +72,14 @@ class APCBagOStuff extends BagOStuff {
 		return true;
 	}
 
+	public function incr( $key, $value = 1 ) {
+		return apc_inc( $key, $value );
+	}
+
+	public function decr( $key, $value = 1 ) {
+		return apc_dec( $key, $value );
+	}
+
 	/**
 	 * @return Array
 	 */
@@ -80,4 +95,3 @@ class APCBagOStuff extends BagOStuff {
 		return $keys;
 	}
 }
-

includes/objectcache/BagOStuff.php

@@ -164,10 +164,11 @@ abstract class BagOStuff {
 	}
 
 	/**
+	 * Increase stored value of $key by $value while preserving its TTL
 	 * @param $key String: Key to increase
 	 * @param $value Integer: Value to add to $key (Default 1)
 	 * @return null if lock is not possible else $key value increased by $value
-	 * @return bool success
+	 * @return integer
 	 */
 	public function incr( $key, $value = 1 ) {
 		if ( !$this->lock( $key ) ) {
@@ -186,9 +187,10 @@ abstract class BagOStuff {
 	}
 
 	/**
+	 * Decrease stored value of $key by $value while preserving its TTL
 	 * @param $key String
 	 * @param $value Integer
-	 * @return bool success
+	 * @return integer
 	 */
 	public function decr( $key, $value = 1 ) {
 		return $this->incr( $key, - $value );
@@ -235,4 +237,14 @@ abstract class BagOStuff {
 			return $exptime;
 		}
 	}
+
+	/**
+	 * Check if a value is an integer
+	 *
+	 * @param $value mixed
+	 * @return bool
+	 */
+	protected function isInteger( $value ) {
+		return ( is_int( $value ) || ctype_digit( $value ) );
+	}
 }

includes/objectcache/DBABagOStuff.php

@@ -45,8 +45,7 @@ class DBABagOStuff extends BagOStuff {
 			$params['dir'] = wfTempDir();
 		}
 
-		$this->mFile = $params['dir']."/mw-cache-" . wfWikiID();
-		$this->mFile .= '.db';
+		$this->mFile = $params['dir'] . '/mw-cache-' . wfWikiID() . '.db';
 		wfDebug( __CLASS__ . ": using cache file {$this->mFile}\n" );
 		$this->mHandler = $wgDBAhandler;
 	}
@@ -58,7 +57,7 @@ class DBABagOStuff extends BagOStuff {
 	 *
 	 * @return string
 	 */
-	function encode( $value, $expiry ) {
+	protected function encode( $value, $expiry ) {
 		# Convert to absolute time
 		$expiry = $this->convertExpiry( $expiry );
 
@@ -69,7 +68,7 @@ class DBABagOStuff extends BagOStuff {
 	 * @param $blob string
 	 * @return array list containing value first and expiry second
 	 */
-	function decode( $blob ) {
+	protected function decode( $blob ) {
 		if ( !is_string( $blob ) ) {
 			return array( null, 0 );
 		} else {
@@ -83,7 +82,7 @@ class DBABagOStuff extends BagOStuff {
 	/**
 	 * @return resource
 	 */
-	function getReader() {
+	protected function getReader() {
 		if ( file_exists( $this->mFile ) ) {
 			$handle = dba_open( $this->mFile, 'rl', $this->mHandler );
 		} else {
@@ -100,7 +99,7 @@ class DBABagOStuff extends BagOStuff {
 	/**
 	 * @return resource
 	 */
-	function getWriter() {
+	protected function getWriter() {
 		$handle = dba_open( $this->mFile, 'cl', $this->mHandler );
 
 		if ( !$handle ) {
@@ -114,7 +113,7 @@ class DBABagOStuff extends BagOStuff {
 	 * @param $key string
 	 * @return mixed|null|string
 	 */
-	function get( $key ) {
+	public function get( $key ) {
 		wfProfileIn( __METHOD__ );
 		wfDebug( __METHOD__ . "($key)\n" );
 
@@ -149,7 +148,7 @@ class DBABagOStuff extends BagOStuff {
 	 * @param $exptime int
 	 * @return bool
 	 */
-	function set( $key, $value, $exptime = 0 ) {
+	public function set( $key, $value, $exptime = 0 ) {
 		wfProfileIn( __METHOD__ );
 		wfDebug( __METHOD__ . "($key)\n" );
 
@@ -173,7 +172,7 @@ class DBABagOStuff extends BagOStuff {
 	 * @param $time int
 	 * @return bool
 	 */
-	function delete( $key, $time = 0 ) {
+	public function delete( $key, $time = 0 ) {
 		wfProfileIn( __METHOD__ );
 		wfDebug( __METHOD__ . "($key)\n" );
 
@@ -196,7 +195,7 @@ class DBABagOStuff extends BagOStuff {
 	 * @param $exptime int
 	 * @return bool
 	 */
-	function add( $key, $value, $exptime = 0 ) {
+	public function add( $key, $value, $exptime = 0 ) {
 		wfProfileIn( __METHOD__ );
 
 		$blob = $this->encode( $value, $exptime );
@@ -214,7 +213,7 @@ class DBABagOStuff extends BagOStuff {
 		if ( !$ret ) {
 			list( $value, $expiry ) = $this->decode( dba_fetch( $key, $handle ) );
 
-			if ( $expiry < time() ) {
+			if ( $expiry && $expiry < time() ) {
 				# Yes expired, delete and try again
 				dba_delete( $key, $handle );
 				$ret = dba_insert( $key, $blob, $handle );
@@ -229,8 +228,43 @@ class DBABagOStuff extends BagOStuff {
 	}
 
 	/**
-	 * @return Array
+	 * @param $key string
+	 * @param $step integer
+	 * @return integer|bool
 	 */
+	public function incr( $key, $step = 1 ) {
+		wfProfileIn( __METHOD__ );
+
+		$handle = $this->getWriter();
+
+		if ( !$handle ) {
+			wfProfileOut( __METHOD__ );
+			return false;
+		}
+
+		list( $value, $expiry ) = $this->decode( dba_fetch( $key, $handle ) );
+		if ( !is_null( $value ) ) {
+			if ( $expiry && $expiry < time() ) {
+				# Key is expired, delete it
+				dba_delete( $key, $handle );
+				wfDebug( __METHOD__ . ": $key expired\n" );
+				$value = null;
+			} else {
+				$value += $step;
+				$blob = $this->encode( $value, $expiry );
+
+				$ret = dba_replace( $key, $blob, $handle );
+				$value = $ret ? $value : null;
+			}
+		}
+
+		dba_close( $handle );
+
+		wfProfileOut( __METHOD__ );
+
+		return is_null( $value ) ? false : (int)$value;
+	}
+
 	function keys() {
 		$reader = $this->getReader();
 		$k1 = dba_firstkey( $reader );
@@ -250,4 +284,3 @@ class DBABagOStuff extends BagOStuff {
 		return $result;
 	}
 }
-

includes/objectcache/XCacheBagOStuff.php

@@ -38,7 +38,11 @@ class XCacheBagOStuff extends BagOStuff {
 		$val = xcache_get( $key );
 
 		if ( is_string( $val ) ) {
-			$val = unserialize( $val );
+			if ( $this->isInteger( $val ) ) {
+				$val = intval( $val );
+			} else {
+				$val = unserialize( $val );
+			}
 		}
 
 		return $val;
@@ -53,7 +57,11 @@ class XCacheBagOStuff extends BagOStuff {
 	 * @return bool
 	 */
 	public function set( $key, $value, $expire = 0 ) {
-		xcache_set( $key, serialize( $value ), $expire );
+		if ( !$this->isInteger( $value ) ) {
+			$value = serialize( $value );
+		}
+
+		xcache_set( $key, $value, $expire );
 		return true;
 	}
 
@@ -68,5 +76,12 @@ class XCacheBagOStuff extends BagOStuff {
 		xcache_unset( $key );
 		return true;
 	}
-}
 
+	public function incr( $key, $value = 1 ) {
+		return xcache_inc( $key, $value );
+	}
+
+	public function decr( $key, $value = 1 ) {
+		return xcache_dec( $key, $value );
+	}
+}