From a79b9737f1b05171af60a3127d6c628ea6a16a96 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 2 May 2018 16:56:07 -0700 Subject: [PATCH] Make DeferredUpdates avoid running during LBFactory::commitMasterChanges Bug: T193668 Change-Id: I50890ef17ea72481a14c4abcd93ae58b93f15d28 --- includes/deferred/DeferredUpdates.php | 20 ++++++---- includes/libs/rdbms/lbfactory/ILBFactory.php | 12 +++++- includes/libs/rdbms/lbfactory/LBFactory.php | 4 ++ .../includes/deferred/DeferredUpdatesTest.php | 38 +++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/includes/deferred/DeferredUpdates.php b/includes/deferred/DeferredUpdates.php index ef46953432..8543c4b1f4 100644 --- a/includes/deferred/DeferredUpdates.php +++ b/includes/deferred/DeferredUpdates.php @@ -36,11 +36,14 @@ use Wikimedia\Rdbms\LoadBalancer; * Updates that work through this system will be more likely to complete by the time the client * makes their next request after this one than with the JobQueue system. * - * In CLI mode, updates run immediately if no DB writes are pending. Otherwise, they run when: - * - a) Any waitForReplication() call if no writes are pending on any DB - * - b) A commit happens on Maintenance::getDB( DB_MASTER ) if no writes are pending on any DB - * - c) EnqueueableDataUpdate tasks may enqueue on commit of Maintenance::getDB( DB_MASTER ) - * - d) At the completion of Maintenance::execute() + * In CLI mode, deferred updates will run: + * - a) During DeferredUpdates::addUpdate if no LBFactory DB handles have writes pending + * - b) On commit of an LBFactory DB handle if no other such handles have writes pending + * - c) During an LBFactory::waitForReplication call if no LBFactory DBs have writes pending + * - d) When the queue is large and an LBFactory DB handle commits (EnqueueableDataUpdate only) + * - e) At the completion of Maintenance::execute() + * + * @see Maintenance::setLBFactoryTriggers * * When updates are deferred, they go into one two FIFO "top-queues" (one for pre-send and one * for post-send). Updates enqueued *during* doUpdate() of a "top" update go into the "sub-queue" @@ -285,8 +288,9 @@ class DeferredUpdates { /** * Run all deferred updates immediately if there are no DB writes active * - * If $mode is 'run' but there are busy databates, EnqueueableDataUpdate - * tasks will be enqueued anyway for the sake of progress. + * If there are many deferred updates pending, $mode is 'run', and there + * are still busy LBFactory database handles, then any EnqueueableDataUpdate + * tasks might be enqueued as jobs to be executed later. * * @param string $mode Use "enqueue" to use the job queue when possible * @return bool Whether updates were allowed to run @@ -373,7 +377,7 @@ class DeferredUpdates { */ private static function areDatabaseTransactionsActive() { $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); - if ( $lbFactory->hasTransactionRound() ) { + if ( $lbFactory->hasTransactionRound() || !$lbFactory->isReadyForRoundOperations() ) { return true; } diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php index 1e8838e71b..45e7cbb756 100644 --- a/includes/libs/rdbms/lbfactory/ILBFactory.php +++ b/includes/libs/rdbms/lbfactory/ILBFactory.php @@ -195,12 +195,22 @@ interface ILBFactory { public function rollbackMasterChanges( $fname = __METHOD__ ); /** - * Check if a transaction round is active + * Check if an explicit transaction round is active * @return bool * @since 1.29 */ public function hasTransactionRound(); + /** + * Check if transaction rounds can be started, committed, or rolled back right now + * + * This can be used as a recusion guard to avoid exceptions in transaction callbacks + * + * @return bool + * @since 1.32 + */ + public function isReadyForRoundOperations(); + /** * Determine if any master connection has pending changes * @return bool diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index ca684c329a..ccaebd3cd4 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -287,6 +287,10 @@ abstract class LBFactory implements ILBFactory { return ( $this->trxRoundId !== false ); } + public function isReadyForRoundOperations() { + return ( $this->trxRoundStage === self::ROUND_CURSORY ); + } + /** * Log query info if multi DB transactions are going to be committed now */ diff --git a/tests/phpunit/includes/deferred/DeferredUpdatesTest.php b/tests/phpunit/includes/deferred/DeferredUpdatesTest.php index 6b41707333..a1e41d97e8 100644 --- a/tests/phpunit/includes/deferred/DeferredUpdatesTest.php +++ b/tests/phpunit/includes/deferred/DeferredUpdatesTest.php @@ -335,4 +335,42 @@ class DeferredUpdatesTest extends MediaWikiTestCase { $this->assertSame( 1, $ran, 'Update ran' ); } + + /** + * @covers DeferredUpdates::tryOpportunisticExecute + */ + public function testTryOpportunisticExecute() { + $calls = []; + $callback1 = function () use ( &$calls ) { + $calls[] = 1; + }; + $callback2 = function () use ( &$calls ) { + $calls[] = 2; + }; + + $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); + $lbFactory->beginMasterChanges( __METHOD__ ); + + DeferredUpdates::addCallableUpdate( $callback1 ); + $this->assertEquals( [], $calls ); + + DeferredUpdates::tryOpportunisticExecute( 'run' ); + $this->assertEquals( [], $calls ); + + $dbw = wfGetDB( DB_MASTER ); + $dbw->onTransactionIdle( function () use ( &$calls, $callback2 ) { + DeferredUpdates::addCallableUpdate( $callback2 ); + $this->assertEquals( [], $calls ); + $calls[] = 'oti'; + } ); + $this->assertEquals( 1, $dbw->trxLevel() ); + $this->assertEquals( [], $calls ); + + $lbFactory->commitMasterChanges( __METHOD__ ); + + $this->assertEquals( [ 'oti' ], $calls ); + + DeferredUpdates::tryOpportunisticExecute( 'run' ); + $this->assertEquals( [ 'oti', 1, 2 ], $calls ); + } } -- 2.20.1