diff --git a/RELEASE-NOTES-1.35 b/RELEASE-NOTES-1.35 index 0eaa7c5a011..3b844b12046 100644 --- a/RELEASE-NOTES-1.35 +++ b/RELEASE-NOTES-1.35 @@ -70,6 +70,9 @@ For notes on 1.34.x and older releases, see HISTORY. administrators to limit the timeouts used by the HTTP client libraries. This only affects callers using HttpRequestFactory and the deprecated wrappers in the Http class. +* Added $wgCdnMaxageStale, which controls the Cache-Control s-maxage header + for page views when PoolCounter lock contention indicates that a stale cache + entry should be sent. * … ==== Changed configuration ==== diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index e48002272b5..930431dad4b 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -2896,6 +2896,14 @@ $wgCdnMaxAge = 18000; */ $wgCdnMaxageLagged = 30; +/** + * Cache timeout when delivering a stale ParserCache response due to PoolCounter + * contention. + * + * @since 1.35 + */ +$wgCdnMaxageStale = 10; + /** * Cache TTL for the user agent sent as max-age, for logged out users. * Only applies if $wgUseCdn is false. diff --git a/includes/page/Article.php b/includes/page/Article.php index d64331fc475..78a3dfedad6 100644 --- a/includes/page/Article.php +++ b/includes/page/Article.php @@ -622,7 +622,7 @@ class Article implements Page { * page of the given title. */ public function view() { - global $wgUseFileCache; + global $wgUseFileCache, $wgCdnMaxageStale; # Get variables from query string # As side effect this will load the revision and update the title @@ -807,11 +807,14 @@ class Article implements Page { $error = $poolArticleView->getError(); $this->mParserOutput = $poolArticleView->getParserOutput() ?: null; - # Don't cache a dirty ParserOutput object + # Cache stale ParserOutput object with a short expiry if ( $poolArticleView->getIsDirty() ) { - $outputPage->setCdnMaxage( 0 ); + $outputPage->setCdnMaxage( $wgCdnMaxageStale ); + $outputPage->setLastModified( $this->mParserOutput->getCacheTime() ); + $staleReason = $poolArticleView->getIsFastStale() + ? 'pool contention' : 'pool overload'; $outputPage->addHTML( "\n" ); + "sending anyway due to $staleReason-->\n" ); } } else { $ok = false; diff --git a/includes/poolcounter/PoolCounter.php b/includes/poolcounter/PoolCounter.php index 14865d1d49c..9255d6497d4 100644 --- a/includes/poolcounter/PoolCounter.php +++ b/includes/poolcounter/PoolCounter.php @@ -67,7 +67,7 @@ abstract class PoolCounter { protected $slots = 0; /** @var int If this number of workers are already working/waiting, fail instead of wait */ protected $maxqueue; - /** @var float Maximum time in seconds to wait for the lock */ + /** @var int Maximum time in seconds to wait for the lock */ protected $timeout; /** @@ -79,6 +79,11 @@ abstract class PoolCounter { */ private static $acquiredMightWaitKey = 0; + /** + * @var bool Enable fast stale mode (T250248). This may be overridden by the work class. + */ + private $fastStale; + /** * @param array $conf * @param string $type The class of actions to limit concurrency for (task type) @@ -91,6 +96,7 @@ abstract class PoolCounter { if ( isset( $conf['slots'] ) ) { $this->slots = $conf['slots']; } + $this->fastStale = $conf['fastStale'] ?? false; if ( $this->slots ) { $key = $this->hashKeyIntoSlots( $type, $key, $this->slots ); @@ -129,17 +135,21 @@ abstract class PoolCounter { /** * I want to do this task and I need to do it myself. * + * @param int|null $timeout Wait timeout, or null to use value passed to + * the constructor * @return Status Value is one of Locked/Error */ - abstract public function acquireForMe(); + abstract public function acquireForMe( $timeout = null ); /** * I want to do this task, but if anyone else does it * instead, it's also fine for me. I will read its cached data. * + * @param int|null $timeout Wait timeout, or null to use value passed to + * the constructor * @return Status Value is one of Locked/Done/Error */ - abstract public function acquireForAnyone(); + abstract public function acquireForAnyone( $timeout = null ); /** * I have successfully finished my task. @@ -207,4 +217,8 @@ abstract class PoolCounter { protected function hashKeyIntoSlots( $type, $key, $slots ) { return $type . ':' . ( hexdec( substr( sha1( $key ), 0, 4 ) ) % $slots ); } + + public function isFastStaleEnabled() { + return $this->fastStale; + } } diff --git a/includes/poolcounter/PoolCounterNull.php b/includes/poolcounter/PoolCounterNull.php index 95a5057131d..9f037042bcc 100644 --- a/includes/poolcounter/PoolCounterNull.php +++ b/includes/poolcounter/PoolCounterNull.php @@ -30,11 +30,11 @@ class PoolCounterNull extends PoolCounter { /* No parameters needed */ } - public function acquireForMe() { + public function acquireForMe( $timeout = null ) { return Status::newGood( PoolCounter::LOCKED ); } - public function acquireForAnyone() { + public function acquireForAnyone( $timeout = null ) { return Status::newGood( PoolCounter::LOCKED ); } diff --git a/includes/poolcounter/PoolCounterRedis.php b/includes/poolcounter/PoolCounterRedis.php index 90054de120b..e5fd95b738e 100644 --- a/includes/poolcounter/PoolCounterRedis.php +++ b/includes/poolcounter/PoolCounterRedis.php @@ -125,22 +125,22 @@ class PoolCounterRedis extends PoolCounter { return Status::newGood( $this->conn ); } - public function acquireForMe() { + public function acquireForMe( $timeout = null ) { $status = $this->precheckAcquire(); if ( !$status->isGood() ) { return $status; } - return $this->waitForSlotOrNotif( self::AWAKE_ONE ); + return $this->waitForSlotOrNotif( self::AWAKE_ONE, $timeout ); } - public function acquireForAnyone() { + public function acquireForAnyone( $timeout = null ) { $status = $this->precheckAcquire(); if ( !$status->isGood() ) { return $status; } - return $this->waitForSlotOrNotif( self::AWAKE_ALL ); + return $this->waitForSlotOrNotif( self::AWAKE_ALL, $timeout ); } public function release() { @@ -229,9 +229,10 @@ LUA; /** * @param int $doWakeup AWAKE_* constant + * @param int|float|null $timeout * @return Status */ - protected function waitForSlotOrNotif( $doWakeup ) { + protected function waitForSlotOrNotif( $doWakeup, $timeout = null ) { if ( $this->slot !== null ) { return Status::newGood( PoolCounter::LOCK_HELD ); // already acquired } @@ -245,6 +246,7 @@ LUA; '@phan-var RedisConnRef $conn'; $now = microtime( true ); + $timeout = $timeout ?? $this->timeout; try { $slot = $this->initAndPopPoolSlotList( $conn, $now ); if ( ctype_digit( $slot ) ) { @@ -261,7 +263,7 @@ LUA; // Just wait for an actual pool slot : [ $this->getSlotListKey() ]; - $res = $conn->blPop( $keys, $this->timeout ); + $res = $conn->blPop( $keys, $timeout ); if ( $res === [] ) { $conn->zRem( $this->getWaitSetKey(), $this->session ); // no longer waiting return Status::newGood( PoolCounter::TIMEOUT ); diff --git a/includes/poolcounter/PoolCounterWork.php b/includes/poolcounter/PoolCounterWork.php index 4eb466629fc..2cc09a97946 100644 --- a/includes/poolcounter/PoolCounterWork.php +++ b/includes/poolcounter/PoolCounterWork.php @@ -45,12 +45,14 @@ abstract class PoolCounterWork { /** * Actually perform the work, caching it if needed + * * @return mixed Work result or false */ abstract public function doWork(); /** * Retrieve the work from cache + * * @return mixed Work result or false */ public function getCachedWork() { @@ -60,9 +62,11 @@ abstract class PoolCounterWork { /** * A work not so good (eg. expired one) but better than an error * message. + * + * @param bool $fast True if PoolCounter is requesting a fast stale response (pre-wait) * @return mixed Work result or false */ - public function fallback() { + public function fallback( $fast ) { return false; } @@ -70,13 +74,21 @@ abstract class PoolCounterWork { * Do something with the error, like showing it to the user. * * @param Status $status - * * @return bool */ public function error( $status ) { return false; } + /** + * Should fast stale mode be used? + * + * @return bool + */ + protected function isFastStaleEnabled() { + return $this->poolCounter->isFastStaleEnabled(); + } + /** * Log an error * @@ -92,8 +104,7 @@ abstract class PoolCounterWork { /** * Get the result of the work (whatever it is), or the result of the error() function. - * This returns the result of the first applicable method that returns a non-false value, - * where the methods are checked in the following order: + * This returns the result of the one of the following methods: * - a) doWork() : Applies if the work is exclusive or no another process * is doing it, and on the condition that either this process * successfully entered the pool or the pool counter is down. @@ -102,12 +113,35 @@ abstract class PoolCounterWork { * - c) fallback() : Applies for all remaining cases. * If these all fall through (by returning false), then the result of error() is returned. * + * In slow stale mode, these three methods are called in the sequence given above, and + * the first non-false response is used. + * + * In fast stale mode, fallback() is called first if the lock acquisition would block. + * If fallback() returns false, the lock is waited on, then the three methods are + * called in the same sequence as for slow stale mode, including potentially calling + * fallback() a second time. + * * @param bool $skipcache * @return mixed */ public function execute( $skipcache = false ) { if ( $this->cacheable && !$skipcache ) { - $status = $this->poolCounter->acquireForAnyone(); + if ( $this->isFastStaleEnabled() ) { + // In fast stale mode, do not wait if fallback() would succeed. + // Try to acquire the lock with timeout=0 + $status = $this->poolCounter->acquireForAnyone( 0 ); + if ( $status->isOK() && $status->value === PoolCounter::TIMEOUT ) { + // Lock acquisition would block: try fallback + $staleResult = $this->fallback( true ); + if ( $staleResult !== false ) { + return $staleResult; + } + // No fallback available, so wait for the lock + $status = $this->poolCounter->acquireForAnyone(); + } // else behave as if $status were returned in slow mode + } else { + $status = $this->poolCounter->acquireForAnyone(); + } } else { $status = $this->poolCounter->acquireForMe(); } @@ -143,7 +177,7 @@ abstract class PoolCounterWork { case PoolCounter::QUEUE_FULL: case PoolCounter::TIMEOUT: - $result = $this->fallback(); + $result = $this->fallback( false ); if ( $result !== false ) { return $result; diff --git a/includes/poolcounter/PoolCounterWorkViaCallback.php b/includes/poolcounter/PoolCounterWorkViaCallback.php index aed52d19575..bd744dfdf5f 100644 --- a/includes/poolcounter/PoolCounterWorkViaCallback.php +++ b/includes/poolcounter/PoolCounterWorkViaCallback.php @@ -76,9 +76,9 @@ class PoolCounterWorkViaCallback extends PoolCounterWork { return false; } - public function fallback() { + public function fallback( $fast ) { if ( $this->fallback ) { - return ( $this->fallback )(); + return ( $this->fallback )( $fast ); } return false; } diff --git a/includes/poolcounter/PoolWorkArticleView.php b/includes/poolcounter/PoolWorkArticleView.php index 56f7fdb0f7d..61d4c62edec 100644 --- a/includes/poolcounter/PoolWorkArticleView.php +++ b/includes/poolcounter/PoolWorkArticleView.php @@ -59,6 +59,9 @@ class PoolWorkArticleView extends PoolCounterWork { /** @var bool */ private $isDirty = false; + /** @var bool */ + private $isFast = false; + /** @var Status|bool */ private $error = false; @@ -149,6 +152,15 @@ class PoolWorkArticleView extends PoolCounterWork { return $this->isDirty; } + /** + * Get whether the ParserOutput was retrieved in fast stale mode + * + * @return bool + */ + public function getIsFastStale() { + return $this->isFast; + } + /** * Get a Status object in case of error or false otherwise * @@ -251,18 +263,44 @@ class PoolWorkArticleView extends PoolCounterWork { } /** + * @param bool $fast Fast stale request * @return bool */ - public function fallback() { + public function fallback( $fast ) { $this->parserOutput = $this->parserCache->getDirty( $this->page, $this->parserOptions ); + $fastMsg = ''; + if ( $this->parserOutput && $fast ) { + /* Check if the stale response is from before the last write to the + * DB by this user. Declining to return a stale response in this + * case ensures that the user will see their own edit after page + * save. + * + * Note that the CP touch time is the timestamp of the shutdown of + * the save request, so there is a bias towards avoiding fast stale + * responses of potentially several seconds. + */ + $lastWriteTime = MediaWikiServices::getInstance()->getDBLoadBalancerFactory() + ->getChronologyProtectorTouched(); + $cacheTime = MWTimestamp::convert( TS_UNIX, $this->parserOutput->getCacheTime() ); + if ( $lastWriteTime && $cacheTime <= $lastWriteTime ) { + wfDebugLog( 'dirty', "declining to send dirty output since cache time " . + $cacheTime . " is before last write time $lastWriteTime" ); + // Forget this ParserOutput -- we will request it again if + // necessary in slow mode. There might be a newer entry + // available by that time. + $this->parserOutput = false; + return false; + } + $this->isFast = true; + $fastMsg = 'fast '; + } + if ( $this->parserOutput === false ) { wfDebugLog( 'dirty', 'dirty missing' ); - wfDebug( __METHOD__ . ": no dirty cache" ); return false; } else { - wfDebug( __METHOD__ . ": sending dirty output" ); - wfDebugLog( 'dirty', "dirty output {$this->cacheKey}" ); + wfDebugLog( 'dirty', "{$fastMsg}dirty output {$this->cacheKey}" ); $this->isDirty = true; return true; } diff --git a/tests/phpunit/integration/includes/poolcounter/PoolCounterWorkTest.php b/tests/phpunit/integration/includes/poolcounter/PoolCounterWorkTest.php index aa877225b81..04eefa45747 100644 --- a/tests/phpunit/integration/includes/poolcounter/PoolCounterWorkTest.php +++ b/tests/phpunit/integration/includes/poolcounter/PoolCounterWorkTest.php @@ -248,6 +248,6 @@ class PoolCounterWorkTest extends MediaWikiIntegrationTestCase { $this->assertFalse( $worker->cacheable ); $this->assertFalse( $worker->getCachedWork() ); $this->assertFalse( $worker->error( Status::newFatal( 'apierror' ) ) ); - $this->assertFalse( $worker->fallback() ); + $this->assertFalse( $worker->fallback( false ) ); } } diff --git a/tests/phpunit/mocks/poolcounter/MockPoolCounterFailing.php b/tests/phpunit/mocks/poolcounter/MockPoolCounterFailing.php index 54dabce7eec..f967e9e0a27 100644 --- a/tests/phpunit/mocks/poolcounter/MockPoolCounterFailing.php +++ b/tests/phpunit/mocks/poolcounter/MockPoolCounterFailing.php @@ -24,11 +24,11 @@ class MockPoolCounterFailing extends PoolCounter { public function __construct() { } - public function acquireForMe() { + public function acquireForMe( $timeout = null ) { return Status::newGood( PoolCounter::QUEUE_FULL ); } - public function acquireForAnyone() { + public function acquireForAnyone( $timeout = null ) { return Status::newGood( PoolCounter::QUEUE_FULL ); }