Fix get()/getMulti() check key race condition in WANObjectCache

If a set() happened around the exact same time as check key was
initialized via get(), the curTTL in future get() calls could
sometimes be 0 instead of a negative value, even though hold-off
should apply.

Change-Id: Ide1fd65112aff425a4798e2ec406d71f2a8e84a7

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 4005abb..95bf743 100644 (file)
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -251,6 +251,7 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 
                // Fetch all of the raw values
                $wrappedValues = $this->cache->getMulti( array_merge( $valueKeys, $checkKeysFlat ) );
+               // Time used to compare/init "check" keys (derived after getMulti() to be pessimistic)
                $now = microtime( true );
 
                // Collect timestamps from all "check" keys
@@ -282,7 +283,10 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
                                foreach ( $purgeValues as $purge ) {
                                        $safeTimestamp = $purge[self::FLD_TIME] + $purge[self::FLD_HOLDOFF];
                                        if ( $safeTimestamp >= $wrappedValues[$vKey][self::FLD_TIME] ) {
-                                               $curTTL = min( $curTTL, $purge[self::FLD_TIME] - $now );
+                                               // How long ago this value was expired by *this* check key
+                                               $ago = min( $purge[self::FLD_TIME] - $now, self::TINY_NEGATIVE );
+                                               // How long ago this value was expired by *any* known check key
+                                               $curTTL = min( $curTTL, $ago );
                                        }
                                }
                        }

diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
index 1511557..efc37d2 100644 (file)
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -303,9 +303,9 @@ class WANObjectCacheTest extends MediaWikiTestCase {
                // Fake initial check key to be set in the past. Otherwise we'd have to sleep for
                // several seconds during the test to assert the behaviour.
                foreach ( array( $checkAll, $check1, $check2 ) as $checkKey ) {
-                       $this->internalCache->set( $cache::TIME_KEY_PREFIX . $checkKey,
-                               $cache::PURGE_VAL_PREFIX . microtime( true ) - $cache::HOLDOFF_TTL, $cache::CHECK_KEY_TTL );
+                       $cache->touchCheckKey( $checkKey, WANObjectCache::HOLDOFF_NONE );
                }
+               usleep( 100 );
 
                $cache->set( 'key1', $value1, 10 );
                $cache->set( 'key2', $value2, 10 );
@@ -322,14 +322,12 @@ class WANObjectCacheTest extends MediaWikiTestCase {
                        $result,
                        'Initial values'
                );
-               $this->assertEquals(
-                       array( 'key1' => 0, 'key2' => 0 ),
-                       $curTTLs,
-                       'Initial ttls'
-               );
+               $this->assertGreaterThanOrEqual( 9.5, $curTTLs['key1'], 'Initial ttls' );
+               $this->assertLessThanOrEqual( 10.5, $curTTLs['key1'], 'Initial ttls' );
+               $this->assertGreaterThanOrEqual( 9.5, $curTTLs['key2'], 'Initial ttls' );
+               $this->assertLessThanOrEqual( 10.5, $curTTLs['key2'], 'Initial ttls' );
 
                $cache->touchCheckKey( $check1 );
-               usleep( 100 );
 
                $curTTLs = array();
                $result = $cache->getMulti( array( 'key1', 'key2', 'key3' ), $curTTLs, array(
@@ -344,10 +342,9 @@ class WANObjectCacheTest extends MediaWikiTestCase {
                        'key1 expired by check1, but value still provided'
                );
                $this->assertLessThan( 0, $curTTLs['key1'], 'key1 TTL expired' );
-               $this->assertEquals( 0, $curTTLs['key2'], 'key2 still valid' );
+               $this->assertGreaterThan( 0, $curTTLs['key2'], 'key2 still valid' );
 
                $cache->touchCheckKey( $checkAll );
-               usleep( 100 );
 
                $curTTLs = array();
                $result = $cache->getMulti( array( 'key1', 'key2', 'key3' ), $curTTLs, array(
@@ -365,6 +362,39 @@ class WANObjectCacheTest extends MediaWikiTestCase {
                $this->assertLessThan( 0, $curTTLs['key2'], 'key2 expired by checkAll' );
        }
 
+       /**
+        * @covers WANObjectCache::get()
+        * @covers WANObjectCache::processCheckKeys()
+        */
+       public function testCheckKeyInitHoldoff() {
+               $cache = $this->cache;
+
+               for ( $i = 0; $i < 500; ++$i ) {
+                       $key = wfRandomString();
+                       $checkKey = wfRandomString();
+                       // miss, set, hit
+                       $cache->get( $key, $curTTL, array( $checkKey ) );
+                       $cache->set( $key, 'val', 10 );
+                       $curTTL = null;
+                       $v = $cache->get( $key, $curTTL, array( $checkKey ) );
+
+                       $this->assertEquals( 'val', $v );
+                       $this->assertLessThan( 0, $curTTL, "Step $i: CTL < 0 (miss/set/hit)" );
+               }
+
+               for ( $i = 0; $i < 500; ++$i ) {
+                       $key = wfRandomString();
+                       $checkKey = wfRandomString();
+                       // set, hit
+                       $cache->set( $key, 'val', 10 );
+                       $curTTL = null;
+                       $v = $cache->get( $key, $curTTL, array( $checkKey ) );
+
+                       $this->assertEquals( 'val', $v );
+                       $this->assertLessThan( 0, $curTTL, "Step $i: CTL < 0 (set/hit)" );
+               }
+       }
+
        /**
         * @covers WANObjectCache::delete()
         */