From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 17 Aug 2016 21:05:41 +0000 (-0700)
Subject: getScopedLockAndFlush() should not commit when exceptions are thrown
X-Git-Tag: 1.31.0-rc.0~6011
X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/operations/?a=commitdiff_plain;h=7287e01b3d7505f3a012ae71ce09057875334e7e;p=lhc%2Fweb%2Fwiklou.git

getScopedLockAndFlush() should not commit when exceptions are thrown

Previously it would commit() since the __destruct() call happens
as an exception is thrown but before it reached MWExceptionHandler.

Change-Id: I3d4186eb9ec02cf4d42ac84590869e2cf29b30b4
---

diff --git a/includes/db/Database.php b/includes/db/Database.php
index 940c3f7ac6..be0399d62f 100644
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -3291,8 +3291,16 @@ abstract class DatabaseBase implements IDatabase {
 		}
 
 		$unlocker = new ScopedCallback( function () use ( $lockKey, $fname ) {
-			$this->commit( __METHOD__, self::FLUSHING_INTERNAL );
-			$this->unlock( $lockKey, $fname );
+			if ( $this->trxLevel() ) {
+				// There is a good chance an exception was thrown, causing any early return
+				// from the caller. Let any error handler get a chance to issue rollback().
+				// If there isn't one, let the error bubble up and trigger server-side rollback.
+				$this->onTransactionResolution( function () use ( $lockKey, $fname ) {
+					$this->unlock( $lockKey, $fname );
+				} );
+			} else {
+				$this->unlock( $lockKey, $fname );
+			}
 		} );
 
 		$this->commit( __METHOD__, self::FLUSHING_INTERNAL );
diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index 772d824cc2..bdab09e7b5 100644
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -1359,6 +1359,10 @@ interface IDatabase {
 	 * Begin a transaction. If a transaction is already in progress,
 	 * that transaction will be committed before the new transaction is started.
 	 *
+	 * Only call this from code with outer transcation scope.
+	 * See https://www.mediawiki.org/wiki/Database_transactions for details.
+	 * Nesting of transactions is not supported.
+	 *
 	 * Note that when the DBO_TRX flag is set (which is usually the case for web
 	 * requests, but not for maintenance scripts), any previous database query
 	 * will have started a transaction automatically.
@@ -1377,6 +1381,8 @@ interface IDatabase {
 	 * Commits a transaction previously started using begin().
 	 * If no transaction is in progress, a warning is issued.
 	 *
+	 * Only call this from code with outer transcation scope.
+	 * See https://www.mediawiki.org/wiki/Database_transactions for details.
 	 * Nesting of transactions is not supported.
 	 *
 	 * @param string $fname
@@ -1397,7 +1403,11 @@ interface IDatabase {
 	 * Rollback a transaction previously started using begin().
 	 * If no transaction is in progress, a warning is issued.
 	 *
-	 * No-op on non-transactional databases.
+	 * Only call this from code with outer transcation scope.
+	 * See https://www.mediawiki.org/wiki/Database_transactions for details.
+	 * Nesting of transactions is not supported. If a serious unexpected error occurs,
+	 * throwing an Exception is preferrable, using a pre-installed error handler to trigger
+	 * rollback (in any case, failure to issue COMMIT will cause rollback server-side).
 	 *
 	 * @param string $fname
 	 * @param string $flush Flush flag, set to a situationally valid IDatabase::FLUSHING_*
@@ -1568,10 +1578,14 @@ interface IDatabase {
 	/**
 	 * Acquire a named lock, flush any transaction, and return an RAII style unlocker object
 	 *
+	 * Only call this from outer transcation scope and when only one DB will be affected.
+	 * See https://www.mediawiki.org/wiki/Database_transactions for details.
+	 *
 	 * This is suitiable for transactions that need to be serialized using cooperative locks,
 	 * where each transaction can see each others' changes. Any transaction is flushed to clear
 	 * out stale REPEATABLE-READ snapshot data. Once the returned object falls out of PHP scope,
-	 * any transaction will be committed and the lock will be released.
+	 * the lock will be released unless a transaction is active. If one is active, then the lock
+	 * will be released when it either commits or rolls back.
 	 *
 	 * If the lock acquisition failed, then no transaction flush happens, and null is returned.
 	 *
diff --git a/tests/phpunit/includes/db/DatabaseTest.php b/tests/phpunit/includes/db/DatabaseTest.php
index 723f1c319e..7e55a7327a 100644
--- a/tests/phpunit/includes/db/DatabaseTest.php
+++ b/tests/phpunit/includes/db/DatabaseTest.php
@@ -285,8 +285,42 @@ class DatabaseTest extends MediaWikiTestCase {
 			$called = true;
 			$db->setFlag( DBO_TRX );
 		} );
-		$db->rollback( __METHOD__ );
+		$db->rollback( __METHOD__, IDatabase::FLUSHING_ALL_PEERS );
 		$this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
 		$this->assertTrue( $called, 'Callback reached' );
 	}
+
+	public function testGetScopedLock() {
+		$db = $this->db;
+
+		$db->setFlag( DBO_TRX );
+		try {
+			$this->badLockingMethodImplicit( $db );
+		} catch ( RunTimeException $e ) {
+			$this->assertTrue( $db->trxLevel() > 0, "Transaction not committed." );
+		}
+		$db->clearFlag( DBO_TRX );
+		$db->rollback( __METHOD__, IDatabase::FLUSHING_ALL_PEERS );
+		$this->assertTrue( $db->lockIsFree( 'meow', __METHOD__ ) );
+
+		try {
+			$this->badLockingMethodExplicit( $db );
+		} catch ( RunTimeException $e ) {
+			$this->assertTrue( $db->trxLevel() > 0, "Transaction not committed." );
+		}
+		$db->rollback( __METHOD__, IDatabase::FLUSHING_ALL_PEERS );
+		$this->assertTrue( $db->lockIsFree( 'meow', __METHOD__ ) );
+	}
+
+	private function badLockingMethodImplicit( IDatabase $db ) {
+		$lock = $db->getScopedLockAndFlush( 'meow', __METHOD__, 1 );
+		$db->query( "SELECT 1" ); // trigger DBO_TRX
+		throw new RunTimeException( "Uh oh!" );
+	}
+
+	private function badLockingMethodExplicit( IDatabase $db ) {
+		$lock = $db->getScopedLockAndFlush( 'meow', __METHOD__, 1 );
+		$db->begin( __METHOD__ );
+		throw new RunTimeException( "Uh oh!" );
+	}
 }