From ef949d79d2810bb0ed14fc64f6d997ec16097f57 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 21 May 2019 14:52:57 -0700 Subject: [PATCH] Introduce TransactionRoundAwareUpdate interface Refactor the deferred update transaction round handling code to use a new TransactionRoundAwareUpdate interface. Also, rename a few DeferredUpdates methods so they do not give the impression that doUpdates() is always called. Change-Id: Idc4c6d81c4e2ca0ce41bca1e7800f797fa7e37f6 --- autoload.php | 1 + includes/deferred/DeferredUpdates.php | 55 +++++++++++++------ .../deferred/TransactionRoundAwareUpdate.php | 19 +++++++ .../TransactionRoundDefiningUpdate.php | 12 +++- .../includes/deferred/DeferredUpdatesTest.php | 22 ++++---- 5 files changed, 81 insertions(+), 28 deletions(-) create mode 100644 includes/deferred/TransactionRoundAwareUpdate.php diff --git a/autoload.php b/autoload.php index 275d20e886..c522198dd9 100644 --- a/autoload.php +++ b/autoload.php @@ -1496,6 +1496,7 @@ $wgAutoloadLocalClasses = [ 'TrackBlobs' => __DIR__ . '/maintenance/storage/trackBlobs.php', 'TrackingCategories' => __DIR__ . '/includes/TrackingCategories.php', 'TraditionalImageGallery' => __DIR__ . '/includes/gallery/TraditionalImageGallery.php', + 'TransactionRoundAwareUpdate' => __DIR__ . '/includes/deferred/TransactionRoundAwareUpdate.php', 'TransactionRoundDefiningUpdate' => __DIR__ . '/includes/deferred/TransactionRoundDefiningUpdate.php', 'TransformParameterError' => __DIR__ . '/includes/media/TransformParameterError.php', 'TransformTooBigImageAreaError' => __DIR__ . '/includes/media/TransformTooBigImageAreaError.php', diff --git a/includes/deferred/DeferredUpdates.php b/includes/deferred/DeferredUpdates.php index 3b7de9df80..4b54378182 100644 --- a/includes/deferred/DeferredUpdates.php +++ b/includes/deferred/DeferredUpdates.php @@ -22,6 +22,7 @@ use Wikimedia\Rdbms\IDatabase; use MediaWiki\MediaWikiServices; use Wikimedia\Rdbms\LBFactory; +use Wikimedia\Rdbms\ILBFactory; use Wikimedia\Rdbms\LoadBalancer; /** @@ -136,11 +137,11 @@ class DeferredUpdates { // Normally, these use the subqueue, but that isn't true for MergeableUpdate items. do { if ( $stage === self::ALL || $stage === self::PRESEND ) { - self::execute( self::$preSendUpdates, $mode, $stageEffective ); + self::handleUpdateQueue( self::$preSendUpdates, $mode, $stageEffective ); } if ( $stage === self::ALL || $stage == self::POSTSEND ) { - self::execute( self::$postSendUpdates, $mode, $stageEffective ); + self::handleUpdateQueue( self::$postSendUpdates, $mode, $stageEffective ); } } while ( $stage === self::ALL && self::$preSendUpdates ); } @@ -169,15 +170,15 @@ class DeferredUpdates { } /** - * Immediately run/queue a list of updates + * Immediately run or enqueue a list of updates * * @param DeferrableUpdate[] &$queue List of DeferrableUpdate objects - * @param string $mode Use "enqueue" to use the job queue when possible + * @param string $mode Either "run" or "enqueue" (to use the job queue when possible) * @param int $stage Class constant (PRESEND, POSTSEND) (since 1.28) * @throws ErrorPageError Happens on top-level calls * @throws Exception Happens on second-level calls */ - protected static function execute( array &$queue, $mode, $stage ) { + protected static function handleUpdateQueue( array &$queue, $mode, $stage ) { $services = MediaWikiServices::getInstance(); $stats = $services->getStatsdDataFactory(); $lbFactory = $services->getDBLoadBalancerFactory(); @@ -216,7 +217,7 @@ class DeferredUpdates { self::$executeContext = [ 'stage' => $stage, 'subqueue' => [] ]; try { /** @var DeferrableUpdate $update */ - $guiError = self::runUpdate( $update, $lbFactory, $mode, $stage ); + $guiError = self::handleUpdate( $update, $lbFactory, $mode, $stage ); $reportableError = $reportableError ?: $guiError; // Do the subqueue updates for $update until there are none while ( self::$executeContext['subqueue'] ) { @@ -228,7 +229,7 @@ class DeferredUpdates { $subUpdate->setTransactionTicket( $ticket ); } - $guiError = self::runUpdate( $subUpdate, $lbFactory, $mode, $stage ); + $guiError = self::handleUpdate( $subUpdate, $lbFactory, $mode, $stage ); $reportableError = $reportableError ?: $guiError; } } finally { @@ -249,13 +250,15 @@ class DeferredUpdates { } /** + * Run or enqueue an update + * * @param DeferrableUpdate $update * @param LBFactory $lbFactory * @param string $mode * @param int $stage * @return ErrorPageError|null */ - private static function runUpdate( + private static function handleUpdate( DeferrableUpdate $update, LBFactory $lbFactory, $mode, $stage ) { $guiError = null; @@ -265,21 +268,15 @@ class DeferredUpdates { $spec = $update->getAsJobSpecification(); $domain = $spec['domain'] ?? $spec['wiki']; JobQueueGroup::singleton( $domain )->push( $spec['job'] ); - } elseif ( $update instanceof TransactionRoundDefiningUpdate ) { - $update->doUpdate(); } else { - // Run the bulk of the update now - $fnameTrxOwner = get_class( $update ) . '::doUpdate'; - $lbFactory->beginMasterChanges( $fnameTrxOwner ); - $update->doUpdate(); - $lbFactory->commitMasterChanges( $fnameTrxOwner ); + self::attemptUpdate( $update, $lbFactory ); } } catch ( Exception $e ) { // Reporting GUI exceptions does not work post-send if ( $e instanceof ErrorPageError && $stage === self::PRESEND ) { $guiError = $e; } - MWExceptionHandler::rollbackMasterChangesAndLog( $e ); + $lbFactory->rollbackMasterChanges( __METHOD__ ); // VW-style hack to work around T190178, so we can make sure // PageMetaDataUpdater doesn't throw exceptions. @@ -291,6 +288,32 @@ class DeferredUpdates { return $guiError; } + /** + * Attempt to run an update with the appropriate transaction round state it expects + * + * DeferredUpdate classes that wrap the execution of bundles of other DeferredUpdate + * instances can use this method to run the updates. Any such wrapper class should + * always use TRX_ROUND_ABSENT itself. + * + * @param DeferrableUpdate $update + * @param ILBFactory $lbFactory + * @since 1.34 + */ + public static function attemptUpdate( DeferrableUpdate $update, ILBFactory $lbFactory ) { + if ( + $update instanceof TransactionRoundAwareUpdate && + $update->getTransactionRoundRequirement() == $update::TRX_ROUND_ABSENT + ) { + $update->doUpdate(); + } else { + // Run the bulk of the update now + $fnameTrxOwner = get_class( $update ) . '::doUpdate'; + $lbFactory->beginMasterChanges( $fnameTrxOwner ); + $update->doUpdate(); + $lbFactory->commitMasterChanges( $fnameTrxOwner ); + } + } + /** * Run all deferred updates immediately if there are no DB writes active * diff --git a/includes/deferred/TransactionRoundAwareUpdate.php b/includes/deferred/TransactionRoundAwareUpdate.php new file mode 100644 index 0000000000..74303f5036 --- /dev/null +++ b/includes/deferred/TransactionRoundAwareUpdate.php @@ -0,0 +1,19 @@ +fname; } + + /** + * @return int One of the class TRX_ROUND_* constants + * @since 1.34 + */ + final public function getTransactionRoundRequirement() { + return self::TRX_ROUND_ABSENT; + } } diff --git a/tests/phpunit/includes/deferred/DeferredUpdatesTest.php b/tests/phpunit/includes/deferred/DeferredUpdatesTest.php index 3662c26646..b377c639f9 100644 --- a/tests/phpunit/includes/deferred/DeferredUpdatesTest.php +++ b/tests/phpunit/includes/deferred/DeferredUpdatesTest.php @@ -8,8 +8,8 @@ class DeferredUpdatesTest extends MediaWikiTestCase { * @covers DeferredUpdates::addUpdate * @covers DeferredUpdates::push * @covers DeferredUpdates::doUpdates - * @covers DeferredUpdates::execute - * @covers DeferredUpdates::runUpdate + * @covers DeferredUpdates::handleUpdateQueue + * @covers DeferredUpdates::attemptUpdate */ public function testAddAndRun() { $update = $this->getMockBuilder( DeferrableUpdate::class ) @@ -92,7 +92,7 @@ class DeferredUpdatesTest extends MediaWikiTestCase { /** * @covers DeferredUpdates::doUpdates - * @covers DeferredUpdates::execute + * @covers DeferredUpdates::handleUpdateQueue * @covers DeferredUpdates::addUpdate */ public function testDoUpdatesWeb() { @@ -189,7 +189,7 @@ class DeferredUpdatesTest extends MediaWikiTestCase { /** * @covers DeferredUpdates::doUpdates - * @covers DeferredUpdates::execute + * @covers DeferredUpdates::handleUpdateQueue * @covers DeferredUpdates::addUpdate */ public function testDoUpdatesCLI() { @@ -263,7 +263,7 @@ class DeferredUpdatesTest extends MediaWikiTestCase { /** * @covers DeferredUpdates::doUpdates - * @covers DeferredUpdates::execute + * @covers DeferredUpdates::handleUpdateQueue * @covers DeferredUpdates::addUpdate */ public function testPresendAddOnPostsendRun() { @@ -295,7 +295,7 @@ class DeferredUpdatesTest extends MediaWikiTestCase { } /** - * @covers DeferredUpdates::runUpdate + * @covers DeferredUpdates::attemptUpdate */ public function testRunUpdateTransactionScope() { $this->setMwGlobals( 'wgCommandLineMode', false ); @@ -315,7 +315,7 @@ class DeferredUpdatesTest extends MediaWikiTestCase { } /** - * @covers DeferredUpdates::runUpdate + * @covers DeferredUpdates::attemptUpdate * @covers TransactionRoundDefiningUpdate::getOrigin */ public function testRunOuterScopeUpdate() { @@ -326,10 +326,10 @@ class DeferredUpdatesTest extends MediaWikiTestCase { $ran = 0; DeferredUpdates::addUpdate( new TransactionRoundDefiningUpdate( - function () use ( &$ran, $lbFactory ) { - $ran++; - $this->assertFalse( $lbFactory->hasTransactionRound(), 'No transaction' ); - } ) + function () use ( &$ran, $lbFactory ) { + $ran++; + $this->assertFalse( $lbFactory->hasTransactionRound(), 'No transaction' ); + } ) ); DeferredUpdates::doUpdates(); -- 2.20.1