Make DeferredUpdates avoid running during LBFactory::commitMasterChanges

Bug: T193668
Change-Id: I50890ef17ea72481a14c4abcd93ae58b93f15d28

diff --git a/includes/deferred/DeferredUpdates.php b/includes/deferred/DeferredUpdates.php
index ef46953..8543c4b 100644 (file)
--- 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 1e8838e..45e7cbb 100644 (file)
--- 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 ca684c3..ccaebd3 100644 (file)
--- 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 6b41707..a1e41d9 100644 (file)
--- 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 );
+       }
 }