From 43ff2a83b53e82193174fa78d9f300be6f5c79c7 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 20 Oct 2016 14:47:03 -0700 Subject: [PATCH] objectcache: avoid using process cache in nested callbacks Because the process cache can be lagged by virtue of blind TTL, the HOLDOFF_TTL might not be enough to account for it, so avoid using it when already inside a callback. Also split of the tests from the MediaWiki test class, so this does not require DB access anymore. Change-Id: I743a1233a5efc7f036fad140a9ff8a30b32f8f27 --- includes/libs/objectcache/WANObjectCache.php | 15 ++++++-- .../libs/objectcache/WANObjectCacheTest.php | 35 +++++++++++-------- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php index e7c4edbee6..8d3c6d96e4 100644 --- a/includes/libs/objectcache/WANObjectCache.php +++ b/includes/libs/objectcache/WANObjectCache.php @@ -88,6 +88,8 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface { /** @var int ERR_* constant for the "last error" registry */ protected $lastRelayError = self::ERR_NONE; + /** @var integer Callback stack depth for getWithSetCallback() */ + private $callbackDepth = 0; /** @var mixed[] Temporary warm-up cache */ private $warmupCache = []; @@ -847,8 +849,10 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface { final public function getWithSetCallback( $key, $ttl, $callback, array $opts = [] ) { $pcTTL = isset( $opts['pcTTL'] ) ? $opts['pcTTL'] : self::TTL_UNCACHEABLE; - // Try the process cache if enabled - if ( $pcTTL >= 0 ) { + // Try the process cache if enabled and the cache callback is not within a cache callback. + // Process cache use in nested callbacks is not lag-safe with regard to HOLDOFF_TTL since + // the in-memory value is further lagged than the shared one since it uses a blind TTL. + if ( $pcTTL >= 0 && $this->callbackDepth == 0 ) { $group = isset( $opts['pcGroup'] ) ? $opts['pcGroup'] : self::PC_PRIMARY; $procCache = $this->getProcessCache( $group ); $value = $procCache->get( $key ); @@ -995,7 +999,12 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface { // Generate the new value from the callback... $setOpts = []; - $value = call_user_func_array( $callback, [ $cValue, &$ttl, &$setOpts, $asOf ] ); + ++$this->callbackDepth; + try { + $value = call_user_func_array( $callback, [ $cValue, &$ttl, &$setOpts, $asOf ] ); + } finally { + --$this->callbackDepth; + } // When delete() is called, writes are write-holed by the tombstone, // so use a special INTERIM key to pass the new value around threads. if ( ( $isTombstone && $lockTSE > 0 ) && $value !== false && $ttl >= 0 ) { diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php index 4e455f7643..aa46c966ad 100644 --- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php +++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php @@ -1,6 +1,6 @@ getCliArg( 'use-wanobjectcache' ) ) { - $name = $this->getCliArg( 'use-wanobjectcache' ); - - $this->cache = ObjectCache::getWANInstance( $name ); - } else { - $this->cache = new WANObjectCache( [ - 'cache' => new HashBagOStuff(), - 'pool' => 'testcache-hash', - 'relayer' => new EventRelayerNull( [] ) - ] ); - } + $this->cache = new WANObjectCache( [ + 'cache' => new HashBagOStuff(), + 'pool' => 'testcache-hash', + 'relayer' => new EventRelayerNull( [] ) + ] ); $wanCache = TestingAccessWrapper::newFromObject( $this->cache ); /** @noinspection PhpUndefinedFieldInspection */ @@ -147,6 +141,19 @@ class WANObjectCacheTest extends MediaWikiTestCase { $key, 100, $callback, [ 'pcTTL' => 5, 'pcGroup' => $groups[$i] ] ); } $this->assertEquals( 9, $hit, "Values evicted" ); + + $key = reset( $keys ); + // Get into cache + $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); + $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); + $this->assertEquals( 10, $hit, "Value cached" ); + $outerCallback = function () use ( &$callback, $key ) { + $v = $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); + + return 43 + $v; + }; + $this->cache->getWithSetCallback( $key, 100, $outerCallback ); + $this->assertEquals( 11, $hit, "Nested callback value process cache skipped" ); } /** @@ -206,7 +213,7 @@ class WANObjectCacheTest extends MediaWikiTestCase { $this->assertEquals( $value, $v, "Value returned" ); $this->assertEquals( 1, $wasSet, "Value regenerated due to check keys" ); $this->assertEquals( $value, $priorValue, "Has prior value" ); - $this->assertType( 'float', $priorAsOf, "Has prior value" ); + $this->assertInternalType( 'float', $priorAsOf, "Has prior value" ); $t1 = $cache->getCheckKeyTime( $cKey1 ); $this->assertGreaterThanOrEqual( $priorTime, $t1, 'Check keys generated on miss' ); $t2 = $cache->getCheckKeyTime( $cKey2 ); @@ -316,7 +323,7 @@ class WANObjectCacheTest extends MediaWikiTestCase { $this->assertEquals( $value, $v[$keyB], "Value returned" ); $this->assertEquals( 1, $wasSet, "Value regenerated due to check keys" ); $this->assertEquals( $value, $priorValue, "Has prior value" ); - $this->assertType( 'float', $priorAsOf, "Has prior value" ); + $this->assertInternalType( 'float', $priorAsOf, "Has prior value" ); $t1 = $cache->getCheckKeyTime( $cKey1 ); $this->assertGreaterThanOrEqual( $priorTime, $t1, 'Check keys generated on miss' ); $t2 = $cache->getCheckKeyTime( $cKey2 ); -- 2.20.1