From b0ee0a8d9f1190740894b4e846bfd83fa6a90de1 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 19 Mar 2018 16:20:15 -0700 Subject: [PATCH] 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 --- includes/libs/rdbms/database/Database.php | 67 ++++++++++--------- .../libs/rdbms/loadbalancer/LoadBalancer.php | 4 +- .../libs/rdbms/database/DatabaseTest.php | 20 ++++-- 3 files changed, 50 insertions(+), 41 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 97ea26645c..003bca064a 100644 --- 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 4c5621ab2e..4de17b7547 100644 --- 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 85574b743e..24d848e406 100644 --- 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' ); } /** -- 2.20.1