From d70b081011b8d3df55e5b0f114c7f8f526522a0b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 4 Jul 2016 11:02:42 -0700 Subject: [PATCH] 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 --- includes/db/DBConnRef.php | 8 +++- includes/db/Database.php | 46 ++++++++++++++++------ includes/db/IDatabase.php | 18 ++++++++- includes/filerepo/file/LocalFile.php | 8 ++-- tests/phpunit/includes/db/DatabaseTest.php | 31 ++++++++++++++- 5 files changed, 91 insertions(+), 20 deletions(-) diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php index af5f8f9fee..1893c73583 100644 --- 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 6bdcb24cdb..cfdf3828d1 100644 --- 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 0a71df2312..36772b8048 100644 --- 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 066da60e30..5d63645dc8 100644 --- 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 0730529bd6..723f1c319e 100644 --- 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' ); + } } -- 2.20.1