From 09cbebb22439bf4223e87ceb3e42169dc85fd311 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 7 Oct 2015 16:21:11 -0700 Subject: [PATCH] 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 --- includes/libs/objectcache/WANObjectCache.php | 48 +++++++++++-------- .../objectcache/WANObjectCacheTest.php | 33 ++++++++++++- 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php index 4beb627d6a..70f2af29e4 100644 --- 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 1fe0bcdc96..4195216518 100644 --- 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() */ -- 2.20.1