WANObjectCache: Change getWithSetCallback() signature to key/ttl/callback/opts

* Put 'checkKeys' param in opts array instead of as a separate parameter.
  It's neat to not have to skip unnamed/positional arguments that are optional.
* Move TTL to be before callback instead of after. This avoids dangling integers
  toward the bottom of a large code block that have no obvious meaning.
  Matches BagOStuff::getWithSetCallback.

Add unit test for lockTSE to confirm

Change-Id: I60d6b64fc5dff14108741bafe801fd1fde16d1df

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 4beb627..70f2af2 100644 (file)
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -621,31 +621,41 @@ class WANObjectCache {
         * @see WANObjectCache::set()
         *
         * @param string $key Cache key
-        * @param callable $callback Value generation function
         * @param integer $ttl Seconds to live for key updates. Special values are:
-        *   - WANObjectCache::TTL_NONE        : cache forever
-        *   - WANObjectCache::TTL_UNCACHEABLE : do not cache at all
-        * @param array $checkKeys List of "check" keys
+        *   - WANObjectCache::TTL_NONE : Cache forever
+        *   - WANObjectCache::TTL_UNCACHEABLE: Do not cache at all
+        * @param callable $callback Value generation function
         * @param array $opts Options map:
-        *   - lowTTL  : consider pre-emptive updates when the current TTL (sec)
-        *               of the key is less than this. It becomes more likely
-        *               over time, becoming a certainty once the key is expired.
-        *               [Default: WANObjectCache::LOW_TTL seconds]
-        *   - lockTSE : if the key is tombstoned or expired (by $checkKeys) less
-        *               than this many seconds ago, then try to have a single
-        *               thread handle cache regeneration at any given time.
-        *               Other threads will try to use stale values if possible.
-        *               If, on miss, the time since expiration is low, the assumption
-        *               is that the key is hot and that a stampede is worth avoiding.
-        *               Setting this above WANObjectCache::HOLDOFF_TTL makes no difference.
-        *               The higher this is set, the higher the worst-case staleness can be.
-        *               Use WANObjectCache::TSE_NONE to disable this logic.
-        *               [Default: WANObjectCache::TSE_NONE]
+        *   - checkKeys: List of "check" keys.
+        *   - lowTTL: Consider pre-emptive updates when the current TTL (sec) of the key is less than
+        *      this. It becomes more likely over time, becoming a certainty once the key is expired.
+        *      Default: WANObjectCache::LOW_TTL seconds.
+        *   - lockTSE: If the key is tombstoned or expired (by checkKeys) less than this many seconds
+        *      ago, then try to have a single thread handle cache regeneration at any given time.
+        *      Other threads will try to use stale values if possible. If, on miss, the time since
+        *      expiration is low, the assumption is that the key is hot and that a stampede is worth
+        *      avoiding. Setting this above WANObjectCache::HOLDOFF_TTL makes no difference. The
+        *      higher this is set, the higher the worst-case staleness can be.
+        *      Use WANObjectCache::TSE_NONE to disable this logic. Default: WANObjectCache::TSE_NONE.
         * @return mixed Value to use for the key
         */
        final public function getWithSetCallback(
-               $key, $callback, $ttl, array $checkKeys = array(), array $opts = array()
+               $key, $ttl, $callback, array $opts = array(), $oldOpts = null
        ) {
+               // Back-compat with 1.26: Swap $ttl and $callback
+               if ( is_int( $callback ) ) {
+                       $temp = $ttl;
+                       $ttl = $callback;
+                       $callback = $temp;
+               }
+               // Back-compat with 1.26: $checkKeys as separate parameter
+               if ( $oldOpts || ( is_array( $opts ) && isset( $opts[0] ) ) ) {
+                       $checkKeys = $opts;
+                       $opts = $oldOpts;
+               } else {
+                       $checkKeys = isset( $opts['checkKeys'] ) ? $opts['checkKeys'] : array();
+               }
+
                $lowTTL = isset( $opts['lowTTL'] ) ? $opts['lowTTL'] : min( self::LOW_TTL, $ttl );
                $lockTSE = isset( $opts['lockTSE'] ) ? $opts['lockTSE'] : self::TSE_NONE;
 

diff --git a/tests/phpunit/includes/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/objectcache/WANObjectCacheTest.php
index 1fe0bcd..4195216 100644 (file)
--- a/tests/phpunit/includes/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/objectcache/WANObjectCacheTest.php
@@ -13,11 +13,14 @@ class WANObjectCacheTest extends MediaWikiTestCase {
                        $this->cache = ObjectCache::getWANInstance( $name );
                } else {
                        $this->cache = new WANObjectCache( array(
-                               'cache'   => new HashBagOStuff(),
-                               'pool'    => 'testcache-hash',
+                               'cache' => new HashBagOStuff(),
+                               'pool' => 'testcache-hash',
                                'relayer' => new EventRelayerNull( array() )
                        ) );
                }
+
+               $wanCache = TestingAccessWrapper::newFromObject( $this->cache );
+               $this->internalCache = $wanCache->cache;
        }
 
        /**
@@ -145,6 +148,32 @@ class WANObjectCacheTest extends MediaWikiTestCase {
                $this->assertLessThanOrEqual( 0, $curTTL, "Value has current TTL < 0 due to check keys" );
        }
 
+       /**
+        * @covers WANObjectCache::getWithSetCallback()
+        */
+       public function testLockTSE() {
+               $cache = $this->cache;
+               $key = wfRandomString();
+               $value = wfRandomString();
+
+               $calls = 0;
+               $func = function() use ( &$calls, $value ) {
+                       ++$calls;
+                       return $value;
+               };
+
+               $cache->delete( $key );
+               $ret = $cache->getWithSetCallback( $key, 30, $func, array(), array( 'lockTSE' => 5 ) );
+               $this->assertEquals( $value, $ret );
+               $this->assertEquals( 1, $calls, 'Value was populated' );
+
+               // Acquire a lock to verify that getWithSetCallback uses lockTSE properly
+               $this->internalCache->lock( $key, 0 );
+               $ret = $cache->getWithSetCallback( $key, 30, $func, array(), array( 'lockTSE' => 5 ) );
+               $this->assertEquals( $value, $ret );
+               $this->assertEquals( 1, $calls, 'Callback was not used' );
+       }
+
        /**
         * @covers WANObjectCache::getMulti()
         */