Fast stale ParserCache responses

If PoolCounter acquisition would block and a stale ParserCache entry is
available, deliver it immediately rather than waiting for the lock. This
should avoid PoolCounter contention on heavily edited pages.

* Add a fastStale pool option to toggle the feature. False by default
  but I'll set the default to true in a followup commit.
* Add a $timeout parameter to PoolCounter::acquireForMe() and
  acquireForAnyone(). This requires a simultaneous update to the
  PoolCounter extension.
* In the Redis implementation, use the requested timeout for blPop()
  but use the configured timeout for data structure cleanup and item
  expiry.
* Add a boolean $fast parameter to fallback() which tells the subclass
  whether it is being called in the fast or slow mode. No extensions
  in CodeSearch extend PoolCounterWork directly so this should not
  cause a fatal.
* Pass through the $fast parameter in PoolCounterWorkViaCallback
* In PoolWorkArticleView, use the $fast flag to decide whether to check
  the ChronologyProtector touched timestamp.
* Add $wgCdnMaxageStale by analogy with $wgCdnMaxageLagged, which
  controls the CC:s-maxage when sending a stale ParserOutput.
* Fix the documented type of the timeout. It really should be a float,
  but locks.c will treat non-integers as zero.

A simultaneous update to the PoolCounter extension is required.

Bug: T250248
Change-Id: I1f410cd5d83588e584b6d27d2e106465f0fad23e

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 ====

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.

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( "<!-- parser cache is expired, " .
-								"sending anyway due to pool overload-->\n" );
+								"sending anyway due to $staleReason-->\n" );
 						}
 					} else {
 						$ok = false;

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;
+	}
 }

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 );
 	}
 

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 );

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;

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;
 	}

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;
 		}

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 ) );
 	}
 }

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 );
 	}