From: Aaron Schulz <aschulz@wikimedia.org>
Date: Thu, 31 May 2018 20:41:48 +0000 (-0700)
Subject: objectcache: make RedisBagOStuff pass all tests
X-Git-Tag: 1.34.0-rc.0~5211^2
X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_aide%28?a=commitdiff_plain;h=13f7232bf4011668afdca27fa60893f4d0be2d31;p=lhc%2Fweb%2Fwiklou.git

objectcache: make RedisBagOStuff pass all tests

* Provide a default lock-based BagOStuff::cas implementation
* Make RedisBagOStuff::merge() use mergeViaCas()
* Use the raw unserialized string as the redis CAS token to
  avoid any bad interaction with __wakeup() methods changing
  field values every time
* Make RedisBagOStuff::incr() return false when there is no
  such key, not null
* Rewrite merge() test to make the order of write/cas phase
  of the parent and child merge() calls well defined instead
  of arbitrary usleep() calls
* Avoid cache key reuse in test runs

Change-Id: I388ec173cf3858bb2fc7a8c8a00cda68703074ce
---

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index 8a88581026..6452c034bb 100644
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -342,7 +342,21 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
 	 * @throws Exception
 	 */
 	protected function cas( $casToken, $key, $value, $exptime = 0 ) {
-		throw new Exception( "CAS is not implemented in " . __CLASS__ );
+		if ( !$this->lock( $key, 0 ) ) {
+			return false; // non-blocking
+		}
+
+		$curCasToken = null; // passed by reference
+		$this->getWithToken( $key, $curCasToken, self::READ_LATEST );
+		if ( $casToken === $curCasToken ) {
+			$success = $this->set( $key, $value, $exptime );
+		} else {
+			$success = false; // mismatched or failed
+		}
+
+		$this->unlock( $key );
+
+		return $success;
 	}
 
 	/**
diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php
index f720010f41..9b0d5956d3 100644
--- a/includes/libs/objectcache/RedisBagOStuff.php
+++ b/includes/libs/objectcache/RedisBagOStuff.php
@@ -91,12 +91,19 @@ class RedisBagOStuff extends BagOStuff {
 	}
 
 	protected function doGet( $key, $flags = 0 ) {
+		$casToken = null;
+
+		return $this->getWithToken( $key, $casToken, $flags );
+	}
+
+	protected function getWithToken( $key, &$casToken, $flags = 0 ) {
 		list( $server, $conn ) = $this->getConnection( $key );
 		if ( !$conn ) {
 			return false;
 		}
 		try {
 			$value = $conn->get( $key );
+			$casToken = $value;
 			$result = $this->unserialize( $value );
 		} catch ( RedisException $e ) {
 			$result = false;
@@ -260,6 +267,10 @@ class RedisBagOStuff extends BagOStuff {
 		return $result;
 	}
 
+	public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
+		return $this->mergeViaCas( $key, $callback, $exptime, $attempts );
+	}
+
 	/**
 	 * Non-atomic implementation of incr().
 	 *
@@ -279,7 +290,7 @@ class RedisBagOStuff extends BagOStuff {
 		}
 		try {
 			if ( !$conn->exists( $key ) ) {
-				return null;
+				return false;
 			}
 			// @FIXME: on races, the key may have a 0 TTL
 			$result = $conn->incrBy( $key, $value );
diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
index 4320d80249..d5702972f4 100644
--- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
@@ -10,6 +10,8 @@ class BagOStuffTest extends MediaWikiTestCase {
 	/** @var BagOStuff */
 	private $cache;
 
