From f9d10f9e0321151914159773edca3b8eb17dfa54 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 10 Apr 2018 16:56:29 -0700 Subject: [PATCH] rdbms: fix transaction flushing in Database::close Use the right IDatabase constants for the $flush parameter to the commit() and rollback() calls. This fixes a regression from 3975e04cf4d1. Also validate the mode/flush parameters to begin() and commit(). Bug: T191916 Change-Id: I0992f9a87f2add303ed309efcc1adb781baecfdc --- includes/libs/rdbms/database/Database.php | 62 +++++++++++++------ includes/libs/rdbms/database/IDatabase.php | 6 +- .../libs/rdbms/database/DatabaseSQLTest.php | 38 ++++++++++++ 3 files changed, 85 insertions(+), 21 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 5b259bd31d..8d38fb0650 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -898,34 +898,48 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $this->conn ) { // Resolve any dangling transaction first if ( $this->trxLevel ) { - // Meaningful transactions should ideally have been resolved by now - if ( $this->writesOrCallbacksPending() ) { - $this->queryLogger->warning( - __METHOD__ . ": writes or callbacks still pending.", - [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] - ); + if ( $this->trxAtomicLevels ) { // Cannot let incomplete atomic sections be committed - if ( $this->trxAtomicLevels ) { - $levels = $this->flatAtomicSectionList(); - $exception = new DBUnexpectedError( - $this, - __METHOD__ . ": atomic sections $levels are still open." - ); - // Check if it is possible to properly commit and trigger callbacks - } elseif ( $this->trxEndCallbacksSuppressed ) { + $levels = $this->flatAtomicSectionList(); + $exception = new DBUnexpectedError( + $this, + __METHOD__ . ": atomic sections $levels are still open." + ); + } elseif ( $this->trxAutomatic ) { + // Only the connection manager can commit non-empty DBO_TRX transactions + if ( $this->writesOrCallbacksPending() ) { $exception = new DBUnexpectedError( $this, - __METHOD__ . ': callbacks are suppressed; cannot properly commit.' + __METHOD__ . + ": mass commit/rollback of peer transaction required (DBO_TRX set)." ); } + } elseif ( $this->trxLevel ) { + // Commit explicit transactions as if this was commit() + $this->queryLogger->warning( + __METHOD__ . ": writes or callbacks still pending.", + [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] + ); + } + + if ( $this->trxEndCallbacksSuppressed ) { + $exception = $exception ?: new DBUnexpectedError( + $this, + __METHOD__ . ': callbacks are suppressed; cannot properly commit.' + ); } + // Commit or rollback the changes and run any callbacks as needed if ( $this->trxStatus === self::STATUS_TRX_OK && !$exception ) { - $this->commit( __METHOD__, self::TRANSACTION_INTERNAL ); + $this->commit( + __METHOD__, + $this->trxAutomatic ? self::FLUSHING_INTERNAL : self::FLUSHING_ONE + ); } else { - $this->rollback( __METHOD__, self::TRANSACTION_INTERNAL ); + $this->rollback( __METHOD__, self::FLUSHING_INTERNAL ); } } + // Close the actual connection in the binding handle $closed = $this->closeConnection(); $this->conn = false; @@ -3513,6 +3527,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } final public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT ) { + static $modes = [ self::TRANSACTION_EXPLICIT, self::TRANSACTION_INTERNAL ]; + if ( !in_array( $mode, $modes, true ) ) { + throw new DBUnexpectedError( $this, "$fname: invalid mode parameter '$mode'." ); + } + // Protect against mismatched atomic section, transaction nesting, and snapshot loss if ( $this->trxLevel ) { if ( $this->trxAtomicLevels ) { @@ -3571,9 +3590,14 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->trxLevel = 1; } - final public function commit( $fname = __METHOD__, $flush = '' ) { + final public function commit( $fname = __METHOD__, $flush = self::FLUSHING_ONE ) { + static $modes = [ self::FLUSHING_ONE, self::FLUSHING_ALL_PEERS, self::FLUSHING_INTERNAL ]; + if ( !in_array( $flush, $modes, true ) ) { + throw new DBUnexpectedError( $this, "$fname: invalid flush parameter '$flush'." ); + } + if ( $this->trxLevel && $this->trxAtomicLevels ) { - // There are still atomic sections open. This cannot be ignored + // There are still atomic sections open; this cannot be ignored $levels = $this->flatAtomicSectionList(); throw new DBUnexpectedError( $this, diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 5876d6bab0..ff561f1238 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -54,9 +54,11 @@ interface IDatabase { /** @var string Atomic section is cancelable */ const ATOMIC_CANCELABLE = 'cancelable'; - /** @var string Transaction operation comes from service managing all DBs */ + /** @var string Commit/rollback is from outside the IDatabase handle and connection manager */ + const FLUSHING_ONE = ''; + /** @var string Commit/rollback is from the connection manager for the IDatabase handle */ const FLUSHING_ALL_PEERS = 'flush'; - /** @var string Transaction operation comes from the database class internally */ + /** @var string Commit/rollback is from the IDatabase handle internally */ const FLUSHING_INTERNAL = 'flush'; /** @var string Do not remember the prior flags */ diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 665e953e9d..2c07739c1c 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -1687,4 +1687,42 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; ROLLBACK' ); $this->assertEquals( 0, $this->database->trxLevel() ); } + + /** + * @covers \Wikimedia\Rdbms\Database::close + */ + public function testPrematureClose3() { + try { + $this->database->setFlag( IDatabase::DBO_TRX ); + $this->database->delete( 'x', [ 'field' => 3 ], __METHOD__ ); + $this->assertEquals( 1, $this->database->trxLevel() ); + $this->database->close(); + $this->fail( 'Expected exception not thrown' ); + } catch ( DBUnexpectedError $ex ) { + $this->assertSame( + 'Wikimedia\Rdbms\Database::close: ' . + 'mass commit/rollback of peer transaction required (DBO_TRX set).', + $ex->getMessage() + ); + } + + $this->assertFalse( $this->database->isOpen() ); + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; ROLLBACK' ); + $this->assertEquals( 0, $this->database->trxLevel() ); + } + + /** + * @covers \Wikimedia\Rdbms\Database::close + */ + public function testPrematureClose4() { + $this->database->setFlag( IDatabase::DBO_TRX ); + $this->database->query( 'SELECT 1', __METHOD__ ); + $this->assertEquals( 1, $this->database->trxLevel() ); + $this->database->close(); + $this->database->clearFlag( IDatabase::DBO_TRX ); + + $this->assertFalse( $this->database->isOpen() ); + $this->assertLastSql( 'BEGIN; SELECT 1; COMMIT' ); + $this->assertEquals( 0, $this->database->trxLevel() ); + } } -- 2.20.1