Merge "rdbms: Ensure onTransactionPreCommitOrIdle() callbacks don't lead transactions"

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 723a4a6..b8b44e6 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -2664,10 +2664,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        }
 
        final public function onTransactionPreCommitOrIdle( callable $callback, $fname = __METHOD__ ) {
-               if ( $this->mTrxLevel ) {
+               if ( $this->mTrxLevel || $this->getFlag( self::DBO_TRX ) ) {
+                       // As long as DBO_TRX is set, writes will accumulate until the load balancer issues
+                       // an implicit commit of all peer databases. This is true even if a transaction has
+                       // not yet been triggered by writes; make sure $callback runs *after* any such writes.
                        $this->mTrxPreCommitCallbacks[] = [ $callback, $fname ];
                } else {
-                       // If no transaction is active, then make one for this callback
+                       // No transaction is active nor will start implicitly, so make one for this callback
                        $this->startAtomic( __METHOD__ );
                        try {
                                call_user_func( $callback );

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index 9bea7ff..70b6c36 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -1,6 +1,7 @@
 <?php
 
 use Wikimedia\Rdbms\IDatabase;
+use Wikimedia\Rdbms\LBFactorySingle;
 use Wikimedia\Rdbms\TransactionProfiler;
 use Wikimedia\TestingAccessWrapper;
 
@@ -135,6 +136,71 @@ class DatabaseTest extends PHPUnit_Framework_TestCase {
                $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
        }
 
+       /**
+        * @covers Wikimedia\Rdbms\Database::onTransactionPreCommitOrIdle
+        * @covers Wikimedia\Rdbms\Database::runOnTransactionPreCommitCallbacks
+        */
+       public function testTransactionPreCommitOrIdle() {
+               $db = $this->getMockDB( [ 'isOpen' ] );
+               $db->method( 'isOpen' )->willReturn( true );
+               $db->clearFlag( DBO_TRX );
+
+               $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX is not set' );
+
+               $called = false;
+               $db->onTransactionPreCommitOrIdle(
+                       function () use ( &$called ) {
+                               $called = true;
+                       },
+                       __METHOD__
+               );
+               $this->assertTrue( $called, 'Called when idle' );
+
+               $db->begin( __METHOD__ );
+               $called = false;
+               $db->onTransactionPreCommitOrIdle(
+                       function () use ( &$called ) {
+                               $called = true;
+                       },
+                       __METHOD__
+               );
+               $this->assertFalse( $called, 'Not called when transaction is active' );
+               $db->commit( __METHOD__ );
+               $this->assertTrue( $called, 'Called when transaction is committed' );
+       }
+
+       /**
+        * @covers Wikimedia\Rdbms\Database::onTransactionPreCommitOrIdle
+        * @covers Wikimedia\Rdbms\Database::runOnTransactionPreCommitCallbacks
+        */
+       public function testTransactionPreCommitOrIdle_TRX() {
+               $db = $this->getMockDB( [ 'isOpen' ] );
+               $db->method( 'isOpen' )->willReturn( true );
+               $db->setFlag( DBO_TRX );
+
+               $lbFactory = LBFactorySingle::newFromConnection( $db );
+               // Ask for the connectin so that LB sets internal state
+               // about this connection being the master connection
+               $lb = $lbFactory->getMainLB();
+               $conn = $lb->openConnection( $lb->getWriterIndex() );
+               $this->assertSame( $db, $conn, 'Same DB instance' );
+               $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
+
+               $called = false;
+               $db->onTransactionPreCommitOrIdle(
+                       function () use ( &$called ) {
+                               $called = true;
+                       }
+               );
+               $this->assertFalse( $called, 'Not called when idle if DBO_TRX is set' );
+
+               $lbFactory->beginMasterChanges( __METHOD__ );
+               $this->assertFalse( $called, 'Not called when lb-transaction is active' );
+
+               $lbFactory->commitMasterChanges( __METHOD__ );
+               $this->assertTrue( $called, 'Called when lb-transaction is committed' );
+       }
+
        /**
         * @covers Wikimedia\Rdbms\Database::onTransactionResolution
         * @covers Wikimedia\Rdbms\Database::runOnTransactionIdleCallbacks