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 940c3f7..be0399d 100644 (file)
--- 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 772d824..bdab09e 100644 (file)
--- 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 723f1c3..7e55a73 100644 (file)
--- 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!" );
+       }
 }