jenkins-bot has submitted this change and it was merged. Change subject: getScopedLockAndFlush() should not commit when exceptions are thrown ......................................................................
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 --- M includes/db/Database.php M includes/db/IDatabase.php M tests/phpunit/includes/db/DatabaseTest.php 3 files changed, 61 insertions(+), 5 deletions(-) Approvals: Krinkle: Looks good to me, approved jenkins-bot: Verified diff --git a/includes/db/Database.php b/includes/db/Database.php index 940c3f7..be0399d 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -3291,8 +3291,16 @@ } $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 --- a/includes/db/IDatabase.php +++ b/includes/db/IDatabase.php @@ -1359,6 +1359,10 @@ * 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 @@ * 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 @@ * 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 @@ /** * 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 --- a/tests/phpunit/includes/db/DatabaseTest.php +++ b/tests/phpunit/includes/db/DatabaseTest.php @@ -285,8 +285,42 @@ $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!" ); + } } -- To view, visit https://gerrit.wikimedia.org/r/305392 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I3d4186eb9ec02cf4d42ac84590869e2cf29b30b4 Gerrit-PatchSet: 5 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org> Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org> Gerrit-Reviewer: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: Parent5446 <tylerro...@gmail.com> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits