From 8f47c177e3b3b454c84bff4259761b864118d9d8 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 22 Aug 2016 22:04:43 -0700
Subject: [PATCH] A few more DBLockManager fixes and cleanups

* Do not do the connection init step if the same DB handle as
  wfGetDB( DB_MASTER ) is being used to avoid clobbering it.
* Remove begin(), since only one of the subclasses wants
  transactions. That one now uses startAtomic() now.
* Make getConnection() throw an error for bad config instead
  of return null, which was not documented or expected.

Change-Id: Ib09a7972d6569c29e83e329a8f7f9f47a393b896
---
 .../filebackend/lockmanager/DBLockManager.php | 39 +++++++++++--------
 .../lockmanager/MySqlLockManager.php          |  2 +
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/includes/filebackend/lockmanager/DBLockManager.php b/includes/filebackend/lockmanager/DBLockManager.php
index 0aefbfa623..cccf71a929 100644
--- a/includes/filebackend/lockmanager/DBLockManager.php
+++ b/includes/filebackend/lockmanager/DBLockManager.php
@@ -144,33 +144,38 @@ abstract class DBLockManager extends QuorumLockManager {
 	 * @param string $lockDb
 	 * @return IDatabase
 	 * @throws DBError
+	 * @throws UnexpectedValueException
 	 */
 	protected function getConnection( $lockDb ) {
 		if ( !isset( $this->conns[$lockDb] ) ) {
-			$db = null;
 			if ( $lockDb === 'localDBMaster' ) {
-				$db = $this->getLocalLB()->getConnection( DB_MASTER, [], $this->domain );
+				$lb = $this->getLocalLB();
+				$db = $lb->getConnection( DB_MASTER, [], $this->domain );
+				# Do not mess with settings if the LoadBalancer is the main singleton
+				# to avoid clobbering the settings of handles from wfGetDB( DB_MASTER ).
+				$init = ( wfGetLB() !== $lb );
 			} elseif ( isset( $this->dbServers[$lockDb] ) ) {
 				$config = $this->dbServers[$lockDb];
 				$db = DatabaseBase::factory( $config['type'], $config );
+				$init = true;
+			} else {
+				throw new UnexpectedValueException( "No server called '$lockDb'." );
 			}
-			if ( !$db ) {
-				return null; // config error?
+
+			if ( $init ) {
+				$db->clearFlag( DBO_TRX );
+				# If the connection drops, try to avoid letting the DB rollback
+				# and release the locks before the file operations are finished.
+				# This won't handle the case of DB server restarts however.
+				$options = [];
+				if ( $this->lockExpiry > 0 ) {
+					$options['connTimeout'] = $this->lockExpiry;
+				}
+				$db->setSessionOptions( $options );
+				$this->initConnection( $lockDb, $db );
 			}
+
 			$this->conns[$lockDb] = $db;
-			$this->conns[$lockDb]->clearFlag( DBO_TRX );
-			# If the connection drops, try to avoid letting the DB rollback
-			# and release the locks before the file operations are finished.
-			# This won't handle the case of DB server restarts however.
-			$options = [];
-			if ( $this->lockExpiry > 0 ) {
-				$options['connTimeout'] = $this->lockExpiry;
-			}
-			$this->conns[$lockDb]->setSessionOptions( $options );
-			$this->initConnection( $lockDb, $this->conns[$lockDb] );
-		}
-		if ( !$this->conns[$lockDb]->trxLevel() ) {
-			$this->conns[$lockDb]->begin( __METHOD__ ); // start transaction
 		}
 
 		return $this->conns[$lockDb];
diff --git a/includes/filebackend/lockmanager/MySqlLockManager.php b/includes/filebackend/lockmanager/MySqlLockManager.php
index c51df16b89..0536091bc4 100644
--- a/includes/filebackend/lockmanager/MySqlLockManager.php
+++ b/includes/filebackend/lockmanager/MySqlLockManager.php
@@ -23,6 +23,8 @@ class MySqlLockManager extends DBLockManager {
 	protected function initConnection( $lockDb, IDatabase $db ) {
 		# Let this transaction see lock rows from other transactions
 		$db->query( "SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;" );
+		# Do everything in a transaction as it all gets rolled back eventually
+		$db->startAtomic( __CLASS__ );
 	}
 
 	/**
-- 
2.20.1