From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 19 Jul 2019 10:31:46 +0000 (-0700)
Subject: objectcache: make "busyValue" stricter to avoid callback ambigiuity
X-Git-Tag: 1.34.0-rc.0~854^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=47aa48f0737538249017c2888ef49883da7240f8

objectcache: make "busyValue" stricter to avoid callback ambigiuity

Change-Id: I01a1503ff5b37d65ef148fef79270505d8eb3146
---

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 69edb11d81..1852685d6c 100644
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -1182,10 +1182,11 @@ class WANObjectCache implements IExpiringStore, IStoreKeyEncoder, LoggerAwareInt
 	 *      being no stale value available until after a thread completes the callback.
 	 *      Use WANObjectCache::TSE_NONE to disable this logic.
 	 *      Default: WANObjectCache::TSE_NONE.
-	 *   - busyValue: If no value exists and another thread is currently regenerating it, use this
-	 *      as a fallback value (or a callback to generate such a value). This assures that cache
-	 *      stampedes cannot happen if the value falls out of cache. This can be used as insurance
-	 *      against cache regeneration becoming very slow for some reason (greater than the TTL).
+	 *   - busyValue: Specify a placeholder value to use when no value exists and another thread
+	 *      is currently regenerating it. This assures that cache stampedes cannot happen if the
+	 *      value falls out of cache. This also mitigates stampedes when value regeneration
+	 *      becomes very slow (greater than $ttl/"lowTTL"). If this is a closure, then it will
+	 *      be invoked to get the placeholder when needed.
 	 *      Default: null.
 	 *   - pcTTL: Process cache the value in this PHP instance for this many seconds. This avoids
 	 *      network I/O when a key is read several times. This will not cache when the callback
@@ -1301,7 +1302,7 @@ class WANObjectCache implements IExpiringStore, IStoreKeyEncoder, LoggerAwareInt
 	 * @return array Ordered list of the following:
 	 *   - Cached or regenerated value
 	 *   - Cached or regenerated value version number or null if not versioned
-	 *   - Timestamp of the cached value or null if there is no value
+	 *   - Timestamp of the current cached value at the key or null if there is no value
 	 * @note Callable type hints are not used to avoid class-autoloading
 	 */
 	private function fetchOrRegenerate( $key, $ttl, $callback, array $opts ) {
@@ -1399,11 +1400,7 @@ class WANObjectCache implements IExpiringStore, IStoreKeyEncoder, LoggerAwareInt
 				$miss = is_infinite( $minAsOf ) ? 'renew' : 'miss';
 				$this->stats->increment( "wanobjectcache.$kClass.$miss.busy" );
 
-				return [
-					is_callable( $busyValue ) ? $busyValue() : $busyValue,
-					$version,
-					$curInfo['asOf']
-				];
+				return [ $this->resolveBusyValue( $busyValue ), $version, $curInfo['asOf'] ];
 			}
 		}
 
@@ -1604,6 +1601,14 @@ class WANObjectCache implements IExpiringStore, IStoreKeyEncoder, LoggerAwareInt
 		);
 	}
 
+	/**
+	 * @param mixed $busyValue
+	 * @return mixed
+	 */
+	private function resolveBusyValue( $busyValue ) {
+		return ( $busyValue instanceof Closure ) ? $busyValue() : $busyValue;
+	}
+
 	/**
 	 * Method to fetch multiple cache keys at once with regeneration
 	 *
diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
index 9e4a5d7d62..329c6426bf 100644
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -1070,7 +1070,7 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 	 * @covers WANObjectCache::getWithSetCallback()
 	 * @covers WANObjectCache::fetchOrRegenerate()
 	 */
-	public function testBusyValue() {
+	public function testBusyValueBasic() {
 		$cache = $this->cache;
 		$key = wfRandomString();
 		$value = wfRandomString();
@@ -1080,7 +1080,7 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 		$cache->setMockTime( $mockWallClock );
 
 		$calls = 0;
-		$func = function () use ( &$calls, $value, $cache, $key ) {
+		$func = function () use ( &$calls, $value ) {
 			++$calls;
 			return $value;
 		};
@@ -1125,6 +1125,52 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase {
 		$this->assertEquals( 3, $calls, 'Callback was not used; used interim' );
 	}
 
+	public function getBusyValues_Provider() {
+		$hash = new HashBagOStuff( [] );
+
+		return [
+			[
+				function () {
+					return "Saint Oliver Plunckett";
+				},
+				'Saint Oliver Plunckett'
+			],
+			[ 'strlen', 'strlen' ],
+			[ 'WANObjectCache::newEmpty', 'WANObjectCache::newEmpty' ],
+			[ [ 'WANObjectCache', 'newEmpty' ], [ 'WANObjectCache', 'newEmpty' ] ],
+			[ [ $hash, 'getLastError' ], [ $hash, 'getLastError' ] ],
+			[ [ 1, 2, 3 ], [ 1, 2, 3 ] ]
+		];
+	}
+
+	/**
+	 * @covers WANObjectCache::getWithSetCallback()
+	 * @covers WANObjectCache::fetchOrRegenerate()
+	 * @dataProvider getBusyValues_Provider
+	 * @param mixed $busyValue
+	 * @param mixed $expected
+	 */
+	public function testBusyValueTypes( $busyValue, $expected ) {
+		$cache = $this->cache;
+		$key = wfRandomString();
+
+		$mockWallClock = 1549343530.2053;
+		$cache->setMockTime( $mockWallClock );
+
+		$calls = 0;
+		$func = function () use ( &$calls ) {
+			++$calls;
+			return 418;
+		};
+
+		// Acquire a lock to verify that getWithSetCallback uses busyValue properly
+		$this->internalCache->add( 'WANCache:m:' . $key, 1, 0 );
+
+		$ret = $cache->getWithSetCallback( $key, 30, $func, [ 'busyValue' => $busyValue ] );
+		$this->assertSame( $expected, $ret, 'busyValue used as expected' );
+		$this->assertSame( 0, $calls, 'busyValue was used' );
+	}
+
 	/**
 	 * @covers WANObjectCache::getMulti()
 	 */