From fb013cdf2759e58fcf67d23042c336c83dc0e627 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 16 Jul 2019 02:31:54 -0700 Subject: [PATCH] objectcache: relax WANObjectCache "pcTTL" nesting rule to allow set() As long as get()s are disallowed from the process cache, the sets() should at least still be up-to-date, so there is little reason to prevent them. Change-Id: Ic62c8380801130de7f8412cddcf85b246e33b3cd --- includes/libs/objectcache/WANObjectCache.php | 8 +- .../libs/objectcache/WANObjectCacheTest.php | 158 +++++++++++++----- 2 files changed, 120 insertions(+), 46 deletions(-) diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php index 660d850946..65059c8ffd 100644 --- a/includes/libs/objectcache/WANObjectCache.php +++ b/includes/libs/objectcache/WANObjectCache.php @@ -1241,18 +1241,18 @@ class WANObjectCache implements IExpiringStore, IStoreKeyEncoder, LoggerAwareInt final public function getWithSetCallback( $key, $ttl, $callback, array $opts = [] ) { $version = $opts['version'] ?? null; $pcTTL = $opts['pcTTL'] ?? self::TTL_UNCACHEABLE; + $pCache = ( $pcTTL >= 0 ) + ? $this->getProcessCache( $opts['pcGroup'] ?? self::PC_PRIMARY ) + : null; // Use the process cache if requested as long as no outer cache callback is running. // Nested callback process cache use is not lag-safe with regard to HOLDOFF_TTL since // process cached values are more lagged than persistent ones as they are not purged. - if ( $pcTTL >= 0 && $this->callbackDepth == 0 ) { - $pCache = $this->getProcessCache( $opts['pcGroup'] ?? self::PC_PRIMARY ); + if ( $pCache && $this->callbackDepth == 0 ) { $cached = $pCache->get( $this->getProcessCacheKey( $key, $version ), INF, false ); if ( $cached !== false ) { return $cached; } - } else { - $pCache = null; } $res = $this->fetchOrRegenerate( $key, $ttl, $callback, $opts ); diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php index 6d32201291..890218c6d3 100644 --- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php +++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php @@ -128,73 +128,147 @@ class WANObjectCacheTest extends PHPUnit\Framework\TestCase { $this->assertFalse( $this->cache->get( $key ), "Stale set() value ignored" ); } - public function testProcessCache() { + /** + * @covers WANObjectCache::getWithSetCallback + */ + public function testProcessCacheLruAndDelete() { + $cache = $this->cache; $mockWallClock = 1549343530.2053; - $this->cache->setMockTime( $mockWallClock ); + $cache->setMockTime( $mockWallClock ); $hit = 0; - $callback = function () use ( &$hit ) { + $fn = function () use ( &$hit ) { ++$hit; return 42; }; - $keys = [ wfRandomString(), wfRandomString(), wfRandomString() ]; - $groups = [ 'thiscache:1', 'thatcache:1', 'somecache:1' ]; + $keysA = [ wfRandomString(), wfRandomString(), wfRandomString() ]; + $keysB = [ wfRandomString(), wfRandomString(), wfRandomString() ]; + $pcg = [ 'thiscache:1', 'thatcache:1', 'somecache:1' ]; - foreach ( $keys as $i => $key ) { - $this->cache->getWithSetCallback( - $key, 100, $callback, [ 'pcTTL' => 5, 'pcGroup' => $groups[$i] ] ); + foreach ( $keysA as $i => $key ) { + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5, 'pcGroup' => $pcg[$i] ] ); } - $this->assertEquals( 3, $hit ); + $this->assertEquals( 3, $hit, "Values not cached yet" ); - foreach ( $keys as $i => $key ) { - $this->cache->getWithSetCallback( - $key, 100, $callback, [ 'pcTTL' => 5, 'pcGroup' => $groups[$i] ] ); + foreach ( $keysA as $i => $key ) { + // Should not evict from process cache + $cache->delete( $key ); + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5, 'pcGroup' => $pcg[$i] ] ); } - $this->assertEquals( 3, $hit, "Values cached" ); + $this->assertEquals( 3, $hit, "Values cached; not cleared by delete()" ); - foreach ( $keys as $i => $key ) { - $this->cache->getWithSetCallback( - "$key-2", 100, $callback, [ 'pcTTL' => 5, 'pcGroup' => $groups[$i] ] ); + foreach ( $keysB as $i => $key ) { + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5, 'pcGroup' => $pcg[$i] ] ); } - $this->assertEquals( 6, $hit ); + $this->assertEquals( 6, $hit, "New values not cached yet" ); - foreach ( $keys as $i => $key ) { - $this->cache->getWithSetCallback( - "$key-2", 100, $callback, [ 'pcTTL' => 5, 'pcGroup' => $groups[$i] ] ); + foreach ( $keysB as $i => $key ) { + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5, 'pcGroup' => $pcg[$i] ] ); } $this->assertEquals( 6, $hit, "New values cached" ); - foreach ( $keys as $i => $key ) { - // Should not evict from process cache - $this->cache->delete( $key ); + foreach ( $keysA as $i => $key ) { + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5, 'pcGroup' => $pcg[$i] ] ); + } + $this->assertEquals( 9, $hit, "Prior values evicted by new values" ); + } + + /** + * @covers WANObjectCache::getWithSetCallback + */ + public function testProcessCacheInterimKeys() { + $cache = $this->cache; + $mockWallClock = 1549343530.2053; + $cache->setMockTime( $mockWallClock ); + + $hit = 0; + $fn = function () use ( &$hit ) { + ++$hit; + return 42; + }; + $keysA = [ wfRandomString(), wfRandomString(), wfRandomString() ]; + $pcg = [ 'thiscache:1', 'thatcache:1', 'somecache:1' ]; + + foreach ( $keysA as $i => $key ) { + $cache->delete( $key ); // tombstone key $mockWallClock += 0.001; // cached values will be newer than tombstone - // Get into cache (specific process cache group) - $this->cache->getWithSetCallback( - $key, 100, $callback, [ 'pcTTL' => 5, 'pcGroup' => $groups[$i] ] ); + // Get into process cache (specific group) and interim cache + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5, 'pcGroup' => $pcg[$i] ] ); } - $this->assertEquals( 9, $hit, "Values evicted by delete()" ); + $this->assertEquals( 3, $hit ); - // Get into cache (default process cache group) - $key = reset( $keys ); - $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); - $this->assertEquals( 9, $hit, "Value recently interim-cached" ); + // Get into process cache (default group) + $key = reset( $keysA ); + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5 ] ); + $this->assertEquals( 3, $hit, "Value recently interim-cached" ); $mockWallClock += 0.2; // interim key not brand new - $this->cache->clearProcessCache(); - $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); - $this->assertEquals( 10, $hit, "Value calculated (interim key not recent and reset)" ); - $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); - $this->assertEquals( 10, $hit, "Value process cached" ); + $cache->clearProcessCache(); + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5 ] ); + $this->assertEquals( 4, $hit, "Value calculated (interim key not recent and reset)" ); + $cache->getWithSetCallback( $key, 100, $fn, [ 'pcTTL' => 5 ] ); + $this->assertEquals( 4, $hit, "Value process cached" ); + } - $mockWallClock += 0.2; // interim key not brand new - $outerCallback = function () use ( &$callback, $key ) { - $v = $this->cache->getWithSetCallback( $key, 100, $callback, [ 'pcTTL' => 5 ] ); + /** + * @covers WANObjectCache::getWithSetCallback + */ + public function testProcessCacheNesting() { + $cache = $this->cache; + $mockWallClock = 1549343530.2053; + $cache->setMockTime( $mockWallClock ); + + $keyOuter = "outer-" . wfRandomString(); + $keyInner = "inner-" . wfRandomString(); + + $innerHit = 0; + $innerFn = function () use ( &$innerHit ) { + ++$innerHit; + return 42; + }; + + $outerHit = 0; + $outerFn = function () use ( $keyInner, $innerFn, $cache, &$outerHit ) { + ++$outerHit; + $v = $cache->getWithSetCallback( $keyInner, 100, $innerFn, [ 'pcTTL' => 5 ] ); return 43 + $v; }; - // Outer key misses and refuses inner key process cache value - $this->cache->getWithSetCallback( "$key-miss-outer", 100, $outerCallback ); - $this->assertEquals( 11, $hit, "Nested callback value process cache skipped" ); + + $cache->getWithSetCallback( $keyInner, 100, $innerFn, [ 'pcTTL' => 5 ] ); + $cache->getWithSetCallback( $keyInner, 100, $innerFn, [ 'pcTTL' => 5 ] ); + + $this->assertEquals( 1, $innerHit, "Inner callback value cached" ); + $cache->delete( $keyInner, $cache::HOLDOFF_NONE ); + $mockWallClock += 1; + + $cache->getWithSetCallback( $keyInner, 100, $innerFn, [ 'pcTTL' => 5 ] ); + $this->assertEquals( 1, $innerHit, "Inner callback process cached" ); + + // Outer key misses and inner key process cache value is refused + $cache->getWithSetCallback( $keyOuter, 100, $outerFn ); + + $this->assertEquals( 1, $outerHit, "Outer callback value not yet cached" ); + $this->assertEquals( 2, $innerHit, "Inner callback value process cache skipped" ); + + $cache->getWithSetCallback( $keyOuter, 100, $outerFn ); + + $this->assertEquals( 1, $outerHit, "Outer callback value cached" ); + + $cache->delete( $keyInner, $cache::HOLDOFF_NONE ); + $cache->delete( $keyOuter, $cache::HOLDOFF_NONE ); + $mockWallClock += 1; + $cache->clearProcessCache(); + $cache->getWithSetCallback( $keyOuter, 100, $outerFn ); + + $this->assertEquals( 2, $outerHit, "Outer callback value not yet cached" ); + $this->assertEquals( 3, $innerHit, "Inner callback value not yet cached" ); + + $cache->delete( $keyInner, $cache::HOLDOFF_NONE ); + $mockWallClock += 1; + $cache->getWithSetCallback( $keyInner, 100, $innerFn, [ 'pcTTL' => 5 ] ); + + $this->assertEquals( 3, $innerHit, "Inner callback value process cached" ); } /** -- 2.20.1