From c91a71c5f608675df6205a945bdc4be29d81d21e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 20 Apr 2018 13:45:49 -0700 Subject: [PATCH] rdbms: make sure cpPosIndex cookie is applied to LBFactory in time Once getMain() was called in setSchemaAliases(), the ChronologyProtector was initialized and the setRequestInfo() call in Setup.php had no effect. Only the request values read in LBFactory::__construct() were used, which reflect $_GET but not cookie values. Use the $wgDBtype variable to avoid this and add an exception when that sort of thing happens. Further defer instantiation of ChronologyProtector so that methods like ILBFactory::getMainLB() do not trigger construction. Bug: T192611 Change-Id: I735d3ade5cd12a5d609f4dae19ac88fec4b18b51 (cherry picked from commit 628a3a9b267620914701a2a0a17bad8ab2e56498) --- includes/ServiceWiring.php | 2 +- includes/db/MWLBFactory.php | 6 ++--- includes/libs/rdbms/lbfactory/LBFactory.php | 11 +++++++++- .../libs/rdbms/loadbalancer/ILoadBalancer.php | 2 +- .../libs/rdbms/loadbalancer/LoadBalancer.php | 22 +++++++++---------- .../phpunit/includes/db/LoadBalancerTest.php | 8 ++++++- 6 files changed, 32 insertions(+), 19 deletions(-) diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index da3f3209d8..f97d60df02 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -62,7 +62,7 @@ return [ $class = MWLBFactory::getLBFactoryClass( $lbConf ); $instance = new $class( $lbConf ); - MWLBFactory::setSchemaAliases( $instance ); + MWLBFactory::setSchemaAliases( $instance, $mainConfig ); return $instance; }, diff --git a/includes/db/MWLBFactory.php b/includes/db/MWLBFactory.php index 82d9c1d564..79f787dba4 100644 --- a/includes/db/MWLBFactory.php +++ b/includes/db/MWLBFactory.php @@ -208,10 +208,8 @@ abstract class MWLBFactory { return $class; } - public static function setSchemaAliases( LBFactory $lbFactory ) { - $mainLB = $lbFactory->getMainLB(); - $masterType = $mainLB->getServerType( $mainLB->getWriterIndex() ); - if ( $masterType === 'mysql' ) { + public static function setSchemaAliases( LBFactory $lbFactory, Config $config ) { + if ( $config->get( 'DBtype' ) === 'mysql' ) { /** * When SQLite indexes were introduced in r45764, it was noted that * SQLite requires index names to be unique within the whole database, diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 19b8fdc238..c272147639 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -30,6 +30,7 @@ use EmptyBagOStuff; use WANObjectCache; use Exception; use RuntimeException; +use LogicException; /** * An interface for generating database load balancers @@ -527,7 +528,11 @@ abstract class LBFactory implements ILBFactory { 'hostname' => $this->hostname, 'cliMode' => $this->cliMode, 'agent' => $this->agent, - 'chronologyProtector' => $this->getChronologyProtector() + 'chronologyCallback' => function ( ILoadBalancer $lb ) { + // Defer ChronologyProtector construction in case setRequestInfo() ends up + // being called later (but before the first connection attempt) (T192611) + $this->getChronologyProtector()->initLB( $lb ); + } ]; } @@ -585,6 +590,10 @@ abstract class LBFactory implements ILBFactory { } public function setRequestInfo( array $info ) { + if ( $this->chronProt ) { + throw new LogicException( 'ChronologyProtector already initialized.' ); + } + $this->requestInfo = $info + $this->requestInfo; } diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 2f493c7dd8..fec496e455 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -101,7 +101,7 @@ interface ILoadBalancer { * - maxLag: Avoid replica DB servers with more lag than this [optional] * - srvCache : BagOStuff object for server cache [optional] * - wanCache : WANObjectCache object [optional] - * - chronologyProtector: ChronologyProtector object [optional] + * - chronologyCallback: Callback to run before the first connection attempt [optional] * - hostname : The name of the current server [optional] * - cliMode: Whether the execution context is a CLI script. [optional] * - profiler : Class name or instance with profileIn()/profileOut() methods. [optional] diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 01589ae4c2..51fdfe308a 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -59,8 +59,8 @@ class LoadBalancer implements ILoadBalancer { /** @var ILoadMonitor */ private $loadMonitor; - /** @var ChronologyProtector|null */ - private $chronProt; + /** @var callable|null Callback to run before the first connection attempt */ + private $chronologyCallback; /** @var BagOStuff */ private $srvCache; /** @var WANObjectCache */ @@ -116,8 +116,8 @@ class LoadBalancer implements ILoadBalancer { /** @var bool */ private $disabled = false; - /** @var bool */ - private $chronProtInitialized = false; + /** @var bool Whether any connection has been attempted yet */ + private $connectionAttempted = false; /** @var int */ private $maxLag = self::MAX_LAG_DEFAULT; @@ -243,8 +243,8 @@ class LoadBalancer implements ILoadBalancer { : ( PHP_SAPI === 'cli' || PHP_SAPI === 'phpdbg' ); $this->agent = isset( $params['agent'] ) ? $params['agent'] : ''; - if ( isset( $params['chronologyProtector'] ) ) { - $this->chronProt = $params['chronologyProtector']; + if ( isset( $params['chronologyCallback'] ) ) { + $this->chronologyCallback = $params['chronologyCallback']; } } @@ -424,7 +424,7 @@ class LoadBalancer implements ILoadBalancer { } else { $i = false; if ( $this->waitForPos && $this->waitForPos->asOfTime() ) { - // ChronologyProtecter sets "waitForPos" for session consistency. + // "chronologyCallback" 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(); @@ -849,11 +849,11 @@ class LoadBalancer implements ILoadBalancer { $domain = false; // local connection requested } - if ( !$this->chronProtInitialized && $this->chronProt ) { + if ( !$this->connectionAttempted && $this->chronologyCallback ) { $this->connLogger->debug( __METHOD__ . ': calling initLB() before first connection.' ); - // Load CP positions before connecting so that doWait() triggers later if needed - $this->chronProtInitialized = true; - $this->chronProt->initLB( $this ); + // Load any "waitFor" positions before connecting so that doWait() is triggered + $this->connectionAttempted = true; + call_user_func( $this->chronologyCallback, $this ); } // Check if an auto-commit connection is being requested. If so, it will not reuse the diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index 7e58555550..e054569d03 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -51,17 +51,23 @@ class LoadBalancerTest extends MediaWikiTestCase { public function testWithoutReplica() { global $wgDBname; + $called = false; $lb = new LoadBalancer( [ 'servers' => [ $this->makeServerConfig() ], 'queryLogger' => MediaWiki\Logger\LoggerFactory::getInstance( 'DBQuery' ), - 'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ) + 'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ), + 'chronologyCallback' => function () use ( &$called ) { + $called = true; + } ] ); $ld = DatabaseDomain::newFromId( $lb->getLocalDomainID() ); $this->assertEquals( $wgDBname, $ld->getDatabase(), 'local domain DB set' ); $this->assertEquals( $this->dbPrefix(), $ld->getTablePrefix(), 'local domain prefix set' ); + $this->assertFalse( $called ); $dbw = $lb->getConnection( DB_MASTER ); + $this->assertTrue( $called ); $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' ); $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX set on master" ); $this->assertWriteAllowed( $dbw ); -- 2.20.1