Fixes to LocalFile::lock()

* Added onTransactionResolution() DB method.
* Use this method so that file unlocks fire on unlockAndRollback()
  as well as on DB errors (via MWExceptionHandler::handleException).
  This prevents locks from getting stuck for minutes when deadlocks
  happen, since the LockManager::destruct() method is not reliable.
* Fix broken reference counting which always released locks on the
  first unlock() call, even if there were 2+ lock() calls.
* Added some type hints to IDatabase methods.
* Fixed DatabaseBase::__destruct() logging to include all callbacks.

Bug: T132921
Change-Id: I684706957f4d794cb6fe61505b0d26b7893de706

diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php
index af5f8f9..1893c73 100644 (file)
--- a/includes/db/DBConnRef.php
+++ b/includes/db/DBConnRef.php
@@ -417,11 +417,15 @@ class DBConnRef implements IDatabase {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 
-       public function onTransactionIdle( $callback ) {
+       public function onTransactionResolution( callable $callback ) {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 
-       public function onTransactionPreCommitOrIdle( $callback ) {
+       public function onTransactionIdle( callable $callback ) {
+               return $this->__call( __FUNCTION__, func_get_args() );
+       }
+
+       public function onTransactionPreCommitOrIdle( callable $callback ) {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 

diff --git a/includes/db/Database.php b/includes/db/Database.php
index 6bdcb24..cfdf382 100644 (file)
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -52,10 +52,12 @@ abstract class DatabaseBase implements IDatabase {
        protected $mConn = null;
        protected $mOpened = false;
 
-       /** @var callable[] */
+       /** @var array[] List of (callable, method name) */
        protected $mTrxIdleCallbacks = [];
-       /** @var callable[] */
+       /** @var array[] List of (callable, method name) */
        protected $mTrxPreCommitCallbacks = [];
+       /** @var array[] List of (callable, method name) */
+       protected $mTrxEndCallbacks = [];
 
        protected $mTablePrefix;
        protected $mSchema;
@@ -2448,14 +2450,21 @@ abstract class DatabaseBase implements IDatabase {
                return false;
        }
 
-       final public function onTransactionIdle( $callback ) {
+       final public function onTransactionResolution( callable $callback ) {
+               if ( !$this->mTrxLevel ) {
+                       throw new DBUnexpectedError( $this, "No transaction is active." );
+               }
+               $this->mTrxEndCallbacks[] = [ $callback, wfGetCaller() ];
+       }
+
+       final public function onTransactionIdle( callable $callback ) {
                $this->mTrxIdleCallbacks[] = [ $callback, wfGetCaller() ];
                if ( !$this->mTrxLevel ) {
                        $this->runOnTransactionIdleCallbacks();
                }
        }
 
-       final public function onTransactionPreCommitOrIdle( $callback ) {
+       final public function onTransactionPreCommitOrIdle( callable $callback ) {
                if ( $this->mTrxLevel ) {
                        $this->mTrxPreCommitCallbacks[] = [ $callback, wfGetCaller() ];
                } else {
@@ -2473,8 +2482,12 @@ abstract class DatabaseBase implements IDatabase {
 
                $e = $ePrior = null; // last exception
                do { // callbacks may add callbacks :)
-                       $callbacks = $this->mTrxIdleCallbacks;
+                       $callbacks = array_merge(
+                               $this->mTrxIdleCallbacks,
+                               $this->mTrxEndCallbacks // include "transaction resolution" callbacks
+                       );
                        $this->mTrxIdleCallbacks = []; // recursion guard
+                       $this->mTrxEndCallbacks = []; // recursion guard
                        foreach ( $callbacks as $callback ) {
                                try {
                                        list( $phpCallback ) = $callback;
@@ -2607,10 +2620,11 @@ abstract class DatabaseBase implements IDatabase {
                                $this->getTransactionProfiler()->transactionWritingOut(
                                        $this->mServer, $this->mDBname, $this->mTrxShortId, $writeTime );
                        }
+
                        $this->runOnTransactionIdleCallbacks();
                }
 
-               # Avoid fatals if close() was called
+               // Avoid fatals if close() was called
                $this->assertOpen();
 
                $this->doBegin( $fname );
@@ -2671,7 +2685,7 @@ abstract class DatabaseBase implements IDatabase {
                        }
                }
 
-               # Avoid fatals if close() was called
+               // Avoid fatals if close() was called
                $this->assertOpen();
 
                $this->runOnTransactionPreCommitCallbacks();
@@ -2682,6 +2696,7 @@ abstract class DatabaseBase implements IDatabase {
                        $this->getTransactionProfiler()->transactionWritingOut(
                                $this->mServer, $this->mDBname, $this->mTrxShortId, $writeTime );
                }
+
                $this->runOnTransactionIdleCallbacks();
        }
 
@@ -2710,17 +2725,19 @@ abstract class DatabaseBase implements IDatabase {
                        }
                }
 
-               # Avoid fatals if close() was called
+               // Avoid fatals if close() was called
                $this->assertOpen();
 
                $this->doRollback( $fname );
-               $this->mTrxIdleCallbacks = []; // cancel
-               $this->mTrxPreCommitCallbacks = []; // cancel
                $this->mTrxAtomicLevels = [];
                if ( $this->mTrxDoneWrites ) {
                        $this->getTransactionProfiler()->transactionWritingOut(
                                $this->mServer, $this->mDBname, $this->mTrxShortId );
                }
+
+               $this->mTrxIdleCallbacks = []; // clear
+               $this->mTrxPreCommitCallbacks = []; // clear
+               $this->runOnTransactionIdleCallbacks();
        }
 
        /**
@@ -3294,9 +3311,14 @@ abstract class DatabaseBase implements IDatabase {
                if ( $this->mTrxLevel && $this->mTrxDoneWrites ) {
                        trigger_error( "Uncommitted DB writes (transaction from {$this->mTrxFname})." );
                }
-               if ( count( $this->mTrxIdleCallbacks ) || count( $this->mTrxPreCommitCallbacks ) ) {
+               $danglingCallbacks = array_merge(
+                       $this->mTrxIdleCallbacks,
+                       $this->mTrxPreCommitCallbacks,
+                       $this->mTrxEndCallbacks
+               );
+               if ( $danglingCallbacks ) {
                        $callers = [];
-                       foreach ( $this->mTrxIdleCallbacks as $callbackInfo ) {
+                       foreach ( $danglingCallbacks as $callbackInfo ) {
                                $callers[] = $callbackInfo[1];
                        }
                        $callers = implode( ', ', $callers );

diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index 0a71df2..36772b8 100644 (file)
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -1215,6 +1215,20 @@ interface IDatabase {
         */
        public function getMasterPos();
 
+       /**
+        * Run an anonymous function as soon as the current transaction commits or rolls back.
+        * An error is thrown if no transaction is pending. Queries in the function will run in
+        * AUTO-COMMIT mode unless there are begin() calls. Callbacks must commit any transactions
+        * that they begin.
+        *
+        * This is useful for combining cooperative locks and DB transactions.
+        *
+        * @param callable $callback
+        * @return mixed
+        * @since 1.28
+        */
+       public function onTransactionResolution( callable $callback );
+
        /**
         * Run an anonymous function as soon as there is no transaction pending.
         * If there is a transaction and it is rolled back, then the callback is cancelled.
@@ -1231,7 +1245,7 @@ interface IDatabase {
         * @param callable $callback
         * @since 1.20
         */
-       public function onTransactionIdle( $callback );
+       public function onTransactionIdle( callable $callback );
 
        /**
         * Run an anonymous function before the current transaction commits or now if there is none.
@@ -1246,7 +1260,7 @@ interface IDatabase {
         * @param callable $callback
         * @since 1.22
         */
-       public function onTransactionPreCommitOrIdle( $callback );
+       public function onTransactionPreCommitOrIdle( callable $callback );
 
        /**
         * Begin an atomic section of statements

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index 066da60..5d63645 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -1932,9 +1932,9 @@ class LocalFile extends File {
 
                                throw new LocalFileLockError( $status );
                        }
-                       // Release the lock *after* commit to avoid row-level contention
-                       $this->locked++;
-                       $dbw->onTransactionIdle( function () use ( $backend, $lockPaths, $logger ) {
+                       // Release the lock *after* commit to avoid row-level contention.
+                       // Make sure it triggers on rollback() as well as commit() (T132921).
+                       $dbw->onTransactionResolution( function () use ( $backend, $lockPaths, $logger ) {
                                $status = $backend->unlockFiles( $lockPaths, LockManager::LOCK_EX );
                                if ( !$status->isGood() ) {
                                        $logger->error( "Failed to unlock '{file}'", [ 'file' => $this->name ] );
@@ -1942,6 +1942,8 @@ class LocalFile extends File {
                        } );
                }
 
+               $this->locked++;
+
                return $this->lockedOwnTrx;
        }
 

diff --git a/tests/phpunit/includes/db/DatabaseTest.php b/tests/phpunit/includes/db/DatabaseTest.php
index 0730529..723f1c3 100644 (file)
--- a/tests/phpunit/includes/db/DatabaseTest.php
+++ b/tests/phpunit/includes/db/DatabaseTest.php
@@ -239,12 +239,15 @@ class DatabaseTest extends MediaWikiTestCase {
                $db = $this->db;
 
                $db->setFlag( DBO_TRX );
+               $called = false;
                $flagSet = null;
-               $db->onTransactionIdle( function() use ( $db, &$flagSet ) {
+               $db->onTransactionIdle( function() use ( $db, &$flagSet, &$called ) {
+                       $called = true;
                        $flagSet = $db->getFlag( DBO_TRX );
                } );
                $this->assertFalse( $flagSet, 'DBO_TRX off in callback' );
                $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
+               $this->assertTrue( $called, 'Callback reached' );
 
                $db->clearFlag( DBO_TRX );
                $flagSet = null;
@@ -260,4 +263,30 @@ class DatabaseTest extends MediaWikiTestCase {
                } );
                $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
        }
+
+       public function testTransactionResolution() {
+               $db = $this->db;
+
+               $db->clearFlag( DBO_TRX );
+               $db->begin( __METHOD__ );
+               $called = false;
+               $db->onTransactionResolution( function() use ( $db, &$called ) {
+                       $called = true;
+                       $db->setFlag( DBO_TRX );
+               } );
+               $db->commit( __METHOD__ );
+               $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
+               $this->assertTrue( $called, 'Callback reached' );
+
+               $db->clearFlag( DBO_TRX );
+               $db->begin( __METHOD__ );
+               $called = false;
+               $db->onTransactionResolution( function() use ( $db, &$called ) {
+                       $called = true;
+                       $db->setFlag( DBO_TRX );
+               } );
+               $db->rollback( __METHOD__ );
+               $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
+               $this->assertTrue( $called, 'Callback reached' );
+       }
 }