From: jenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Date: Thu, 22 Mar 2018 01:07:06 +0000 (+0000)
Subject: Merge "rdbms: clean up DBO_TRX behavior for onTransactionPreCommitOrIdle()"
X-Git-Tag: 1.31.0-rc.0~316
X-Git-Url: https://git.cyclocoop.org/%7B%7B%20url_for%28?a=commitdiff_plain;h=6c1cd929cb1c1fe0bcbde9fea59a1ff9bc2d9c6f;hp=cc1191e5bdd5b4878ee29fc0b103e55420011e92;p=lhc%2Fweb%2Fwiklou.git

Merge "rdbms: clean up DBO_TRX behavior for onTransactionPreCommitOrIdle()"
---

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 00bc1cbc49..417f64c8a5 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -3395,16 +3395,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 transactions (DBO_TRX set)."
@@ -3412,33 +3406,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' );
 	}
 
 	/**