From: Aaron Schulz <aschulz@wikimedia.org>
Date: Thu, 8 Mar 2018 21:38:10 +0000 (-0800)
Subject: rdbms: avoid throwing exceptions in Database::close() on reconnect
X-Git-Tag: 1.31.0-rc.0~392
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=397918afcde366c4e50aeff2166d2aec0b645f41

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 014c4af399..74da370c10 100644
--- 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 28a812573e..4594ac6974 100644
--- 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.