From 33eb67af1d7ca97bca43941f0b4a4fa584c5343d Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 14 Nov 2012 11:40:36 -0800
Subject: [PATCH] [Database] Fixed case where trx idle callbacks might be lost.

* (bug 41656) LoadBalancer always commits if there are callbacks pending.
  This works by checking a new DatabaseBase::writesOrCallbacksPending() function.
* Made transaction idle callbacks run in autocommit mode. Generally callers already
  want autocommit mode or do quick begin()/commit() calls anyway. The docs already
  make stat that callbacks should close any connections they start, but this makes
  it harder for people to forget about implicit transactions. Since the callbacks
  often may happen in commitMasterChanges(), if transactions are left open, they
  might not get committed by anything afterwards.
* Added sanity exceptions and warnings if callbacks get lost for some reason.
* Renamed trxIdleCallbacks -> mTrxIdleCallbacks for consistency.

Change-Id: I00e1e0fcdd7deeee1fbac6d0f295160479cb8962
---
 includes/db/Database.php     | 39 ++++++++++++++++++++++++++++--------
 includes/db/LoadBalancer.php |  2 +-
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/includes/db/Database.php b/includes/db/Database.php
index db050f2df5..659c6e0c24 100644
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -230,9 +230,9 @@ abstract class DatabaseBase implements DatabaseType {
 
 	/**
 	 * @since 1.20
-	 * @var array of callable
+	 * @var array of Closure
 	 */
-	protected $trxIdleCallbacks = array();
+	protected $mTrxIdleCallbacks = array();
 
 	protected $mTablePrefix;
 	protected $mFlags;
@@ -534,6 +534,16 @@ abstract class DatabaseBase implements DatabaseType {
 		return $this->mDoneWrites;
 	}
 
+	/**
+	 * Returns true if there is a transaction open with possible write
+	 * queries or transaction idle callbacks waiting on it to finish.
+	 *
+	 * @return bool
+	 */
+	public function writesOrCallbacksPending() {
+		return $this->mTrxLevel && ( $this->mTrxDoneWrites || $this->mTrxIdleCallbacks );
+	}
+
 	/**
 	 * Is a connection to the database open?
 	 * @return Boolean
@@ -757,6 +767,9 @@ abstract class DatabaseBase implements DatabaseType {
 	 * @return Bool operation success. true if already closed.
 	 */
 	public function close() {
+		if ( count( $this->mTrxIdleCallbacks ) ) { // sanity
+			throw new MWException( "Transaction idle callbacks still pending." );
+		}
 		$this->mOpened = false;
 		if ( $this->mConn ) {
 			if ( $this->trxLevel() ) {
@@ -926,7 +939,7 @@ abstract class DatabaseBase implements DatabaseType {
 		if ( false === $ret && $this->wasErrorReissuable() ) {
 			# Transaction is gone, like it or not
 			$this->mTrxLevel = 0;
-			$this->trxIdleCallbacks = array(); // cancel
+			$this->mTrxIdleCallbacks = array(); // cancel
 			wfDebug( "Connection lost, reconnecting...\n" );
 
 			if ( $this->ping() ) {
@@ -2899,7 +2912,7 @@ abstract class DatabaseBase implements DatabaseType {
 	 */
 	final public function onTransactionIdle( Closure $callback ) {
 		if ( $this->mTrxLevel ) {
-			$this->trxIdleCallbacks[] = $callback;
+			$this->mTrxIdleCallbacks[] = $callback;
 		} else {
 			$callback();
 		}
@@ -2911,16 +2924,20 @@ abstract class DatabaseBase implements DatabaseType {
 	 * @since 1.20
 	 */
 	protected function runOnTransactionIdleCallbacks() {
+		$autoTrx = $this->getFlag( DBO_TRX ); // automatic begin() enabled?
+
 		$e = null; // last exception
 		do { // callbacks may add callbacks :)
-			$callbacks = $this->trxIdleCallbacks;
-			$this->trxIdleCallbacks = array(); // recursion guard
+			$callbacks = $this->mTrxIdleCallbacks;
+			$this->mTrxIdleCallbacks = array(); // recursion guard
 			foreach ( $callbacks as $callback ) {
 				try {
+					$this->clearFlag( DBO_TRX ); // make each query its own transaction
 					$callback();
+					$this->setFlag( $autoTrx ? DBO_TRX : 0 ); // restore automatic begin()
 				} catch ( Exception $e ) {}
 			}
-		} while ( count( $this->trxIdleCallbacks ) );
+		} while ( count( $this->mTrxIdleCallbacks ) );
 
 		if ( $e instanceof Exception ) {
 			throw $e; // re-throw any last exception
@@ -3037,7 +3054,7 @@ abstract class DatabaseBase implements DatabaseType {
 			wfWarn( "$fname: No transaction to rollback, something got out of sync!" );
 		}
 		$this->doRollback( $fname );
-		$this->trxIdleCallbacks = array(); // cancel
+		$this->mTrxIdleCallbacks = array(); // cancel
 	}
 
 	/**
@@ -3619,4 +3636,10 @@ abstract class DatabaseBase implements DatabaseType {
 	public function __toString() {
 		return (string)$this->mConn;
 	}
+
+	public function __destruct() {
+		if ( count( $this->mTrxIdleCallbacks ) ) { // sanity
+			trigger_error( "Transaction idle callbacks still pending." );
+		}
+	}
 }
diff --git a/includes/db/LoadBalancer.php b/includes/db/LoadBalancer.php
index 46d24fc0d4..8d32d35939 100644
--- a/includes/db/LoadBalancer.php
+++ b/includes/db/LoadBalancer.php
@@ -926,7 +926,7 @@ class LoadBalancer {
 				continue;
 			}
 			foreach ( $conns2[$masterIndex] as $conn ) {
-				if ( $conn->trxLevel() && $conn->doneWrites() ) {
+				if ( $conn->trxLevel() && $conn->writesOrCallbacksPending() ) {
 					$conn->commit( __METHOD__, 'flush' );
 				}
 			}
-- 
2.20.1