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

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index a934950..6fb15bb 100644 (file)
--- 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 5c0a8c7..1b3e6cc 100644 (file)
--- 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 b1582a1..83216d4 100644 (file)
--- 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 600e0d3..4488d9e 100644 (file)
--- 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() );
        }
 }