From ba06ccb394df994b6434f1d3434c6b21e064f963 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 4 Dec 2015 14:22:02 -0800
Subject: [PATCH] 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
---
 includes/libs/objectcache/WANObjectCache.php  |  6 ++-
 .../libs/objectcache/WANObjectCacheTest.php   | 50 +++++++++++++++----
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 4005abb090..95bf7432dc 100644
--- 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 1511557c62..efc37d2508 100644
--- 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()
 	 */
-- 
2.20.1