From c35fef1e2f86244f57ff426cb518f24656617fbd Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 13 Mar 2019 19:29:36 -0700
Subject: [PATCH] rdbms: improve database connection loss handling

Avoid throwing errors in Database::replaceLostConnection()

Bug: T218226
Change-Id: Id07f305816c61f62aaf1ae893f5d37c03c865f46
---
 includes/libs/rdbms/database/Database.php | 55 ++++++++++++++---------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 224bcf29d0..9ce3086c38 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -926,6 +926,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	final public function close() {
 		$exception = null; // error to throw after disconnecting
 
+		$wasOpen = $this->opened;
+		// This should mostly do nothing if the connection is already closed
 		if ( $this->conn ) {
 			// Roll back any dangling transaction first
 			if ( $this->trxLevel ) {
@@ -968,11 +970,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
 			// Close the actual connection in the binding handle
 			$closed = $this->closeConnection();
-			$this->conn = false;
 		} else {
 			$closed = true; // already closed; nothing to do
 		}
 
+		$this->conn = false;
 		$this->opened = false;
 
 		// Throw any unexpected errors after having disconnected
@@ -980,12 +982,18 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			throw $exception;
 		}
 
-		// Sanity check that no callbacks are dangling
-		$fnames = $this->pendingWriteAndCallbackCallers();
-		if ( $fnames ) {
-			throw new RuntimeException(
-				"Transaction callbacks are still pending:\n" . implode( ', ', $fnames )
-			);
+		// Note that various subclasses call close() at the start of open(), which itself is
+		// called by replaceLostConnection(). In that case, just because onTransactionResolution()
+		// callbacks are pending does not mean that an exception should be thrown. Rather, they
+		// will be executed after the reconnection step.
+		if ( $wasOpen ) {
+			// Sanity check that no callbacks are dangling
+			$fnames = $this->pendingWriteAndCallbackCallers();
+			if ( $fnames ) {
+				throw new RuntimeException(
+					"Transaction callbacks are still pending:\n" . implode( ', ', $fnames )
+				);
+			}
 		}
 
 		return $closed;
@@ -1444,9 +1452,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 	}
 
 	/**
-	 * Clean things up after session (and thus transaction) loss
+	 * Clean things up after session (and thus transaction) loss before reconnect
 	 */
-	private function handleSessionLoss() {
+	private function handleSessionLossPreconnect() {
 		// Clean up tracking of session-level things...
 		// https://dev.mysql.com/doc/refman/5.7/en/implicit-commit.html
 		// https://www.postgresql.org/docs/9.2/static/sql-createtable.html (ignoring ON COMMIT)
@@ -1455,13 +1463,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		// https://www.postgresql.org/docs/9.4/static/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS
 		$this->namedLocksHeld = [];
 		// Session loss implies transaction loss
-		$this->handleTransactionLoss();
-	}
-
-	/**
-	 * Clean things up after transaction loss
-	 */
-	private function handleTransactionLoss() {
+		$this->trxLevel = 0;
+		$this->trxAtomicCounter = 0;
+		$this->trxIdleCallbacks = []; // T67263; transaction already lost
+		$this->trxPreCommitCallbacks = []; // T67263; transaction already lost
+		// @note: leave trxRecurringCallbacks in place
 		if ( $this->trxDoneWrites ) {
 			$this->trxProfiler->transactionWritingOut(
 				$this->server,
@@ -1471,10 +1477,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 				$this->trxWriteAffectedRows
 			);
 		}
-		$this->trxLevel = 0;
-		$this->trxAtomicCounter = 0;
-		$this->trxIdleCallbacks = []; // T67263; transaction already lost
-		$this->trxPreCommitCallbacks = []; // T67263; transaction already lost
+	}
+
+	/**
+	 * Clean things up after session (and thus transaction) loss after reconnect
+	 */
+	private function handleSessionLossPostconnect() {
 		try {
 			// Handle callbacks in trxEndCallbacks, e.g. onTransactionResolution().
 			// If callback suppression is set then the array will remain unhandled.
@@ -4177,6 +4185,9 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 		$this->closeConnection();
 		$this->opened = false;
 		$this->conn = false;
+
+		$this->handleSessionLossPreconnect();
+
 		try {
 			$this->open(
 				$this->server,
@@ -4205,7 +4216,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			);
 		}
 
-		$this->handleSessionLoss();
+		$this->handleSessionLossPostconnect();
 
 		return $ok;
 	}
@@ -4718,7 +4729,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			$this->opened = false;
 			$this->conn = false;
 			$this->trxEndCallbacks = []; // don't copy
-			$this->handleSessionLoss(); // no trx or locks anymore
+			$this->handleSessionLossPreconnect(); // no trx or locks anymore
 			$this->open(
 				$this->server,
 				$this->user,
-- 
2.20.1