From 86af2ef383b6fc9c4032dad769c00e672922d530 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 9 May 2018 21:14:40 -0700 Subject: [PATCH] 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 --- includes/libs/rdbms/lbfactory/LBFactory.php | 15 +- .../libs/rdbms/loadbalancer/ILoadBalancer.php | 6 + .../libs/rdbms/loadbalancer/LoadBalancer.php | 8 ++ tests/phpunit/includes/db/LBFactoryTest.php | 132 ++++++++++++------ 4 files changed, 117 insertions(+), 44 deletions(-) diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 38c7a5c7e9..097775afc9 100644 --- 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 850f9afab8..81ce4baeaf 100644 --- 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 405ed14160..360be4256e 100644 --- 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 a84cc04514..6e23e53639 100644 --- 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 */ -- 2.20.1