From 70dcaba317840972ce388f53c6ab56d2fdb4cea2 Mon Sep 17 00:00:00 2001 From: Amir Sarabadani Date: Fri, 1 Sep 2023 12:42:14 +0200 Subject: [PATCH] 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 | 3 ++- includes/ServiceWiring.php | 1 + includes/libs/rdbms/lbfactory/ILBFactory.php | 8 ------- includes/libs/rdbms/lbfactory/LBFactory.php | 10 +------- .../loadbalancer/ILoadBalancerForOwner.php | 4 +--- .../libs/rdbms/loadbalancer/LoadBalancer.php | 18 +++++++-------- includes/page/ParserOutputAccess.php | 6 +++++ .../PoolWorkArticleViewCurrent.php | 7 +++++- tests/phpunit/includes/db/LBFactoryTest.php | 2 +- .../phpunit/includes/db/LoadBalancerTest.php | 23 +++++++++++-------- .../includes/page/ParserOutputAccessTest.php | 1 + .../PoolWorkArticleViewCurrentTest.php | 21 ++++++++--------- 12 files changed, 51 insertions(+), 53 deletions(-) diff --git a/RELEASE-NOTES-1.41 b/RELEASE-NOTES-1.41 index b6e32359ff6..5d6c43ccedb 100644 --- a/RELEASE-NOTES-1.41 +++ b/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() diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index f1246b78950..2618f885aaf 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -1460,6 +1460,7 @@ return [ $services->getRevisionRenderer(), $services->getStatsdDataFactory(), $services->getDBLoadBalancerFactory(), + $services->getChronologyProtector(), LoggerFactory::getProvider(), $services->getWikiPageFactory(), $services->getTitleFormatter() diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index c3b58d97972..2604d249463 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/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 * diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index c07f725c288..691958e063b 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/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 ]; diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancerForOwner.php b/includes/libs/rdbms/loadbalancer/ILoadBalancerForOwner.php index 440d2e15e74..079c6f154cb 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancerForOwner.php +++ b/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] diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 7354d309e4e..93b5efe9187 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/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 ) ) { diff --git a/includes/page/ParserOutputAccess.php b/includes/page/ParserOutputAccess.php index 2119d10a7eb..b88bf472eb3 100644 --- a/includes/page/ParserOutputAccess.php +++ b/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 ), diff --git a/includes/poolcounter/PoolWorkArticleViewCurrent.php b/includes/poolcounter/PoolWorkArticleViewCurrent.php index 977056b1776..7b69aa67c44 100644 --- a/includes/poolcounter/PoolWorkArticleViewCurrent.php +++ b/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( diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 0dda3112fdd..79efe1b39f8 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/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 ); } diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index c0973fc000b..ca34056f17a 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/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() ); diff --git a/tests/phpunit/includes/page/ParserOutputAccessTest.php b/tests/phpunit/includes/page/ParserOutputAccessTest.php index adf7f19ce23..6d5819578a5 100644 --- a/tests/phpunit/includes/page/ParserOutputAccessTest.php +++ b/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() diff --git a/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php b/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php index fe0e5507020..7c62cdc9f40 100644 --- a/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php +++ b/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() );