objectcache: Make WANObjectCache interim caching not interfere with ChronologyProtector

Also removed useless line from testLockTSE(). That would have needed
to be using $this->internalCache and those locks are freed immediately.

Bug: T180035
Change-Id: Ida1a923f779aaf8410da76643457d2200da6cb20

diff --git a/includes/Setup.php b/includes/Setup.php
index 081ea68..d6f4b2f 100644 (file)
--- a/includes/Setup.php
+++ b/includes/Setup.php
@@ -736,17 +736,20 @@ if ( !$wgDBerrorLogTZ ) {
 
 // Initialize the request object in $wgRequest
 $wgRequest = RequestContext::getMain()->getRequest(); // BackCompat
-// Set user IP/agent information for causal consistency purposes
+// Set user IP/agent information for causal consistency purposes.
+// The cpPosTime cookie has no prefix and is set by MediaWiki::preOutputCommit().
+$cpPosTime = $wgRequest->getFloat( 'cpPosTime', $wgRequest->getCookie( 'cpPosTime', '' ) );
 MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->setRequestInfo( [
        'IPAddress' => $wgRequest->getIP(),
        'UserAgent' => $wgRequest->getHeader( 'User-Agent' ),
        'ChronologyProtection' => $wgRequest->getHeader( 'ChronologyProtection' ),
-       // The cpPosTime cookie has no prefix and is set by MediaWiki::preOutputCommit()
-       'ChronologyPositionTime' => $wgRequest->getFloat(
-               'cpPosTime',
-               $wgRequest->getCookie( 'cpPosTime', '' )
-       )
+       'ChronologyPositionTime' => $cpPosTime
 ] );
+// Make sure that caching does not compromise the consistency improvements
+if ( $cpPosTime ) {
+       MediaWikiServices::getInstance()->getMainWANObjectCache()->useInterimHoldOffCaching( false );
+}
+unset( $cpPosTime );
 
 // Useful debug output
 if ( $wgCommandLineMode ) {

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index db27e42..74ec7b9 100644 (file)
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -91,6 +91,8 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
        protected $logger;
        /** @var StatsdDataFactoryInterface */
        protected $stats;
+       /** @var bool Whether to use "interim" caching while keys are tombstoned */
+       protected $useInterimHoldOffCaching = true;
 
        /** @var int ERR_* constant for the "last error" registry */
        protected $lastRelayError = self::ERR_NONE;
@@ -1104,6 +1106,10 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
         * @return mixed
         */
        protected function getInterimValue( $key, $versioned, $minTime, &$asOf ) {
+               if ( !$this->useInterimHoldOffCaching ) {
+                       return false; // disabled
+               }
+
                $wrapped = $this->cache->get( self::INTERIM_KEY_PREFIX . $key );
                list( $value ) = $this->unwrap( $wrapped, $this->getCurrentTime() );
                if ( $value !== false && $this->isValid( $value, $versioned, $asOf, $minTime ) ) {
@@ -1495,6 +1501,30 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
                $this->processCaches = [];
        }
 
+       /**
+        * Disable the use of brief caching for tombstoned keys
+        *
+        * When a key is purged via delete(), there normally is a period where caching
+        * is hold-off limited to an extremely short time. This method will disable that
+        * caching, forcing the callback to run for any of:
+        *   - WANObjectCache::getWithSetCallback()
+        *   - WANObjectCache::getMultiWithSetCallback()
+        *   - WANObjectCache::getMultiWithUnionSetCallback()
+        *
+        * This is useful when both:
+        *   - a) the database used by the callback is known to be up-to-date enough
+        *        for some particular purpose (e.g. replica DB has applied transaction X)
+        *   - b) the caller needs to exploit that fact, and therefore needs to avoid the
+        *        use of inherently volatile and possibly stale interim keys
+        *
+        * @see WANObjectCache::delete()
+        * @param bool $enabled Whether to enable interim caching
+        * @since 1.31
+        */
+       public function useInterimHoldOffCaching( $enabled ) {
+               $this->useInterimHoldOffCaching = $enabled;
+       }
+
        /**
         * @param int $flag ATTR_* class constant
         * @return int QOS_* class constant

diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
index df8228d..c2be911 100644 (file)
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -769,8 +769,6 @@ class WANObjectCacheTest extends PHPUnit_Framework_TestCase {
                $calls = 0;
                $func = function () use ( &$calls, $value, $cache, $key ) {
                        ++$calls;
-                       // Immediately kill any mutex rather than waiting a second
-                       $cache->delete( $cache::MUTEX_KEY_PREFIX . $key );
                        return $value;
                };
 
@@ -778,7 +776,7 @@ class WANObjectCacheTest extends PHPUnit_Framework_TestCase {
                $this->assertEquals( $value, $ret );
                $this->assertEquals( 1, $calls, 'Value was populated' );
 
-               // Acquire a lock to verify that getWithSetCallback uses lockTSE properly
+               // Acquire the mutex to verify that getWithSetCallback uses lockTSE properly
                $this->internalCache->add( $cache::MUTEX_KEY_PREFIX . $key, 1, 0 );
 
                $checkKeys = [ wfRandomString() ]; // new check keys => force misses
@@ -795,8 +793,8 @@ class WANObjectCacheTest extends PHPUnit_Framework_TestCase {
 
                $ret = $cache->getWithSetCallback( $key, 30, $func,
                        [ 'lockTSE' => 5, 'checkKeys' => $checkKeys ] );
-               $this->assertEquals( $value, $ret, 'Callback was not used; used interim' );
-               $this->assertEquals( 2, $calls, 'Callback was not used; used interim' );
+               $this->assertEquals( $value, $ret, 'Callback was not used; used interim (mutex failed)' );
+               $this->assertEquals( 2, $calls, 'Callback was not used; used interim (mutex failed)' );
        }
 
        /**
@@ -1187,6 +1185,58 @@ class WANObjectCacheTest extends PHPUnit_Framework_TestCase {
                ];
        }
 
+       /**
+        * @covers WANObjectCache::useInterimHoldOffCaching
+        * @covers WANObjectCache::getInterimValue
+        */
+       public function testInterimHoldOffCaching() {
+               $cache = $this->cache;
+
+               $value = 'CRL-40-940';
+               $wasCalled = 0;
+               $func = function () use ( &$wasCalled, $value ) {
+                       $wasCalled++;
+
+                       return $value;
+               };
+
+               $cache->useInterimHoldOffCaching( true );
+
+               $key = wfRandomString( 32 );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 1, $wasCalled, 'Value cached' );
+               $cache->delete( $key );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 2, $wasCalled, 'Value regenerated (got mutex)' ); // sets interim
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 3, $wasCalled, 'Value regenerated (got mutex)' ); // sets interim
+               // Lock up the mutex so interim cache is used
+               $this->internalCache->add( $cache::MUTEX_KEY_PREFIX . $key, 1, 0 );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 3, $wasCalled, 'Value interim cached (failed mutex)' );
+               $this->internalCache->delete( $cache::MUTEX_KEY_PREFIX . $key );
+
+               $cache->useInterimHoldOffCaching( false );
+
+               $wasCalled = 0;
+               $key = wfRandomString( 32 );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 1, $wasCalled, 'Value cached' );
+               $cache->delete( $key );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 2, $wasCalled, 'Value regenerated (got mutex)' );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 3, $wasCalled, 'Value still regenerated (got mutex)' );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 4, $wasCalled, 'Value still regenerated (got mutex)' );
+               // Lock up the mutex so interim cache is used
+               $this->internalCache->add( $cache::MUTEX_KEY_PREFIX . $key, 1, 0 );
+               $v = $cache->getWithSetCallback( $key, 60, $func );
+               $this->assertEquals( 5, $wasCalled, 'Value still regenerated (failed mutex)' );
+       }
+
        /**
         * @covers WANObjectCache::touchCheckKey
         * @covers WANObjectCache::resetCheckKey