rdbms: Inject CP instead of relying on LBF

This patch makes two major changes:
 - In the PoolCounter chain, we simply inject CP and call it directly
   and as result, there is no need for ILBF::getChronologyProtectorTouched
 - Instead of injecting CP callback to LB, just pass the object down the
   chain which leads to simpler and more stable code.

Bug: T275713
Change-Id: If78f4498d98e256015e54cc46561cb11b2947058

RELEASE-NOTES-1.41

@@ -177,7 +177,8 @@ because of Phabricator reports.
 * TransactionProfiler::setSilenced() deprecated in 1.40, has been removed.
 * ILoadBalancer::closeConnection, unused, has been removed without deprecation.
 * ILoadBalancer::waitFor, unused, has been removed without deprecation.
-* ILBFactory::resolveDomainID, unused, has been removed without deprecation.
+* ILBFactory::resolveDomainID and ::getChronologyProtectorTouched, unused,
+  have been removed without deprecation.
 * The following methods have been moved from ILoadBalancer to
   ILoadBalancerForOwner:
   - ::redefineLocalDomain()

includes/ServiceWiring.php

@@ -1460,6 +1460,7 @@ return [
 			$services->getRevisionRenderer(),
 			$services->getStatsdDataFactory(),
 			$services->getDBLoadBalancerFactory(),
+			$services->getChronologyProtector(),
 			LoggerFactory::getProvider(),
 			$services->getWikiPageFactory(),
 			$services->getTitleFormatter()

includes/libs/rdbms/lbfactory/ILBFactory.php

@@ -358,14 +358,6 @@ interface ILBFactory extends IConnectionProvider {
 	 */
 	public function setWaitForReplicationListener( $name, callable $callback = null );
 
-	/**
-	 * Get the UNIX timestamp when the client last touched the DB, if they did so recently
-	 *
-	 * @param DatabaseDomain|string|false $domain Domain ID, or false for the current domain
-	 * @return float|false UNIX timestamp; false if not recent or on record
-	 */
-	public function getChronologyProtectorTouched( $domain = false );
-
 	/**
 	 * Disable the ChronologyProtector on all instantiated tracked load balancer instances
 	 *

includes/libs/rdbms/lbfactory/LBFactory.php

@@ -557,10 +557,6 @@ abstract class LBFactory implements ILBFactory {
 		return $waitSucceeded;
 	}
 
-	public function getChronologyProtectorTouched( $domain = false ) {
-		return $this->chronologyProtector->getTouched( $this->getMainLB( $domain ) );
-	}
-
 	public function disableChronologyProtection() {
 		$this->chronologyProtector->setEnabled( false );
 	}
@@ -593,11 +589,7 @@ abstract class LBFactory implements ILBFactory {
 			'cliMode' => $this->cliMode,
 			'agent' => $this->agent,
 			'defaultGroup' => $this->defaultGroup,
-			'chronologyCallback' => function ( ILoadBalancer $lb ) {
-				// Defer ChronologyProtector construction in case setRequestInfo() ends up
-				// being called later (but before the first connection attempt) (T192611)
-				return $this->chronologyProtector->getSessionPrimaryPos( $lb );
-			},
+			'chronologyProtector' => $this->chronologyProtector,
 			'roundStage' => $initStage,
 			'criticalSectionProvider' => $this->csProvider
 		];

includes/libs/rdbms/loadbalancer/ILoadBalancerForOwner.php

@@ -52,9 +52,7 @@ interface ILoadBalancerForOwner extends ILoadBalancer {
 	 *  - srvCache : BagOStuff object for server cache [optional]
 	 *  - wanCache : WANObjectCache object [optional]
 	 *  - databaseFactory: DatabaseFactory object [optional]
-	 *  - chronologyCallback: Callback to run before the first connection attempt.
-	 *     It takes this ILoadBalancerForOwner instance and yields the relevant DBPrimaryPos
-	 *     for session (null if not applicable). [optional]
+	 *  - chronologyProtector: ChronologyProtector object [optional]
 	 *  - defaultGroup: Default query group; the generic group if not specified [optional]
 	 *  - hostname : The name of the current server [optional]
 	 *  - cliMode: Whether the execution context is a CLI script [optional]

includes/libs/rdbms/loadbalancer/LoadBalancer.php

@@ -41,8 +41,6 @@ use Wikimedia\ScopedCallback;
 class LoadBalancer implements ILoadBalancerForOwner {
 	/** @var ILoadMonitor */
 	private $loadMonitor;
-	/** @var callable|null Callback to run before the first connection attempt */
-	private $chronologyCallback;
 	/** @var BagOStuff */
 	private $srvCache;
 	/** @var WANObjectCache */
@@ -102,8 +100,9 @@ class LoadBalancer implements ILoadBalancerForOwner {
 	private $connectionCounter = 0;
 	/** @var bool */
 	private $disabled = false;
+	private ?ChronologyProtector $chronologyProtector = null;
 	/** @var bool Whether the session consistency callback already executed */
-	private $chronologyCallbackTriggered = false;
+	private $chronologyProtectorCalled = false;
 
 	/** @var Database|null The last connection handle that caused a problem */
 	private $lastErrorConn;
@@ -207,8 +206,8 @@ class LoadBalancer implements ILoadBalancerForOwner {
 		$this->trxProfiler = $params['trxProfiler'] ?? new TransactionProfiler();
 		$this->statsd = $params['statsdDataFactory'] ?? new NullStatsdDataFactory();
 
-		if ( isset( $params['chronologyCallback'] ) ) {
-			$this->chronologyCallback = $params['chronologyCallback'];
+		if ( isset( $params['chronologyProtector'] ) ) {
+			$this->chronologyProtector = $params['chronologyProtector'];
 		}
 
 		if ( isset( $params['roundStage'] ) ) {
@@ -526,7 +525,8 @@ class LoadBalancer implements ILoadBalancerForOwner {
 		while ( count( $currentLoads ) ) {
 			if ( $this->waitForPos && $this->waitForPos->asOfTime() ) {
 				$this->logger->debug( __METHOD__ . ": session has replication position" );
-				// "chronologyCallback" sets "waitForPos" for session consistency.
+				// ChronologyProtector::getSessionPrimaryPos called in $this->loadSessionPrimaryPos()
+				// sets "waitForPos" for session consistency.
 				// This triggers doWait() after connect, so it's especially good to
 				// avoid lagged servers so as to avoid excessive delay in that method.
 				$ago = microtime( true ) - $this->waitForPos->asOfTime();
@@ -1139,9 +1139,9 @@ class LoadBalancer implements ILoadBalancerForOwner {
 	 * @see awaitSessionPrimaryPos()
 	 */
 	private function loadSessionPrimaryPos() {
-		if ( !$this->chronologyCallbackTriggered && $this->chronologyCallback ) {
-			$this->chronologyCallbackTriggered = true;
-			$pos = ( $this->chronologyCallback )( $this );
+		if ( !$this->chronologyProtectorCalled && $this->chronologyProtector ) {
+			$this->chronologyProtectorCalled = true;
+			$pos = $this->chronologyProtector->getSessionPrimaryPos( $this );
 			$this->logger->debug( __METHOD__ . ': executed chronology callback.' );
 			if ( $pos ) {
 				if ( !$this->waitForPos || $pos->hasReached( $this->waitForPos ) ) {

includes/page/ParserOutputAccess.php

@@ -37,6 +37,7 @@ use PoolWorkArticleViewCurrent;
 use PoolWorkArticleViewOld;
 use TitleFormatter;
 use Wikimedia\Assert\Assert;
+use Wikimedia\Rdbms\ChronologyProtector;
 use Wikimedia\Rdbms\ILBFactory;
 
 /**
@@ -111,6 +112,7 @@ class ParserOutputAccess {
 
 	/** @var ILBFactory */
 	private $lbFactory;
+	private ChronologyProtector $chronologyProtector;
 
 	/** @var LoggerSpi */
 	private $loggerSpi;
@@ -127,6 +129,7 @@ class ParserOutputAccess {
 	 * @param RevisionRenderer $revisionRenderer
 	 * @param IBufferingStatsdDataFactory $statsDataFactory
 	 * @param ILBFactory $lbFactory
+	 * @param ChronologyProtector $chronologyProtector
 	 * @param LoggerSpi $loggerSpi
 	 * @param WikiPageFactory $wikiPageFactory
 	 * @param TitleFormatter $titleFormatter
@@ -137,6 +140,7 @@ class ParserOutputAccess {
 		RevisionRenderer $revisionRenderer,
 		IBufferingStatsdDataFactory $statsDataFactory,
 		ILBFactory $lbFactory,
+		ChronologyProtector $chronologyProtector,
 		LoggerSpi $loggerSpi,
 		WikiPageFactory $wikiPageFactory,
 		TitleFormatter $titleFormatter
@@ -146,6 +150,7 @@ class ParserOutputAccess {
 		$this->revisionRenderer = $revisionRenderer;
 		$this->statsDataFactory = $statsDataFactory;
 		$this->lbFactory = $lbFactory;
+		$this->chronologyProtector = $chronologyProtector;
 		$this->loggerSpi = $loggerSpi;
 		$this->wikiPageFactory = $wikiPageFactory;
 		$this->titleFormatter = $titleFormatter;
@@ -396,6 +401,7 @@ class ParserOutputAccess {
 					$this->revisionRenderer,
 					$primaryCache,
 					$this->lbFactory,
+					$this->chronologyProtector,
 					$this->loggerSpi,
 					$this->wikiPageFactory,
 					!( $options & self::OPT_NO_UPDATE_CACHE ),

includes/poolcounter/PoolWorkArticleViewCurrent.php

@@ -25,6 +25,7 @@ use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Revision\RevisionRenderer;
 use MediaWiki\Status\Status;
 use MediaWiki\Utils\MWTimestamp;
+use Wikimedia\Rdbms\ChronologyProtector;
 use Wikimedia\Rdbms\ILBFactory;
 
 /**
@@ -45,6 +46,7 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 	private $wikiPageFactory;
 	/** @var bool Whether it should trigger an opportunistic LinksUpdate or not */
 	private bool $triggerLinksUpdate;
+	private ChronologyProtector $chronologyProtector;
 
 	/**
 	 * @param string $workKey
@@ -54,6 +56,7 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 	 * @param RevisionRenderer $revisionRenderer
 	 * @param ParserCache $parserCache
 	 * @param ILBFactory $lbFactory
+	 * @param ChronologyProtector $chronologyProtector
 	 * @param LoggerSpi $loggerSpi
 	 * @param WikiPageFactory $wikiPageFactory
 	 * @param bool $cacheable Whether it should store the result in cache or not
@@ -67,6 +70,7 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 		RevisionRenderer $revisionRenderer,
 		ParserCache $parserCache,
 		ILBFactory $lbFactory,
+		ChronologyProtector $chronologyProtector,
 		LoggerSpi $loggerSpi,
 		WikiPageFactory $wikiPageFactory,
 		bool $cacheable = true,
@@ -84,6 +88,7 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 		$this->page = $page;
 		$this->parserCache = $parserCache;
 		$this->lbFactory = $lbFactory;
+		$this->chronologyProtector = $chronologyProtector;
 		$this->wikiPageFactory = $wikiPageFactory;
 		$this->cacheable = $cacheable;
 		$this->triggerLinksUpdate = $triggerLinksUpdate;
@@ -156,7 +161,7 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 			 * the save request, so there is a bias towards avoiding fast stale
 			 * responses of potentially several seconds.
 			 */
-			$lastWriteTime = $this->lbFactory->getChronologyProtectorTouched();
+			$lastWriteTime = $this->chronologyProtector->getTouched( $this->lbFactory->getMainLB() );
 			$cacheTime = MWTimestamp::convert( TS_UNIX, $parserOutput->getCacheTime() );
 			if ( $lastWriteTime && $cacheTime <= $lastWriteTime ) {
 				$logger->info(

tests/phpunit/includes/db/LBFactoryTest.php

@@ -673,7 +673,7 @@ class LBFactoryTest extends MediaWikiIntegrationTestCase {
 
 		$lbFactory = $this->newLBFactoryMulti( [ 'chronologyProtector' => $chronologyProtector ] );
 		$mockWallClock += 1.0;
-		$touched = $lbFactory->getChronologyProtectorTouched();
+		$touched = $chronologyProtector->getTouched( $lbFactory->getMainLB() );
 		$this->assertEquals( $priorTime, $touched );
 	}
 

tests/phpunit/includes/db/LoadBalancerTest.php

@@ -17,6 +17,8 @@
  *
  * @file
  */
+
+use Wikimedia\Rdbms\ChronologyProtector;
 use Wikimedia\Rdbms\Database;
 use Wikimedia\Rdbms\DatabaseDomain;
 use Wikimedia\Rdbms\DBConnRef;
@@ -58,14 +60,19 @@ class LoadBalancerTest extends MediaWikiIntegrationTestCase {
 		global $wgDBname;
 
 		$called = false;
+		$chronologyProtector = $this->createMock( ChronologyProtector::class );
+		$chronologyProtector->method( 'getSessionPrimaryPos' )
+			->willReturnCallback(
+				static function () use ( &$called ) {
+					$called = true;
+				}
+			);
 		$lb = new LoadBalancer( [
 			// Simulate web request with DBO_TRX
 			'servers' => [ $this->makeServerConfig( DBO_TRX ) ],
 			'logger' => MediaWiki\Logger\LoggerFactory::getInstance( 'rdbms' ),
 			'localDomain' => new DatabaseDomain( $wgDBname, null, self::dbPrefix() ),
-			'chronologyCallback' => static function () use ( &$called ) {
-				$called = true;
-			},
+			'chronologyProtector' => $chronologyProtector,
 			'clusterName' => 'xyz'
 		] );
 
@@ -784,14 +791,12 @@ class LoadBalancerTest extends MediaWikiIntegrationTestCase {
 
 	public function testClusterName() {
 		global $wgDBname;
-
+		$chronologyProtector = $this->createMock( ChronologyProtector::class );
 		$lb1 = new LoadBalancer( [
 			'servers' => [ $this->makeServerConfig() ],
 			'logger' => MediaWiki\Logger\LoggerFactory::getInstance( 'rdbms' ),
 			'localDomain' => new DatabaseDomain( $wgDBname, null, self::dbPrefix() ),
-			'chronologyCallback' => static function () use ( &$called ) {
-				$called = true;
-			},
+			'chronologyProtector' => $chronologyProtector,
 			'clusterName' => 'xx'
 		] );
 		$this->assertSame( 'xx', $lb1->getClusterName() );
@@ -800,9 +805,7 @@ class LoadBalancerTest extends MediaWikiIntegrationTestCase {
 			'servers' => [ $this->makeServerConfig() ],
 			'logger' => MediaWiki\Logger\LoggerFactory::getInstance( 'rdbms' ),
 			'localDomain' => new DatabaseDomain( $wgDBname, null, self::dbPrefix() ),
-			'chronologyCallback' => static function () use ( &$called ) {
-				$called = true;
-			},
+			'chronologyProtector' => $chronologyProtector,
 			'clusterName' => null
 		] );
 		$this->assertSame( 'testhost', $lb2->getClusterName() );

tests/phpunit/includes/page/ParserOutputAccessTest.php

@@ -172,6 +172,7 @@ class ParserOutputAccessTest extends MediaWikiIntegrationTestCase {
 			$revRenderer,
 			new NullStatsdDataFactory(),
 			$this->getServiceContainer()->getDBLoadBalancerFactory(),
+			$this->getServiceContainer()->getChronologyProtector(),
 			$this->getLoggerSpi(),
 			$this->getServiceContainer()->getWikiPageFactory(),
 			$this->getServiceContainer()->getTitleFormatter()

tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php

@@ -4,7 +4,7 @@ use MediaWiki\Json\JsonCodec;
 use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Status\Status;
 use Psr\Log\NullLogger;
-use Wikimedia\Rdbms\LBFactory;
+use Wikimedia\Rdbms\ChronologyProtector;
 use Wikimedia\TestingAccessWrapper;
 
 /**
@@ -37,17 +37,16 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 		}
 
 		$parserCache = $this->parserCache ?: $this->installParserCache();
-		$lbFactory = $this->getServiceContainer()->getDBLoadBalancerFactory();
-		$revisionRenderer = $this->getServiceContainer()->getRevisionRenderer();
 
 		return new PoolWorkArticleViewCurrent(
 			'test:' . $rev->getId(),
 			$page,
 			$rev,
 			$options,
-			$revisionRenderer,
+			$this->getServiceContainer()->getRevisionRenderer(),
 			$parserCache,
-			$lbFactory,
+			$this->getServiceContainer()->getDBLoadBalancerFactory(),
+			$this->getServiceContainer()->getChronologyProtector(),
 			$this->getLoggerSpi(),
 			$this->getServiceContainer()->getWikiPageFactory()
 		);
@@ -118,8 +117,8 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 		$lastWrite = 10;
 		$outdated = $lastWrite;
 
-		$lbFactory = $this->createNoOpMock( LBFactory::class, [ 'getChronologyProtectorTouched' ] );
-		$lbFactory->method( 'getChronologyProtectorTouched' )->willReturn( $lastWrite );
+		$chronologyProtector = $this->createNoOpMock( ChronologyProtector::class, [ 'getTouched' ] );
+		$chronologyProtector->method( 'getTouched' )->willReturn( $lastWrite );
 
 		$output = $this->createNoOpMock( ParserOutput::class, [ 'getCacheTime' ] );
 		$output->method( 'getCacheTime' )->willReturn( $outdated );
@@ -130,7 +129,7 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 			$this->createMock( WikiPage::class ),
 			$this->createMock( RevisionRecord::class )
 		);
-		TestingAccessWrapper::newFromObject( $work )->lbFactory = $lbFactory;
+		TestingAccessWrapper::newFromObject( $work )->chronologyProtector = $chronologyProtector;
 
 		$this->assertFalse( $work->fallback( true ) );
 
@@ -145,8 +144,8 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 		$lastWrite = 10;
 		$moreRecent = $lastWrite + 1;
 
-		$lbFactory = $this->createNoOpMock( LBFactory::class, [ 'getChronologyProtectorTouched' ] );
-		$lbFactory->method( 'getChronologyProtectorTouched' )->willReturn( $lastWrite );
+		$chronologyProtector = $this->createNoOpMock( ChronologyProtector::class, [ 'getTouched' ] );
+		$chronologyProtector->method( 'getTouched' )->willReturn( $lastWrite );
 
 		$output = $this->createNoOpMock( ParserOutput::class, [ 'getCacheTime' ] );
 		$output->method( 'getCacheTime' )->willReturn( $moreRecent );
@@ -157,7 +156,7 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 			$this->createMock( WikiPage::class ),
 			$this->createMock( RevisionRecord::class )
 		);
-		TestingAccessWrapper::newFromObject( $work )->lbFactory = $lbFactory;
+		TestingAccessWrapper::newFromObject( $work )->chronologyProtector = $chronologyProtector;
 
 		$status = $work->fallback( true );
 		$this->assertTrue( $status->isOK() );