+	const TEST_KEY = 'test';
+
 	protected function setUp() {
 		parent::setUp();
 
@@ -23,7 +25,7 @@ class BagOStuffTest extends MediaWikiTestCase {
 			$this->cache = new HashBagOStuff;
 		}
 
-		$this->cache->delete( wfMemcKey( 'test' ) );
+		$this->cache->delete( $this->cache->makeKey( self::TEST_KEY ) );
 	}
 
 	/**
@@ -63,42 +65,34 @@ class BagOStuffTest extends MediaWikiTestCase {
 	/**
 	 * @covers BagOStuff::merge
 	 * @covers BagOStuff::mergeViaLock
+	 * @covers BagOStuff::mergeViaCas
 	 */
 	public function testMerge() {
-		$key = wfMemcKey( 'test' );
-
-		$usleep = 0;
-
-		/**
-		 * Callback method: append "merged" to whatever is in cache.
-		 *
-		 * @param BagOStuff $cache
-		 * @param string $key
-		 * @param int $existingValue
-		 * @use int $usleep
-		 * @return int
-		 */
-		$callback = function ( BagOStuff $cache, $key, $existingValue ) use ( &$usleep ) {
-			// let's pretend this is an expensive callback to test concurrent merge attempts
-			usleep( $usleep );
-
-			if ( $existingValue === false ) {
-				return 'merged';
-			}
-
-			return $existingValue . 'merged';
+		$key = $this->cache->makeKey( self::TEST_KEY );
+		$callback = function ( BagOStuff $cache, $key, $oldVal ) {
+			return ( $oldVal === false ) ? 'merged' : $oldVal . 'merged';
 		};
 
 		// merge on non-existing value
-		$merged = $this->cache->merge( $key, $callback, 0 );
+		$merged = $this->cache->merge( $key, $callback, 5 );
 		$this->assertTrue( $merged );
 		$this->assertEquals( $this->cache->get( $key ), 'merged' );
 
 		// merge on existing value
-		$merged = $this->cache->merge( $key, $callback, 0 );
+		$merged = $this->cache->merge( $key, $callback, 5 );
 		$this->assertTrue( $merged );
 		$this->assertEquals( $this->cache->get( $key ), 'mergedmerged' );
+	}
 
+	/**
+	 * @covers BagOStuff::merge
+	 * @covers BagOStuff::mergeViaLock
+	 */
+	public function testMerge_fork() {
+		$key = $this->cache->makeKey( self::TEST_KEY );
+		$callback = function ( BagOStuff $cache, $key, $oldVal ) {
+			return ( $oldVal === false ) ? 'merged' : $oldVal . 'merged';
+		};
 		/*
 		 * Test concurrent merges by forking this process, if:
 		 * - not manually called with --use-bagostuff
@@ -111,33 +105,39 @@ class BagOStuffTest extends MediaWikiTestCase {
 		$fork &= !$this->cache instanceof EmptyBagOStuff;
 		$fork &= !$this->cache instanceof MultiWriteBagOStuff;
 		if ( $fork ) {
-			// callback should take awhile now so that we can test concurrent merge attempts
-			$pid = pcntl_fork();
+			$pid = null;
+			// Function to start merge(), run another merge() midway through, then finish
+			$outerFunc = function ( BagOStuff $cache, $key, $oldVal ) use ( $callback, &$pid ) {
+				$pid = pcntl_fork();
+				if ( $pid == -1 ) {
+					return false;
+				} elseif ( $pid ) {
+					pcntl_wait( $status );
+
+					return $callback( $cache, $key, $oldVal );
+				} else {
+					$this->cache->merge( $key, $callback, 0, 1 );
+					// Bail out of the outer merge() in the child process since it does not
+					// need to attempt to write anything. Success is checked by the parent.
+					parent::tearDown(); // avoid phpunit notices
+					exit;
+				}
+			};
+
+			// attempt a merge - this should fail
+			$merged = $this->cache->merge( $key, $outerFunc, 0, 1 );
+
 			if ( $pid == -1 ) {
-				// can't fork, ignore this test...
-			} elseif ( $pid ) {
-				// wait a little, making sure that the child process is calling merge
-				usleep( 3000 );
-
-				// attempt a merge - this should fail
-				$merged = $this->cache->merge( $key, $callback, 0, 1 );
-
-				// merge has failed because child process was merging (and we only attempted once)
-				$this->assertFalse( $merged );
-
-				// make sure the child's merge is completed and verify
-				usleep( 3000 );
-				$this->assertEquals( $this->cache->get( $key ), 'mergedmergedmerged' );
-			} else {
-				$this->cache->merge( $key, $callback, 0, 1 );
-
-				// Note: I'm not even going to check if the merge worked, I'll
-				// compare values in the parent process to test if this merge worked.
-				// I'm just going to exit this child process, since I don't want the
-				// child to output any test results (would be rather confusing to
-				// have test output twice)
-				exit;
+				return; // can't fork, ignore this test...
 			}
+
+			// merge has failed because child process was merging (and we only attempted once)
+			$this->assertFalse( $merged );
+
+			// make sure the child's merge is completed and verify
+			$this->assertEquals( $this->cache->get( $key ), 'mergedmerged' );
+		} else {
+			$this->markTestSkipped( 'No pcntl methods available' );
 		}
 	}
 
@@ -145,10 +145,10 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers BagOStuff::changeTTL
 	 */
 	public function testChangeTTL() {
-		$key = wfMemcKey( 'test' );
+		$key = $this->cache->makeKey( self::TEST_KEY );
 		$value = 'meow';
 
-		$this->cache->add( $key, $value );
+		$this->cache->add( $key, $value, 5 );
 		$this->assertTrue( $this->cache->changeTTL( $key, 5 ) );
 		$this->assertEquals( $this->cache->get( $key ), $value );
 		$this->cache->delete( $key );
@@ -159,8 +159,8 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers BagOStuff::add
 	 */
 	public function testAdd() {
-		$key = wfMemcKey( 'test' );
-		$this->assertTrue( $this->cache->add( $key, 'test' ) );
+		$key = $this->cache->makeKey( self::TEST_KEY );
+		$this->assertTrue( $this->cache->add( $key, 'test', 5 ) );
 	}
 
 	/**
@@ -169,16 +169,18 @@ class BagOStuffTest extends MediaWikiTestCase {
 	public function testGet() {
 		$value = [ 'this' => 'is', 'a' => 'test' ];
 
-		$key = wfMemcKey( 'test' );
-		$this->cache->add( $key, $value );
+		$key = $this->cache->makeKey( self::TEST_KEY );
+		$this->cache->add( $key, $value, 5 );
 		$this->assertEquals( $this->cache->get( $key ), $value );
 	}
 
 	/**
+	 * @covers BagOStuff::get
+	 * @covers BagOStuff::set
 	 * @covers BagOStuff::getWithSetCallback
 	 */
 	public function testGetWithSetCallback() {
-		$key = wfMemcKey( 'test' );
+		$key = $this->cache->makeKey( self::TEST_KEY );
 		$value = $this->cache->getWithSetCallback(
 			$key,
 			30,
@@ -195,8 +197,8 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers BagOStuff::incr
 	 */
 	public function testIncr() {
-		$key = wfMemcKey( 'test' );
-		$this->cache->add( $key, 0 );
+		$key = $this->cache->makeKey( self::TEST_KEY );
+		$this->cache->add( $key, 0, 5 );
 		$this->cache->incr( $key );
 		$expectedValue = 1;
 		$actualValue = $this->cache->get( $key );
@@ -207,7 +209,7 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers BagOStuff::incrWithInit
 	 */
 	public function testIncrWithInit() {
-		$key = wfMemcKey( 'test' );
+		$key = $this->cache->makeKey( self::TEST_KEY );
 		$val = $this->cache->incrWithInit( $key, 0, 1, 3 );
 		$this->assertEquals( 3, $val, "Correct init value" );
 
@@ -224,18 +226,24 @@ class BagOStuffTest extends MediaWikiTestCase {
 		$value3 = [ 'testing a key that may be encoded when sent to cache backend' ];
 		$value4 = [ 'another test where chars in key will be encoded' ];
 
-		$key1 = wfMemcKey( 'test1' );
-		$key2 = wfMemcKey( 'test2' );
+		$key1 = $this->cache->makeKey( 'test-1' );
+		$key2 = $this->cache->makeKey( 'test-2' );
 		// internally, MemcachedBagOStuffs will encode to will-%25-encode
-		$key3 = wfMemcKey( 'will-%-encode' );
-		$key4 = wfMemcKey(
+		$key3 = $this->cache->makeKey( 'will-%-encode' );
+		$key4 = $this->cache->makeKey(
 			'flowdb:flow_ref:wiki:by-source:v3:Parser\'s_"broken"_+_(page)_&_grill:testwiki:1:4.7'
 		);
 
-		$this->cache->add( $key1, $value1 );
-		$this->cache->add( $key2, $value2 );
-		$this->cache->add( $key3, $value3 );
-		$this->cache->add( $key4, $value4 );
+		// cleanup
+		$this->cache->delete( $key1 );
+		$this->cache->delete( $key2 );
+		$this->cache->delete( $key3 );
+		$this->cache->delete( $key4 );
+
+		$this->cache->add( $key1, $value1, 5 );
+		$this->cache->add( $key2, $value2, 5 );
+		$this->cache->add( $key3, $value3, 5 );
+		$this->cache->add( $key4, $value4, 5 );
 
 		$this->assertEquals(
 			[ $key1 => $value1, $key2 => $value2, $key3 => $value3, $key4 => $value4 ],
@@ -253,7 +261,7 @@ class BagOStuffTest extends MediaWikiTestCase {
 	 * @covers BagOStuff::getScopedLock
 	 */
 	public function testGetScopedLock() {
-		$key = wfMemcKey( 'test' );
+		$key = $this->cache->makeKey( self::TEST_KEY );
 		$value1 = $this->cache->getScopedLock( $key, 0 );
 		$value2 = $this->cache->getScopedLock( $key, 0 );