From dad254dfabbc3f6edee2a209254c783021b8b834 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Tue, 19 Apr 2016 08:58:49 -0700
Subject: [PATCH] Avoid lock error exceptions during upgradeRow() contention

Bug: T132921
Change-Id: I229031c3d4ae5b700fcc4d4dd3f5208a853823dc
---
 autoload.php                         |  1 +
 includes/filerepo/file/LocalFile.php | 36 +++++++++++++++++++---------
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/autoload.php b/autoload.php
index f802ddd874..3b17215365 100644
--- a/autoload.php
+++ b/autoload.php
@@ -711,6 +711,7 @@ $wgAutoloadLocalClasses = [
 	'LoadMonitorNull' => __DIR__ . '/includes/db/loadbalancer/LoadMonitor.php',
 	'LocalFile' => __DIR__ . '/includes/filerepo/file/LocalFile.php',
 	'LocalFileDeleteBatch' => __DIR__ . '/includes/filerepo/file/LocalFile.php',
+	'LocalFileLockError' => __DIR__ . '/includes/filerepo/file/LocalFile.php',
 	'LocalFileMoveBatch' => __DIR__ . '/includes/filerepo/file/LocalFile.php',
 	'LocalFileRestoreBatch' => __DIR__ . '/includes/filerepo/file/LocalFile.php',
 	'LocalIdLookup' => __DIR__ . '/includes/user/LocalIdLookup.php',
diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index aa278aadf9..f7275fc02c 100644
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -559,11 +559,11 @@ class LocalFile extends File {
 			return;
 		}
 
+		$upgrade = false;
 		if ( is_null( $this->media_type ) ||
 			$this->mime == 'image/svg'
 		) {
-			$this->upgradeRow();
-			$this->upgraded = true;
+			$upgrade = true;
 		} else {
 			$handler = $this->getHandler();
 			if ( $handler ) {
@@ -571,11 +571,19 @@ class LocalFile extends File {
 				if ( $validity === MediaHandler::METADATA_BAD
 					|| ( $validity === MediaHandler::METADATA_COMPATIBLE && $wgUpdateCompatibleMetadata )
 				) {
-					$this->upgradeRow();
-					$this->upgraded = true;
+					$upgrade = true;
 				}
 			}
 		}
+
+		if ( $upgrade ) {
+			try {
+				$this->upgradeRow();
+			} catch ( LocalFileLockError $e ) {
+				// let the other process handle it (or do it next time)
+			}
+			$this->upgraded = true; // avoid rework/retries
+		}
 	}
 
 	function getUpgraded() {
@@ -586,7 +594,6 @@ class LocalFile extends File {
 	 * Fix assorted version-related problems with the image row by reloading it from the file
 	 */
 	function upgradeRow() {
-
 		$this->lock(); // begin
 
 		$this->loadFromFile();
@@ -1898,18 +1905,16 @@ class LocalFile extends File {
 	/**
 	 * Start a transaction and lock the image for update
 	 * Increments a reference counter if the lock is already held
-	 * @throws MWException Throws an error if the lock was not acquired
+	 * @throws LocalFileLockError Throws an error if the lock was not acquired
 	 * @return bool Whether the file lock owns/spawned the DB transaction
 	 */
 	function lock() {
-		$dbw = $this->repo->getMasterDB();
-
 		if ( !$this->locked ) {
+			$dbw = $this->repo->getMasterDB();
 			if ( !$dbw->trxLevel() ) {
 				$dbw->begin( __METHOD__ );
 				$this->lockedOwnTrx = true;
 			}
-			$this->locked++;
 			// Bug 54736: use simple lock to handle when the file does not exist.
 			// SELECT FOR UPDATE prevents changes, not other SELECTs with FOR UPDATE.
 			// Also, that would cause contention on INSERT of similarly named rows.
@@ -1917,10 +1922,15 @@ class LocalFile extends File {
 			$lockPaths = [ $this->getPath() ]; // represents all versions of the file
 			$status = $backend->lockFiles( $lockPaths, LockManager::LOCK_EX, 5 );
 			if ( !$status->isGood() ) {
-				throw new MWException( "Could not acquire lock for '{$this->getName()}.'" );
+				if ( $this->lockedOwnTrx ) {
+					$dbw->rollback( __METHOD__ );
+				}
+				throw new LocalFileLockError( "Could not acquire lock for '{$this->getName()}.'" );
 			}
+			// Release the lock *after* commit to avoid row-level contention
+			$this->locked++;
 			$dbw->onTransactionIdle( function () use ( $backend, $lockPaths ) {
-				$backend->unlockFiles( $lockPaths, LockManager::LOCK_EX ); // release on commit
+				$backend->unlockFiles( $lockPaths, LockManager::LOCK_EX );
 			} );
 		}
 
@@ -3031,3 +3041,7 @@ class LocalFileMoveBatch {
 		$this->file->repo->cleanupBatch( $files );
 	}
 }
+
+class LocalFileLockError extends Exception {
+
+}
-- 
2.20.1