rdbms: avoid throwing exceptions in Database::close() on reconnect

The check caused problems in reconnect() calls from rollback()
triggered by LBFactory::rollbackMasterChanges(). Since callbacks
are suppressed at that time, handleSessionLoss() does not consume
all of them, so the the call to open() triggered a close() call
that would error out since the callbacks are still there.

Only do that check if a connection was present beforehand.
Check for callback suppression before trying commit() too.

Also make writesOrCallbacksPending() check trxEndCallbacks.

Bug: T188875
Change-Id: Ia46d30d75132358a0b4f60e847937013781c1daa

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 014c4af..74da370 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -607,7 +607,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
        public function writesOrCallbacksPending() {
                return $this->trxLevel && (
-                       $this->trxDoneWrites || $this->trxIdleCallbacks || $this->trxPreCommitCallbacks
+                       $this->trxDoneWrites ||
+                       $this->trxIdleCallbacks ||
+                       $this->trxPreCommitCallbacks ||
+                       $this->trxEndCallbacks
                );
        }
 
@@ -810,21 +813,38 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
        public function close() {
                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() ]
+                                       );
+                               }
+                               // Check if it is possible to properly commit and trigger callbacks
+                               if ( $this->trxEndCallbacksSuppressed ) {
+                                       throw new DBUnexpectedError(
+                                               $this,
+                                               __METHOD__ . ': callbacks are suppressed; cannot properly commit.'
+                                       );
+                               }
+                               // Commit the changes and run any callbacks as needed
                                $this->commit( __METHOD__, self::FLUSHING_INTERNAL );
                        }
-
+                       // Close the actual connection in the binding handle
                        $closed = $this->closeConnection();
                        $this->conn = false;
-               } elseif (
-                       $this->trxIdleCallbacks ||
-                       $this->trxPreCommitCallbacks ||
-                       $this->trxEndCallbacks
-               ) { // sanity
-                       throw new RuntimeException( "Transaction callbacks still pending." );
+                       // Sanity check that no callbacks are dangling
+                       if (
+                               $this->trxIdleCallbacks || $this->trxPreCommitCallbacks || $this->trxEndCallbacks
+                       ) {
+                               throw new RuntimeException( "Transaction callbacks still pending." );
+                       }
                } else {
-                       $closed = true;
+                       $closed = true; // already closed; nothing to do
                }
+
                $this->opened = false;
 
                return $closed;

diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php
index 28a8125..4594ac6 100644 (file)
--- a/includes/libs/rdbms/database/IDatabase.php
+++ b/includes/libs/rdbms/database/IDatabase.php
@@ -357,7 +357,7 @@ interface IDatabase {
        public function getType();
 
        /**
-        * Open a connection to the database. Usually aborts on failure
+        * Open a new connection to the database (closing any existing one)
         *
         * @param string $server Database server host
         * @param string $user Database user name
@@ -492,8 +492,11 @@ interface IDatabase {
        public function getServerVersion();
 
        /**
-        * Closes a database connection.
-        * if it is open : commits any open transactions
+        * 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.
         *
         * @throws DBError
         * @return bool Operation success. true if already closed.