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 8a88581..6452c03 100644 (file)
--- 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 f720010..9b0d595 100644 (file)
--- 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 4320d80..d570297 100644 (file)
--- 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 );