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

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index e7c4edb..8d3c6d9 100644 (file)
--- 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 4e455f7..aa46c96 100644 (file)
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -1,6 +1,6 @@
 <?php
 
-class WANObjectCacheTest extends MediaWikiTestCase {
+class WANObjectCacheTest extends PHPUnit_Framework_TestCase  {
        /** @var WANObjectCache */
        private $cache;
        /**@var BagOStuff */
@@ -9,17 +9,11 @@ class WANObjectCacheTest extends MediaWikiTestCase {
        protected function setUp() {
                parent::setUp();
 
-               if ( $this->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 );