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)

diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index da3f320..f97d60d 100644 (file)
--- 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 82d9c1d..79f787d 100644 (file)
--- 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 19b8fdc..c272147 100644 (file)
--- 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 2f493c7..fec496e 100644 (file)
--- 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 01589ae..51fdfe3 100644 (file)
--- 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 7e58555..e054569 100644 (file)
--- 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 );