rdbms: clean up DBO_TRX behavior for onTransactionPreCommitOrIdle()

* Make sure cancelled onTransactionPreCommitOrIdle() callbacks do not
  run if a transaction round is rolled back and then a second round is
  committed. LoadBalancer::rollbackMasterChanges() now always calls
  rollback(), which in turn always cleans up such callbacks.
* Remove error logging for rollback() calls when trxLevel = 0; this is
  harmless and is sometimes hard to avoid in error handling anyway.
* Add more related unit tests.

Change-Id: I6bdefe8bf8b6630fc252b5bbafe4808758ba1684

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 97ea266..003bca0 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -3294,16 +3294,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        }
 
        final public function rollback( $fname = __METHOD__, $flush = '' ) {
-               if ( $flush === self::FLUSHING_INTERNAL || $flush === self::FLUSHING_ALL_PEERS ) {
-                       if ( !$this->trxLevel ) {
-                               return; // nothing to do
-                       }
-               } else {
-                       if ( !$this->trxLevel ) {
-                               $this->queryLogger->error(
-                                       "$fname: No transaction to rollback, something got out of sync." );
-                               return; // nothing to do
-                       } elseif ( $this->getFlag( self::DBO_TRX ) ) {
+               $trxActive = $this->trxLevel;
+
+               if ( $flush !== self::FLUSHING_INTERNAL && $flush !== self::FLUSHING_ALL_PEERS ) {
+                       if ( $this->getFlag( self::DBO_TRX ) ) {
                                throw new DBUnexpectedError(
                                        $this,
                                        "$fname: Expected mass rollback of all peer databases (DBO_TRX set)."
@@ -3311,33 +3305,40 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        }
                }
 
-               // Avoid fatals if close() was called
-               $this->assertOpen();
+               if ( $trxActive ) {
+                       // Avoid fatals if close() was called
+                       $this->assertOpen();
 
-               $this->doRollback( $fname );
-               $this->trxAtomicLevels = [];
-               if ( $this->trxDoneWrites ) {
-                       $this->trxProfiler->transactionWritingOut(
-                               $this->server,
-                               $this->dbName,
-                               $this->trxShortId
-                       );
+                       $this->doRollback( $fname );
+                       $this->trxAtomicLevels = [];
+                       if ( $this->trxDoneWrites ) {
+                               $this->trxProfiler->transactionWritingOut(
+                                       $this->server,
+                                       $this->dbName,
+                                       $this->trxShortId
+                               );
+                       }
                }
 
-               $this->trxIdleCallbacks = []; // clear
-               $this->trxPreCommitCallbacks = []; // clear
-               try {
-                       $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK );
-               } catch ( Exception $e ) {
-                       // already logged; finish and let LoadBalancer move on during mass-rollback
-               }
-               try {
-                       $this->runTransactionListenerCallbacks( self::TRIGGER_ROLLBACK );
-               } catch ( Exception $e ) {
-                       // already logged; let LoadBalancer move on during mass-rollback
-               }
+               // Clear any commit-dependant callbacks. They might even be present
+               // only due to transaction rounds, with no SQL transaction being active
+               $this->trxIdleCallbacks = [];
+               $this->trxPreCommitCallbacks = [];
 
-               $this->affectedRowCount = 0; // for the sake of consistency
+               if ( $trxActive ) {
+                       try {
+                               $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK );
+                       } catch ( Exception $e ) {
+                               // already logged; finish and let LoadBalancer move on during mass-rollback
+                       }
+                       try {
+                               $this->runTransactionListenerCallbacks( self::TRIGGER_ROLLBACK );
+                       } catch ( Exception $e ) {
+                               // already logged; let LoadBalancer move on during mass-rollback
+                       }
+
+                       $this->affectedRowCount = 0; // for the sake of consistency
+               }
        }
 
        /**

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index 4c5621a..4de17b7 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -1408,9 +1408,7 @@ class LoadBalancer implements ILoadBalancer {
                $this->trxRoundId = false;
                $this->forEachOpenMasterConnection(
                        function ( IDatabase $conn ) use ( $fname, $restore ) {
-                               if ( $conn->writesOrCallbacksPending() || $conn->explicitTrxActive() ) {
-                                       $conn->rollback( $fname, $conn::FLUSHING_ALL_PEERS );
-                               }
+                               $conn->rollback( $fname, $conn::FLUSHING_ALL_PEERS );
                                if ( $restore ) {
                                        $this->undoTransactionRoundFlags( $conn );
                                }

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index 85574b7..24d848e 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -263,11 +263,10 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
                $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
 
                $called = false;
-               $db->onTransactionPreCommitOrIdle(
-                       function () use ( &$called ) {
-                               $called = true;
-                       }
-               );
+               $callback = function () use ( &$called ) {
+                       $called = true;
+               };
+               $db->onTransactionPreCommitOrIdle( $callback, __METHOD__ );
                $this->assertFalse( $called, 'Not called when idle if DBO_TRX is set' );
 
                $lbFactory->beginMasterChanges( __METHOD__ );
@@ -275,6 +274,17 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
 
                $lbFactory->commitMasterChanges( __METHOD__ );
                $this->assertTrue( $called, 'Called when lb-transaction is committed' );
+
+               $called = false;
+               $lbFactory->beginMasterChanges( __METHOD__ );
+               $db->onTransactionPreCommitOrIdle( $callback, __METHOD__ );
+               $this->assertFalse( $called, 'Not called when lb-transaction is active' );
+
+               $lbFactory->rollbackMasterChanges( __METHOD__ );
+               $this->assertFalse( $called, 'Not called when lb-transaction is rolled back' );
+
+               $lbFactory->commitMasterChanges( __METHOD__ );
+               $this->assertFalse( $called, 'Not called in next round commit' );
        }
 
        /**