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
