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 69edb11..1852685 100644 (file)
--- 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 9e4a5d7..329c642 100644 (file)
--- 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()
         */