From bf30fcb71427d673f7c83a067b3241040d3470b6 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 3 Oct 2018 13:38:19 -0400 Subject: [PATCH] Database: close() should not commit transactions Transactional databases normally roll back when a connection is closed with an open transaction rather than committing them, so MediaWiki committing them is unexpected. There are two cases being changed here: automatic transactions without writes and manual transactions. For the former it shouldn't make a difference if we commit or roll back since no writes were done anyway. The latter has logged a message since MW 1.31 (I0992f9a8), and that warning has not been logged in Wikimedia production in the past 60 days so we should be ok there too. Bug: T206147 Change-Id: Ieceef4deb49044db8f0622d38ee76c9d9f39704c --- RELEASE-NOTES-1.32 | 1 + includes/libs/rdbms/database/Database.php | 25 ++++++++----------- includes/libs/rdbms/database/IDatabase.php | 4 +-- .../libs/rdbms/database/DatabaseSQLTest.php | 17 ++++++++++--- 4 files changed, 27 insertions(+), 20 deletions(-) diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index a934950147..6fb15bb7fc 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -497,6 +497,7 @@ because of Phabricator reports. * The image_comment_temp database table is merged into the image table and deprecated. Since access should be mediated by the CommentStore class, this change shouldn't affect external code. +* (T206147) Database::close() will no longer commit any open transactions. == Compatibility == MediaWiki 1.32 requires PHP 7.0.0 or later. Although HHVM 3.18.5 or later is diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 5c0a8c71d5..1b3e6cc5da 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -911,7 +911,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $exception = null; // error to throw after disconnecting if ( $this->conn ) { - // Resolve any dangling transaction first + // Roll back any dangling transaction first if ( $this->trxLevel ) { if ( $this->trxAtomicLevels ) { // Cannot let incomplete atomic sections be committed @@ -922,6 +922,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware ); } elseif ( $this->trxAutomatic ) { // Only the connection manager can commit non-empty DBO_TRX transactions + // (empty ones we can silently roll back) if ( $this->writesOrCallbacksPending() ) { $exception = new DBUnexpectedError( $this, @@ -929,11 +930,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware ": 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() ] + } else { + // Manual transactions should have been committed or rolled + // back, even if empty. + $exception = new DBUnexpectedError( + $this, + __METHOD__ . ": transaction is still open (from {$this->trxFname})." ); } @@ -944,15 +946,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware ); } - // Commit or rollback the changes and run any callbacks as needed - if ( $this->trxStatus === self::STATUS_TRX_OK && !$exception ) { - $this->commit( - __METHOD__, - $this->trxAutomatic ? self::FLUSHING_INTERNAL : self::FLUSHING_ONE - ); - } else { - $this->rollback( __METHOD__, self::FLUSHING_INTERNAL ); - } + // Rollback the changes and run any callbacks as needed + $this->rollback( __METHOD__, self::FLUSHING_INTERNAL ); } // Close the actual connection in the binding handle diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index b1582a140b..83216d4591 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -486,8 +486,8 @@ interface IDatabase { * Close the database connection * * This should only be called after any transactions have been resolved, - * aside from read-only transactions (assuming no callbacks are registered). - * If a transaction is still open anyway, it will be committed if possible. + * aside from read-only automatic transactions (assuming no callbacks are registered). + * If a transaction is still open anyway, it will be rolled back. * * @throws DBError * @return bool Operation success. true if already closed. diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 600e0d31fd..4488d9e9fe 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -2057,11 +2057,22 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->database->onTransactionCommitOrIdle( function () use ( $fname ) { $this->database->query( 'SELECT 1', $fname ); } ); + $this->database->onTransactionResolution( function () use ( $fname ) { + $this->database->query( 'SELECT 2', $fname ); + } ); $this->database->delete( 'x', [ 'field' => 3 ], __METHOD__ ); - $this->database->close(); + try { + $this->database->close(); + $this->fail( 'Expected exception not thrown' ); + } catch ( DBUnexpectedError $ex ) { + $this->assertSame( + "Wikimedia\Rdbms\Database::close: transaction is still open (from $fname).", + $ex->getMessage() + ); + } $this->assertFalse( $this->database->isOpen() ); - $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; COMMIT; SELECT 1' ); + $this->assertLastSql( 'BEGIN; DELETE FROM x WHERE field = \'3\'; ROLLBACK; SELECT 2' ); $this->assertEquals( 0, $this->database->trxLevel() ); } @@ -2125,7 +2136,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { $this->database->clearFlag( IDatabase::DBO_TRX ); $this->assertFalse( $this->database->isOpen() ); - $this->assertLastSql( 'BEGIN; SELECT 1; COMMIT' ); + $this->assertLastSql( 'BEGIN; SELECT 1; ROLLBACK' ); $this->assertEquals( 0, $this->database->trxLevel() ); } } -- 2.20.1