From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sun, 18 Oct 2015 22:57:42 +0000 (-0700)
Subject: Add WRITE_SYNC flag to BagOStuff::set()/merge()
X-Git-Tag: 1.31.0-rc.0~9302
X-Git-Url: http://git.cyclocoop.org/url?a=commitdiff_plain;h=5c8ef13306bcad6f07232beccf2c816cd3dcc6f1;p=lhc%2Fweb%2Fwiklou.git

Add WRITE_SYNC flag to BagOStuff::set()/merge()

* This blocks on writing to all replicas
  and returns false if any failed.
* This is useful if ChronologyProtector is to work across
  domains by having the writes go everywhere so that later
  reads will see them (and be local at the same time).
* Redundant doc comments were also removed.

Change-Id: I9ed098d563c64dba605e7809bc96731da3b3e79d
---

diff --git a/includes/libs/objectcache/APCBagOStuff.php b/includes/libs/objectcache/APCBagOStuff.php
index 522c5d7196..b5419b75ee 100644
--- a/includes/libs/objectcache/APCBagOStuff.php
+++ b/includes/libs/objectcache/APCBagOStuff.php
@@ -40,7 +40,7 @@ class APCBagOStuff extends BagOStuff {
 		return $val;
 	}
 
-	public function set( $key, $value, $exptime = 0 ) {
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
 		apc_store( $key . self::KEY_SUFFIX, $value, $exptime );
 
 		return true;
diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 95c1bc76c7..6089aaa667 100644
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -67,6 +67,8 @@ abstract class BagOStuff implements LoggerAwareInterface {
 	/** Bitfield constants for get()/getMulti() */
 	const READ_LATEST = 1; // use latest data for replicated stores
 	const READ_VERIFIED = 2; // promise that caller can tell when keys are stale
+	/** Bitfield constants for set()/merge() */
+	const WRITE_SYNC = 1; // synchronously write to all locations for replicated stores
 
 	public function __construct( array $params = array() ) {
 		if ( isset( $params['logger'] ) ) {
@@ -158,6 +160,7 @@ abstract class BagOStuff implements LoggerAwareInterface {
 	 * @param mixed $casToken
 	 * @param integer $flags Bitfield of BagOStuff::READ_* constants [optional]
 	 * @return mixed Returns false on failure and if the item does not exist
+	 * @throws Exception
 	 */
 	protected function getWithToken( $key, &$casToken, $flags = 0 ) {
 		throw new Exception( __METHOD__ . ' not implemented.' );
@@ -169,9 +172,10 @@ abstract class BagOStuff implements LoggerAwareInterface {
 	 * @param string $key
 	 * @param mixed $value
 	 * @param int $exptime Either an interval in seconds or a unix timestamp for expiry
+	 * @param int $flags Bitfield of BagOStuff::WRITE_* constants
 	 * @return bool Success
 	 */
-	abstract public function set( $key, $value, $exptime = 0 );
+	abstract public function set( $key, $value, $exptime = 0, $flags = 0 );
 
 	/**
 	 * Delete an item
@@ -191,15 +195,16 @@ abstract class BagOStuff implements LoggerAwareInterface {
 	 * @param callable $callback Callback method to be executed
 	 * @param int $exptime Either an interval in seconds or a unix timestamp for expiry
 	 * @param int $attempts The amount of times to attempt a merge in case of failure
+	 * @param int $flags Bitfield of BagOStuff::WRITE_* constants
 	 * @return bool Success
 	 * @throws InvalidArgumentException
 	 */
-	public function merge( $key, $callback, $exptime = 0, $attempts = 10 ) {
+	public function merge( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
 		if ( !is_callable( $callback ) ) {
 			throw new InvalidArgumentException( "Got invalid callback." );
 		}
 
-		return $this->mergeViaLock( $key, $callback, $exptime, $attempts );
+		return $this->mergeViaLock( $key, $callback, $exptime, $attempts, $flags );
 	}
 
 	/**
@@ -262,9 +267,10 @@ abstract class BagOStuff implements LoggerAwareInterface {
 	 * @param callable $callback Callback method to be executed
 	 * @param int $exptime Either an interval in seconds or a unix timestamp for expiry
 	 * @param int $attempts The amount of times to attempt a merge in case of failure
+	 * @param int $flags Bitfield of BagOStuff::WRITE_* constants
 	 * @return bool Success
 	 */
-	protected function mergeViaLock( $key, $callback, $exptime = 0, $attempts = 10 ) {
+	protected function mergeViaLock( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
 		if ( !$this->lock( $key, 6 ) ) {
 			return false;
 		}
@@ -279,7 +285,7 @@ abstract class BagOStuff implements LoggerAwareInterface {
 			if ( $value === false ) {
 				$success = true; // do nothing
 			} else {
-				$success = $this->set( $key, $value, $exptime ); // set the new value
+				$success = $this->set( $key, $value, $exptime, $flags ); // set the new value
 			}
 		}
 
diff --git a/includes/libs/objectcache/EmptyBagOStuff.php b/includes/libs/objectcache/EmptyBagOStuff.php
index bef04569b1..4321b25ffb 100644
--- a/includes/libs/objectcache/EmptyBagOStuff.php
+++ b/includes/libs/objectcache/EmptyBagOStuff.php
@@ -31,7 +31,7 @@ class EmptyBagOStuff extends BagOStuff {
 		return false;
 	}
 
-	public function set( $key, $value, $exp = 0 ) {
+	public function set( $key, $value, $exp = 0, $flags = 0 ) {
 		return true;
 	}
 
@@ -39,7 +39,7 @@ class EmptyBagOStuff extends BagOStuff {
 		return true;
 	}
 
-	public function merge( $key, $callback, $exptime = 0, $attempts = 10 ) {
+	public function merge( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
 		return true; // faster
 	}
 }
diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php
index d4044e9f16..94bc95fe39 100644
--- a/includes/libs/objectcache/HashBagOStuff.php
+++ b/includes/libs/objectcache/HashBagOStuff.php
@@ -60,7 +60,7 @@ class HashBagOStuff extends BagOStuff {
 		return $this->bag[$key][0];
 	}
 
-	public function set( $key, $value, $exptime = 0 ) {
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
 		$this->bag[$key] = array( $value, $this->convertExpiry( $exptime ) );
 		return true;
 	}
diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php
index b98c982e6f..40ac1bb3a7 100644
--- a/includes/libs/objectcache/ReplicatedBagOStuff.php
+++ b/includes/libs/objectcache/ReplicatedBagOStuff.php
@@ -84,8 +84,8 @@ class ReplicatedBagOStuff extends BagOStuff {
 			: $this->readStore->getMulti( $keys, $flags );
 	}
 
-	public function set( $key, $value, $exptime = 0 ) {
-		return $this->writeStore->set( $key, $value, $exptime );
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
+		return $this->writeStore->set( $key, $value, $exptime, $flags );
 	}
 
 	public function delete( $key ) {
@@ -112,8 +112,8 @@ class ReplicatedBagOStuff extends BagOStuff {
 		return $this->writeStore->unlock( $key );
 	}
 
-	public function merge( $key, $callback, $exptime = 0, $attempts = 10 ) {
-		return $this->writeStore->merge( $key, $callback, $exptime, $attempts );
+	public function merge( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
+		return $this->writeStore->merge( $key, $callback, $exptime, $attempts, $flags );
 	}
 
 	public function getLastError() {
diff --git a/includes/libs/objectcache/WinCacheBagOStuff.php b/includes/libs/objectcache/WinCacheBagOStuff.php
index 592565ff83..8b0161090c 100644
--- a/includes/libs/objectcache/WinCacheBagOStuff.php
+++ b/includes/libs/objectcache/WinCacheBagOStuff.php
@@ -46,7 +46,7 @@ class WinCacheBagOStuff extends BagOStuff {
 		return $val;
 	}
 
-	public function set( $key, $value, $expire = 0 ) {
+	public function set( $key, $value, $expire = 0, $flags = 0 ) {
 		$result = wincache_ucache_set( $key, serialize( $value ), $expire );
 
 		/* wincache_ucache_set returns an empty array on success if $value
@@ -64,7 +64,7 @@ class WinCacheBagOStuff extends BagOStuff {
 		return true;
 	}
 
-	public function merge( $key, $callback, $exptime = 0, $attempts = 10 ) {
+	public function merge( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
 		if ( !is_callable( $callback ) ) {
 			throw new Exception( "Got invalid callback." );
 		}
diff --git a/includes/libs/objectcache/XCacheBagOStuff.php b/includes/libs/objectcache/XCacheBagOStuff.php
index dc34f557a3..47c29064b1 100644
--- a/includes/libs/objectcache/XCacheBagOStuff.php
+++ b/includes/libs/objectcache/XCacheBagOStuff.php
@@ -44,7 +44,7 @@ class XCacheBagOStuff extends BagOStuff {
 		return $val;
 	}
 
-	public function set( $key, $value, $expire = 0 ) {
+	public function set( $key, $value, $expire = 0, $flags = 0 ) {
 		if ( !$this->isInteger( $value ) ) {
 			$value = serialize( $value );
 		}
diff --git a/includes/objectcache/MemcachedBagOStuff.php b/includes/objectcache/MemcachedBagOStuff.php
index 412f017302..95f5c8d55f 100644
--- a/includes/objectcache/MemcachedBagOStuff.php
+++ b/includes/objectcache/MemcachedBagOStuff.php
@@ -27,6 +27,7 @@
  * @ingroup Cache
  */
 class MemcachedBagOStuff extends BagOStuff {
+	/** @var MWMemcached|Memcached */
 	protected $client;
 
 	/**
@@ -67,49 +68,26 @@ class MemcachedBagOStuff extends BagOStuff {
 		return $this->client->get( $this->encodeKey( $key ), $casToken );
 	}
 
-	/**
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
-	public function set( $key, $value, $exptime = 0 ) {
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
 		return $this->client->set( $this->encodeKey( $key ), $value,
 			$this->fixExpiry( $exptime ) );
 	}
 
-	/**
-	 * @param mixed $casToken
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
 	protected function cas( $casToken, $key, $value, $exptime = 0 ) {
 		return $this->client->cas( $casToken, $this->encodeKey( $key ),
 			$value, $this->fixExpiry( $exptime ) );
 	}
 
-	/**
-	 * @param string $key
-	 * @return bool
-	 */
 	public function delete( $key ) {
 		return $this->client->delete( $this->encodeKey( $key ) );
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $value
-	 * @param int $exptime (default 0)
-	 * @return mixed
-	 */
 	public function add( $key, $value, $exptime = 0 ) {
 		return $this->client->add( $this->encodeKey( $key ), $value,
 			$this->fixExpiry( $exptime ) );
 	}
 
-	public function merge( $key, $callback, $exptime = 0, $attempts = 10 ) {
+	public function merge( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
 		if ( !is_callable( $callback ) ) {
 			throw new Exception( "Got invalid callback." );
 		}
diff --git a/includes/objectcache/MemcachedPeclBagOStuff.php b/includes/objectcache/MemcachedPeclBagOStuff.php
index a7b48a2a2e..365236d883 100644
--- a/includes/objectcache/MemcachedPeclBagOStuff.php
+++ b/includes/objectcache/MemcachedPeclBagOStuff.php
@@ -122,33 +122,16 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
 		return $result;
 	}
 
-	/**
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
-	public function set( $key, $value, $exptime = 0 ) {
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
 		$this->debugLog( "set($key)" );
 		return $this->checkResult( $key, parent::set( $key, $value, $exptime ) );
 	}
 
-	/**
-	 * @param float $casToken
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
 	protected function cas( $casToken, $key, $value, $exptime = 0 ) {
 		$this->debugLog( "cas($key)" );
 		return $this->checkResult( $key, parent::cas( $casToken, $key, $value, $exptime ) );
 	}
 
-	/**
-	 * @param string $key
-	 * @return bool
-	 */
 	public function delete( $key ) {
 		$this->debugLog( "delete($key)" );
 		$result = parent::delete( $key );
@@ -160,33 +143,17 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
 		}
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $value
-	 * @param int $exptime
-	 * @return mixed
-	 */
 	public function add( $key, $value, $exptime = 0 ) {
 		$this->debugLog( "add($key)" );
 		return $this->checkResult( $key, parent::add( $key, $value, $exptime ) );
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $value
-	 * @return mixed
-	 */
 	public function incr( $key, $value = 1 ) {
 		$this->debugLog( "incr($key)" );
 		$result = $this->client->increment( $key, $value );
 		return $this->checkResult( $key, $result );
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $value
-	 * @return mixed
-	 */
 	public function decr( $key, $value = 1 ) {
 		$this->debugLog( "decr($key)" );
 		$result = $this->client->decrement( $key, $value );
diff --git a/includes/objectcache/MultiWriteBagOStuff.php b/includes/objectcache/MultiWriteBagOStuff.php
index 98380d9e21..73bdabd1c2 100644
--- a/includes/objectcache/MultiWriteBagOStuff.php
+++ b/includes/objectcache/MultiWriteBagOStuff.php
@@ -98,11 +98,10 @@ class MultiWriteBagOStuff extends BagOStuff {
 		);
 	}
 
-	/**
-	 * @param bool $debug
-	 */
 	public function setDebug( $debug ) {
-		$this->doWrite( self::ALL, 'setDebug', $debug );
+		foreach ( $this->caches as $cache ) {
+			$cache->setDebug( $debug );
+		}
 	}
 
 	protected function doGet( $key, $flags = 0 ) {
@@ -127,75 +126,43 @@ class MultiWriteBagOStuff extends BagOStuff {
 			&& $misses > 0
 			&& ( $flags & self::READ_VERIFIED ) == self::READ_VERIFIED
 		) {
-			$this->doWrite( $misses, 'set', $key, $value, self::UPGRADE_TTL );
+			$this->doWrite( $misses, $this->asyncWrites, 'set', $key, $value, self::UPGRADE_TTL );
 		}
 
 		return $value;
 	}
 
-	/**
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
-	public function set( $key, $value, $exptime = 0 ) {
-		return $this->doWrite( self::ALL, 'set', $key, $value, $exptime );
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
+		$asyncWrites = ( ( $flags & self::WRITE_SYNC ) == self::WRITE_SYNC )
+			? false
+			: $this->asyncWrites;
+
+		return $this->doWrite( self::ALL, $asyncWrites, 'set', $key, $value, $exptime );
 	}
 
-	/**
-	 * @param string $key
-	 * @return bool
-	 */
 	public function delete( $key ) {
-		return $this->doWrite( self::ALL, 'delete', $key );
+		return $this->doWrite( self::ALL, $this->asyncWrites, 'delete', $key );
 	}
 
-	/**
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
 	public function add( $key, $value, $exptime = 0 ) {
-		return $this->doWrite( self::ALL, 'add', $key, $value, $exptime );
+		return $this->doWrite( self::ALL, $this->asyncWrites, 'add', $key, $value, $exptime );
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $value
-	 * @return bool|null
-	 */
 	public function incr( $key, $value = 1 ) {
-		return $this->doWrite( self::ALL, 'incr', $key, $value );
+		return $this->doWrite( self::ALL, $this->asyncWrites, 'incr', $key, $value );
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $value
-	 * @return bool
-	 */
 	public function decr( $key, $value = 1 ) {
-		return $this->doWrite( self::ALL, 'decr', $key, $value );
+		return $this->doWrite( self::ALL, $this->asyncWrites, 'decr', $key, $value );
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $timeout
-	 * @param int $expiry
-	 * @param string $rclass
-	 * @return bool
-	 */
 	public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) {
-		// Lock only the first cache, to avoid deadlocks
+		// Only need to lock the first cache; also avoids deadlocks
 		return $this->caches[0]->lock( $key, $timeout, $expiry, $rclass );
 	}
 
-	/**
-	 * @param string $key
-	 * @return bool
-	 */
 	public function unlock( $key ) {
+		// Only the first cache is locked
 		return $this->caches[0]->unlock( $key );
 	}
 
@@ -211,20 +178,21 @@ class MultiWriteBagOStuff extends BagOStuff {
 	 * Apply a write method to the first $count backing caches
 	 *
 	 * @param integer $count
+	 * @param bool $asyncWrites
 	 * @param string $method
 	 * @param mixed ...
 	 * @return bool
 	 */
-	protected function doWrite( $count, $method /*, ... */ ) {
+	protected function doWrite( $count, $asyncWrites, $method /*, ... */ ) {
 		$ret = true;
-		$args = array_slice( func_get_args(), 2 );
+		$args = array_slice( func_get_args(), 3 );
 
 		foreach ( $this->caches as $i => $cache ) {
 			if ( $i >= $count ) {
 				break; // ignore the lower tiers
 			}
 
-			if ( $i == 0 || !$this->asyncWrites ) {
+			if ( $i == 0 || !$asyncWrites ) {
 				// First store or in sync mode: write now and get result
 				if ( !call_user_func_array( array( $cache, $method ), $args ) ) {
 					$ret = false;
diff --git a/includes/objectcache/RedisBagOStuff.php b/includes/objectcache/RedisBagOStuff.php
index e6b3f9eb2f..2c12fdf099 100644
--- a/includes/objectcache/RedisBagOStuff.php
+++ b/includes/objectcache/RedisBagOStuff.php
@@ -102,7 +102,7 @@ class RedisBagOStuff extends BagOStuff {
 		return $result;
 	}
 
-	public function set( $key, $value, $expiry = 0 ) {
+	public function set( $key, $value, $expiry = 0, $flags = 0 ) {
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
 			return false;
diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php
index c2e5bd760c..1f384c7d07 100644
--- a/includes/objectcache/SqlBagOStuff.php
+++ b/includes/objectcache/SqlBagOStuff.php
@@ -289,11 +289,6 @@ class SqlBagOStuff extends BagOStuff {
 		return $values;
 	}
 
-	/**
-	 * @param array $data
-	 * @param int $expiry
-	 * @return bool
-	 */
 	public function setMulti( array $data, $expiry = 0 ) {
 		$keysByTable = array();
 		foreach ( $data as $key => $value ) {
@@ -353,23 +348,10 @@ class SqlBagOStuff extends BagOStuff {
 		return $result;
 	}
 
-	/**
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
-	public function set( $key, $value, $exptime = 0 ) {
+	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
 		return $this->setMulti( array( $key => $value ), $exptime );
 	}
 
-	/**
-	 * @param mixed $casToken
-	 * @param string $key
-	 * @param mixed $value
-	 * @param int $exptime
-	 * @return bool
-	 */
 	protected function cas( $casToken, $key, $value, $exptime = 0 ) {
 		list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
 		try {
@@ -410,10 +392,6 @@ class SqlBagOStuff extends BagOStuff {
 		return (bool)$db->affectedRows();
 	}
 
-	/**
-	 * @param string $key
-	 * @return bool
-	 */
 	public function delete( $key ) {
 		list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
 		try {
@@ -430,11 +408,6 @@ class SqlBagOStuff extends BagOStuff {
 		return true;
 	}
 
-	/**
-	 * @param string $key
-	 * @param int $step
-	 * @return int|null
-	 */
 	public function incr( $key, $step = 1 ) {
 		list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
 		try {
@@ -479,7 +452,7 @@ class SqlBagOStuff extends BagOStuff {
 		return $newValue;
 	}
 
-	public function merge( $key, $callback, $exptime = 0, $attempts = 10 ) {
+	public function merge( $key, $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
 		if ( !is_callable( $callback ) ) {
 			throw new Exception( "Got invalid callback." );
 		}
diff --git a/tests/phpunit/includes/objectcache/MultiWriteBagOStuffTest.php b/tests/phpunit/includes/objectcache/MultiWriteBagOStuffTest.php
index ac52e1256f..1d8f43a0e0 100644
--- a/tests/phpunit/includes/objectcache/MultiWriteBagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/MultiWriteBagOStuffTest.php
@@ -34,6 +34,41 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase {
 		$this->assertEquals( $value, $this->cache2->get( $key ), 'Written to tier 2' );
 	}
 
+	public function testSyncMerge() {
+		$key = wfRandomString();
+		$value = wfRandomString();
+		$func = function () use ( $value ) {
+			return $value;
+		};
+
+		// XXX: DeferredUpdates bound to transactions in CLI mode
+		$dbw = wfGetDB( DB_MASTER );
+		$dbw->begin();
+		$this->cache->merge( $key, $func );
+
+		// Set in tier 1
+		$this->assertEquals( $value, $this->cache1->get( $key ), 'Written to tier 1' );
+		// Not yet set in tier 2
+		$this->assertEquals( false, $this->cache2->get( $key ), 'Not written to tier 2' );
+
+		$dbw->commit();
+
+		// Set in tier 2
+		$this->assertEquals( $value, $this->cache2->get( $key ), 'Written to tier 2' );
+
+		$key = wfRandomString();
+
+		$dbw->begin();
+		$this->cache->merge( $key, $func, 0, 1, BagOStuff::WRITE_SYNC );
+
+		// Set in tier 1
+		$this->assertEquals( $value, $this->cache1->get( $key ), 'Written to tier 1' );
+		// Also set in tier 2
+		$this->assertEquals( $value, $this->cache2->get( $key ), 'Written to tier 2' );
+
+		$dbw->commit();
+	}
+
 	public function testSetDelayed() {
 		$key = wfRandomString();
 		$value = wfRandomString();