rdbms: fix callback stage errors in LBFactory::commitMasterChanges

Just like 082ed053b6 fixed pre-commit callback errors when new instances
of LoadBalancer are made during that step, do the same for post-commit
callbacks.

Bug: T194308
Change-Id: Ie79e0f22b3aced425cf067d0df6b67e368223e6c

diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php
index 38c7a5c..097775a 100644 (file)
--- a/includes/libs/rdbms/lbfactory/LBFactory.php
+++ b/includes/libs/rdbms/lbfactory/LBFactory.php
@@ -95,6 +95,8 @@ abstract class LBFactory implements ILBFactory {
        const ROUND_BEGINNING = 'within-begin';
        const ROUND_COMMITTING = 'within-commit';
        const ROUND_ROLLING_BACK = 'within-rollback';
+       const ROUND_COMMIT_CALLBACKS = 'within-commit-callbacks';
+       const ROUND_ROLLBACK_CALLBACKS = 'within-rollback-callbacks';
 
        private static $loggerFields =
                [ 'replLogger', 'connLogger', 'queryLogger', 'perfLogger' ];
@@ -261,6 +263,7 @@ abstract class LBFactory implements ILBFactory {
                // Actually perform the commit on all master DB connections and revert DBO_TRX
                $this->forEachLBCallMethod( 'commitMasterChanges', [ $fname ] );
                // Run all post-commit callbacks in a separate step
+               $this->trxRoundStage = self::ROUND_COMMIT_CALLBACKS;
                $e = $this->executePostTransactionCallbacks();
                $this->trxRoundStage = self::ROUND_CURSORY;
                // Throw any last post-commit callback error
@@ -275,6 +278,7 @@ abstract class LBFactory implements ILBFactory {
                // Actually perform the rollback on all master DB connections and revert DBO_TRX
                $this->forEachLBCallMethod( 'rollbackMasterChanges', [ $fname ] );
                // Run all post-commit callbacks in a separate step
+               $this->trxRoundStage = self::ROUND_ROLLBACK_CALLBACKS;
                $this->executePostTransactionCallbacks();
                $this->trxRoundStage = self::ROUND_CURSORY;
        }
@@ -551,6 +555,14 @@ abstract class LBFactory implements ILBFactory {
         * @return array
         */
        final protected function baseLoadBalancerParams() {
+               if ( $this->trxRoundStage === self::ROUND_COMMIT_CALLBACKS ) {
+                       $initStage = ILoadBalancer::STAGE_POSTCOMMIT_CALLBACKS;
+               } elseif ( $this->trxRoundStage === self::ROUND_ROLLBACK_CALLBACKS ) {
+                       $initStage = ILoadBalancer::STAGE_POSTROLLBACK_CALLBACKS;
+               } else {
+                       $initStage = null;
+               }
+
                return [
                        'localDomain' => $this->localDomain,
                        'readOnlyReason' => $this->readOnlyReason,
@@ -570,7 +582,8 @@ abstract class LBFactory implements ILBFactory {
                                // Defer ChronologyProtector construction in case setRequestInfo() ends up
                                // being called later (but before the first connection attempt) (T192611)
                                $this->getChronologyProtector()->initLB( $lb );
-                       }
+                       },
+                       'roundStage' => $initStage
                ];
        }
 

diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
index 850f9af..81ce4ba 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
@@ -89,6 +89,11 @@ interface ILoadBalancer {
        /** @var int Alias for CONN_TRX_AUTOCOMMIT for b/c; deprecated since 1.31 */
        const CONN_TRX_AUTO = 1;
 
+       /** @var string Manager of ILoadBalancer instances is running post-commit callbacks */
+       const STAGE_POSTCOMMIT_CALLBACKS = 'stage-postcommit-callbacks';
+       /** @var string Manager of ILoadBalancer instances is running post-rollback callbacks */
+       const STAGE_POSTROLLBACK_CALLBACKS = 'stage-postrollback-callbacks';
+
        /**
         * Construct a manager of IDatabase connection objects
         *
@@ -112,6 +117,7 @@ interface ILoadBalancer {
         *  - perfLogger: PSR-3 logger instance. [optional]
         *  - errorLogger : Callback that takes an Exception and logs it. [optional]
         *  - deprecationLogger: Callback to log a deprecation warning. [optional]
+        *  - roundStage: STAGE_POSTCOMMIT_* class constant; for internal use [optional]
         * @throws InvalidArgumentException
         */
        public function __construct( array $params );

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index 405ed14..360be42 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -261,6 +261,14 @@ class LoadBalancer implements ILoadBalancer {
                if ( isset( $params['chronologyCallback'] ) ) {
                        $this->chronologyCallback = $params['chronologyCallback'];
                }
+
+               if ( isset( $params['roundStage'] ) ) {
+                       if ( $params['roundStage'] === self::STAGE_POSTCOMMIT_CALLBACKS ) {
+                               $this->trxRoundStage = self::ROUND_COMMIT_CALLBACKS;
+                       } elseif ( $params['roundStage'] === self::STAGE_POSTROLLBACK_CALLBACKS ) {
+                               $this->trxRoundStage = self::ROUND_ROLLBACK_CALLBACKS;
+                       }
+               }
        }
 
        /**

diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php
index a84cc04..6e23e53 100644 (file)
--- a/tests/phpunit/includes/db/LBFactoryTest.php
+++ b/tests/phpunit/includes/db/LBFactoryTest.php
@@ -153,64 +153,75 @@ class LBFactoryTest extends MediaWikiTestCase {
                $lb->closeAll();
        }
 
-       public function testLBFactoryMulti() {
-               global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir;
+       public function testLBFactoryMultiConns() {
+               $factory = $this->newLBFactoryMultiLBs();
 
-               $factory = new LBFactoryMulti( [
-                       'sectionsByDB' => [
-                               's1wiki' => 's1',
-                       ],
-                       'sectionLoads' => [
-                               's1' => [
-                                       'test-db3' => 0,
-                                       'test-db4' => 100,
-                               ],
-                               'DEFAULT' => [
-                                       'test-db1' => 0,
-                                       'test-db2' => 100,
-                               ]
-                       ],
-                       'serverTemplate' => [
-                               'dbname'      => $wgDBname,
-                               'user'        => $wgDBuser,
-                               'password'    => $wgDBpassword,
-                               'type'        => $wgDBtype,
-                               'dbDirectory' => $wgSQLiteDataDir,
-                               'flags'       => DBO_DEFAULT
-                       ],
-                       'hostsByName' => [
-                               'test-db1'  => $wgDBserver,
-                               'test-db2'  => $wgDBserver,
-                               'test-db3'  => $wgDBserver,
-                               'test-db4'  => $wgDBserver
-                       ],
-                       'loadMonitorClass' => LoadMonitorNull::class
-               ] );
-               $lb = $factory->getMainLB();
-
-               $dbw = $lb->getConnection( DB_MASTER );
+               $dbw = $factory->getMainLB()->getConnection( DB_MASTER );
                $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' );
 
-               $dbr = $lb->getConnection( DB_REPLICA );
+               $dbr = $factory->getMainLB()->getConnection( DB_REPLICA );
                $this->assertTrue( $dbr->getLBInfo( 'replica' ), 'slave shows as slave' );
 
-               // Test that LoadBalancer instances made during commitMasterChanges() do not throw
-               // DBTransactionError due to transaction ROUND_* stages being mismatched.
+               // Destructor should trigger without round stage errors
+               unset( $factory );
+       }
+
+       public function testLBFactoryMultiRoundCallbacks() {
+               $called = 0;
+               $countLBsFunc = function ( LBFactoryMulti $factory ) {
+                       $count = 0;
+                       $factory->forEachLB( function () use ( &$count ) {
+                               ++$count;
+                       } );
+
+                       return $count;
+               };
+
+               $factory = $this->newLBFactoryMultiLBs();
+               $this->assertEquals( 0, $countLBsFunc( $factory ) );
+               $dbw = $factory->getMainLB()->getConnection( DB_MASTER );
+               $this->assertEquals( 1, $countLBsFunc( $factory ) );
+               // Test that LoadBalancer instances made during pre-commit callbacks in do not
+               // throw DBTransactionError due to transaction ROUND_* stages being mismatched.
                $factory->beginMasterChanges( __METHOD__ );
-               $dbw->onTransactionPreCommitOrIdle( function () use ( $factory ) {
+               $dbw->onTransactionPreCommitOrIdle( function () use ( $factory, &$called ) {
+                       ++$called;
                        // Trigger s1 LoadBalancer instantiation during "finalize" stage.
                        // There is no s1wiki DB to select so it is not in getConnection(),
                        // but this fools getMainLB() at least.
                        $factory->getMainLB( 's1wiki' )->getConnection( DB_MASTER );
                } );
                $factory->commitMasterChanges( __METHOD__ );
+               $this->assertEquals( 1, $called );
+               $this->assertEquals( 2, $countLBsFunc( $factory ) );
+               $factory->shutdown();
+               $factory->closeAll();
 
-               $count = 0;
-               $factory->forEachLB( function () use ( &$count ) {
-                       ++$count;
+               $called = 0;
+               $factory = $this->newLBFactoryMultiLBs();
+               $this->assertEquals( 0, $countLBsFunc( $factory ) );
+               $dbw = $factory->getMainLB()->getConnection( DB_MASTER );
+               $this->assertEquals( 1, $countLBsFunc( $factory ) );
+               // Test that LoadBalancer instances made during pre-commit callbacks in do not
+               // throw DBTransactionError due to transaction ROUND_* stages being mismatched.hrow
+               // DBTransactionError due to transaction ROUND_* stages being mismatched.
+               $factory->beginMasterChanges( __METHOD__ );
+               $dbw->query( "SELECT 1 as t", __METHOD__ );
+               $dbw->onTransactionResolution( function () use ( $factory, &$called ) {
+                       ++$called;
+                       // Trigger s1 LoadBalancer instantiation during "finalize" stage.
+                       // There is no s1wiki DB to select so it is not in getConnection(),
+                       // but this fools getMainLB() at least.
+                       $factory->getMainLB( 's1wiki' )->getConnection( DB_MASTER );
                } );
-               $this->assertEquals( 2, $count );
+               $factory->commitMasterChanges( __METHOD__ );
+               $this->assertEquals( 1, $called );
+               $this->assertEquals( 2, $countLBsFunc( $factory ) );
+               $factory->shutdown();
+               $factory->closeAll();
 
+               $factory = $this->newLBFactoryMultiLBs();
+               $dbw = $factory->getMainLB()->getConnection( DB_MASTER );
                // DBTransactionError should not be thrown
                $ran = 0;
                $dbw->onTransactionPreCommitOrIdle( function () use ( &$ran ) {
@@ -223,6 +234,41 @@ class LBFactoryTest extends MediaWikiTestCase {
                $factory->closeAll();
        }
 
+       private function newLBFactoryMultiLBs() {
+               global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir;
+
+               return new LBFactoryMulti( [
+                       'sectionsByDB' => [
+                               's1wiki' => 's1',
+                       ],
+                       'sectionLoads' => [
+                               's1' => [
+                                       'test-db3' => 0,
+                                       'test-db4' => 100,
+                               ],
+                               'DEFAULT' => [
+                                       'test-db1' => 0,
+                                       'test-db2' => 100,
+                               ]
+                       ],
+                       'serverTemplate' => [
+                               'dbname' => $wgDBname,
+                               'user' => $wgDBuser,
+                               'password' => $wgDBpassword,
+                               'type' => $wgDBtype,
+                               'dbDirectory' => $wgSQLiteDataDir,
+                               'flags' => DBO_DEFAULT
+                       ],
+                       'hostsByName' => [
+                               'test-db1' => $wgDBserver,
+                               'test-db2' => $wgDBserver,
+                               'test-db3' => $wgDBserver,
+                               'test-db4' => $wgDBserver
+                       ],
+                       'loadMonitorClass' => LoadMonitorNull::class
+               ] );
+       }
+
        /**
         * @covers \Wikimedia\Rdbms\ChronologyProtector
         */