Make sure that each DataUpdate still has outer transaction scope

Bug: T221577
Change-Id: I620e461d791416ca37fa9ca4fca501e28d778cf5

diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php
index ff5541d..53fe615 100644 (file)
--- a/includes/Storage/DerivedPageDataUpdater.php
+++ b/includes/Storage/DerivedPageDataUpdater.php
@@ -48,6 +48,7 @@ use MediaWiki\Revision\SlotRoleRegistry;
 use MediaWiki\Revision\SlotRecord;
 use MediaWiki\User\UserIdentity;
 use MessageCache;
+use MWCallableUpdate;
 use ParserCache;
 use ParserOptions;
 use ParserOutput;
@@ -1423,12 +1424,18 @@ class DerivedPageDataUpdater implements IDBAccessObject {
                }
 
                // Defer the getCannonicalParserOutput() call triggered by getSecondaryDataUpdates()
-               DeferredUpdates::addCallableUpdate( function () {
-                       $this->doSecondaryDataUpdates( [
-                               // T52785 do not update any other pages on a null edit
-                               'recursive' => $this->options['changed']
-                       ] );
-               } );
+               // by wrapping the code that schedules the secondary updates in a callback itself
+               $wrapperUpdate = new MWCallableUpdate(
+                       function () {
+                               $this->doSecondaryDataUpdates( [
+                                       // T52785 do not update any other pages on a null edit
+                                       'recursive' => $this->options['changed']
+                               ] );
+                       },
+                       __METHOD__
+               );
+               $wrapperUpdate->setTransactionRoundRequirement( $wrapperUpdate::TRX_ROUND_ABSENT );
+               DeferredUpdates::addUpdate( $wrapperUpdate );
 
                // TODO: MCR: check if *any* changed slot supports categories!
                if ( $this->rcWatchCategoryMembership
@@ -1569,24 +1576,15 @@ class DerivedPageDataUpdater implements IDBAccessObject {
         *     current page or otherwise depend on it (default: false)
         *   - defer: one of the DeferredUpdates constants, or false to run immediately after waiting
         *     for replication of the changes from the SecondaryDataUpdates hooks (default: false)
-        *   - transactionTicket: a transaction ticket from LBFactory::getEmptyTransactionTicket(),
-        *     only when defer is false (default: null)
         * @since 1.32
         */
        public function doSecondaryDataUpdates( array $options = [] ) {
                $this->assertHasRevision( __METHOD__ );
-               $options += [
-                       'recursive' => false,
-                       'defer' => false,
-                       'transactionTicket' => null,
-               ];
+               $options += [ 'recursive' => false, 'defer' => false ];
                $deferValues = [ false, DeferredUpdates::PRESEND, DeferredUpdates::POSTSEND ];
                if ( !in_array( $options['defer'], $deferValues, true ) ) {
                        throw new InvalidArgumentException( 'Invalid value for defer: ' . $options['defer'] );
                }
-               Assert::parameterType(
-                       'integer|null', $options['transactionTicket'], '$options[\'transactionTicket\']' );
-
                $updates = $this->getSecondaryDataUpdates( $options['recursive'] );
 
                $triggeringUser = $this->options['triggeringUser'] ?? $this->user;
@@ -1596,14 +1594,6 @@ class DerivedPageDataUpdater implements IDBAccessObject {
                $causeAction = $this->options['causeAction'] ?? 'unknown';
                $causeAgent = $this->options['causeAgent'] ?? 'unknown';
                $legacyRevision = new Revision( $this->revision );
-               $ticket = $options['transactionTicket'];
-
-               if ( $options['defer'] === false && $ticket !== null ) {
-                       // For legacy hook handlers doing updates via LinksUpdateConstructed, make sure
-                       // any pending writes they made get flushed before the doUpdate() calls below.
-                       // This avoids snapshot-clearing errors in LinksUpdate::acquirePageLock().
-                       $this->loadbalancerFactory->commitAndWaitForReplication( __METHOD__, $ticket );
-               }
 
                foreach ( $updates as $update ) {
                        if ( $update instanceof DataUpdate ) {
@@ -1613,13 +1603,16 @@ class DerivedPageDataUpdater implements IDBAccessObject {
                                $update->setRevision( $legacyRevision );
                                $update->setTriggeringUser( $triggeringUser );
                        }
+               }
 
-                       if ( $options['defer'] === false ) {
-                               if ( $update instanceof DataUpdate && $ticket !== null ) {
-                                       $update->setTransactionTicket( $ticket );
-                               }
-                               $update->doUpdate();
-                       } else {
+               if ( $options['defer'] === false ) {
+                       // T221577: flush any transaction; each update needs outer transaction scope
+                       $this->loadbalancerFactory->commitMasterChanges( __METHOD__ );
+                       foreach ( $updates as $update ) {
+                               DeferredUpdates::attemptUpdate( $update, $this->loadbalancerFactory );
+                       }
+               } else {
+                       foreach ( $updates as $update ) {
                                DeferredUpdates::addUpdate( $update, $options['defer'] );
                        }
                }

diff --git a/includes/deferred/DeferredUpdates.php b/includes/deferred/DeferredUpdates.php
index 4b54378..f5d22c1 100644 (file)
--- a/includes/deferred/DeferredUpdates.php
+++ b/includes/deferred/DeferredUpdates.php
@@ -184,8 +184,6 @@ class DeferredUpdates {
                $lbFactory = $services->getDBLoadBalancerFactory();
                $method = RequestContext::getMain()->getRequest()->getMethod();
 
-               $ticket = $lbFactory->getEmptyTransactionTicket( __METHOD__ );
-
                /** @var ErrorPageError $reportableError */
                $reportableError = null;
                /** @var DeferrableUpdate[] $updates Snapshot of queue */
@@ -199,7 +197,6 @@ class DeferredUpdates {
                        $updatesByType = [ 'data' => [], 'generic' => [] ];
                        foreach ( $updates as $du ) {
                                if ( $du instanceof DataUpdate ) {
-                                       $du->setTransactionTicket( $ticket );
                                        $updatesByType['data'][] = $du;
                                } else {
                                        $updatesByType['generic'][] = $du;
@@ -225,10 +222,6 @@ class DeferredUpdates {
                                                        $firstKey = key( self::$executeContext['subqueue'] );
                                                        unset( self::$executeContext['subqueue'][$firstKey] );
 
-                                                       if ( $subUpdate instanceof DataUpdate ) {
-                                                               $subUpdate->setTransactionTicket( $ticket );
-                                                       }
-
                                                        $guiError = self::handleUpdate( $subUpdate, $lbFactory, $mode, $stage );
                                                        $reportableError = $reportableError ?: $guiError;
                                                }
@@ -300,6 +293,10 @@ class DeferredUpdates {
         * @since 1.34
         */
        public static function attemptUpdate( DeferrableUpdate $update, ILBFactory $lbFactory ) {
+               if ( $update instanceof DataUpdate ) {
+                       $update->setTransactionTicket( $lbFactory->getEmptyTransactionTicket( __METHOD__ ) );
+               }
+
                if (
                        $update instanceof TransactionRoundAwareUpdate &&
                        $update->getTransactionRoundRequirement() == $update::TRX_ROUND_ABSENT

diff --git a/includes/deferred/MWCallableUpdate.php b/includes/deferred/MWCallableUpdate.php
index efca406..707035c 100644 (file)
--- a/includes/deferred/MWCallableUpdate.php
+++ b/includes/deferred/MWCallableUpdate.php
@@ -5,11 +5,15 @@ use Wikimedia\Rdbms\IDatabase;
 /**
  * Deferrable Update for closure/callback
  */
-class MWCallableUpdate implements DeferrableUpdate, DeferrableCallback {
-       /** @var callable|null */
+class MWCallableUpdate
+       implements DeferrableUpdate, DeferrableCallback, TransactionRoundAwareUpdate
+{
+       /** @var callable|null Callback, or null if it was cancelled */
        private $callback;
-       /** @var string */
+       /** @var string Calling method name */
        private $fname;
+       /** @var int One of the class TRX_ROUND_* constants */
+       private $trxRoundRequirement = self::TRX_ROUND_PRESENT;
 
        /**
         * @param callable $callback
@@ -48,4 +52,16 @@ class MWCallableUpdate implements DeferrableUpdate, DeferrableCallback {
        public function getOrigin() {
                return $this->fname;
        }
+
+       /**
+        * @since 1.34
+        * @param int $mode One of the class TRX_ROUND_* constants
+        */
+       public function setTransactionRoundRequirement( $mode ) {
+               $this->trxRoundRequirement = $mode;
+       }
+
+       public function getTransactionRoundRequirement() {
+               return $this->trxRoundRequirement;
+       }
 }

diff --git a/includes/jobqueue/jobs/RefreshLinksJob.php b/includes/jobqueue/jobs/RefreshLinksJob.php
index a0b6e07..89ecb0e 100644 (file)
--- a/includes/jobqueue/jobs/RefreshLinksJob.php
+++ b/includes/jobqueue/jobs/RefreshLinksJob.php
@@ -279,8 +279,7 @@ class RefreshLinksJob extends Job {
                        'recursive' => !empty( $this->params['useRecursiveLinksUpdate'] ),
                        // Carry over cause so the update can do extra logging
                        'causeAction' => $this->params['causeAction'],
-                       'causeAgent' => $this->params['causeAgent'],
-                       'transactionTicket' => $ticket
+                       'causeAgent' => $this->params['causeAgent']
                ];
                if ( !empty( $this->params['triggeringUser'] ) ) {
                        $userInfo = $this->params['triggeringUser'];

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index 863a3f3..3814112 100644 (file)
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -2106,8 +2106,6 @@ class WikiPage implements Page, IDBAccessObject {
         *   - defer: one of the DeferredUpdates constants, or false to run immediately (default: false).
         *     Note that even when this is set to false, some updates might still get deferred (as
         *     some update might directly add child updates to DeferredUpdates).
-        *   - transactionTicket: a transaction ticket from LBFactory::getEmptyTransactionTicket(),
-        *     only when defer is false (default: null)
         * @since 1.32
         */
        public function doSecondaryDataUpdates( array $options = [] ) {