From 1f313389e29d170a7d8ee3c40a979f4dd18fc83b Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 19 Aug 2016 13:17:33 -0700
Subject: [PATCH] Remove direct rollback() calls from some places

Rely on the mass-rollback logic in MWExceptionHandler instead.
This results in a better chance of atomicity.

Change-Id: I2eb5661d4acc105a1323d69c5463268c234bd745
---
 includes/FileDeleteForm.php           | 39 +++++++++++----------------
 includes/jobqueue/JobQueueDB.php      |  5 +---
 includes/objectcache/SqlBagOStuff.php |  8 ++++++
 includes/page/WikiPage.php            |  2 --
 4 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/includes/FileDeleteForm.php b/includes/FileDeleteForm.php
index 361058b17a..65638f286f 100644
--- a/includes/FileDeleteForm.php
+++ b/includes/FileDeleteForm.php
@@ -189,31 +189,24 @@ class FileDeleteForm {
 			);
 			$page = WikiPage::factory( $title );
 			$dbw = wfGetDB( DB_MASTER );
-			try {
-				$dbw->startAtomic( __METHOD__ );
-				// delete the associated article first
-				$error = '';
-				$deleteStatus = $page->doDeleteArticleReal( $reason, $suppress, 0, false, $error, $user );
-				// doDeleteArticleReal() returns a non-fatal error status if the page
-				// or revision is missing, so check for isOK() rather than isGood()
-				if ( $deleteStatus->isOK() ) {
-					$status = $file->delete( $reason, $suppress, $user );
-					if ( $status->isOK() ) {
-						$status->value = $deleteStatus->value; // log id
-						$dbw->endAtomic( __METHOD__ );
-					} else {
-						// Page deleted but file still there? rollback page delete
-						wfGetLBFactory()->rollbackMasterChanges( __METHOD__ );
-					}
-				} else {
-					// Done; nothing changed
+			$dbw->startAtomic( __METHOD__ );
+			// delete the associated article first
+			$error = '';
+			$deleteStatus = $page->doDeleteArticleReal( $reason, $suppress, 0, false, $error, $user );
+			// doDeleteArticleReal() returns a non-fatal error status if the page
+			// or revision is missing, so check for isOK() rather than isGood()
+			if ( $deleteStatus->isOK() ) {
+				$status = $file->delete( $reason, $suppress, $user );
+				if ( $status->isOK() ) {
+					$status->value = $deleteStatus->value; // log id
 					$dbw->endAtomic( __METHOD__ );
+				} else {
+					// Page deleted but file still there? rollback page delete
+					wfGetLBFactory()->rollbackMasterChanges( __METHOD__ );
 				}
-			} catch ( Exception $e ) {
-				// Rollback before returning to prevent UI from displaying
-				// incorrect "View or restore N deleted edits?"
-				$dbw->rollback( __METHOD__ );
-				throw $e;
+			} else {
+				// Done; nothing changed
+				$dbw->endAtomic( __METHOD__ );
 			}
 		}
 
diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php
index 479ec3289c..b9f4592ffc 100644
--- a/includes/jobqueue/JobQueueDB.php
+++ b/includes/jobqueue/JobQueueDB.php
@@ -245,10 +245,7 @@ class JobQueueDB extends JobQueue {
 				count( $rowSet ) + count( $rowList ) - count( $rows )
 			);
 		} catch ( DBError $e ) {
-			if ( $flags & self::QOS_ATOMIC ) {
-				$dbw->rollback( $method );
-			}
-			throw $e;
+			$this->throwDBException( $e );
 		}
 		if ( $flags & self::QOS_ATOMIC ) {
 			$dbw->endAtomic( $method );
diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php
index 5556dd8923..84936a4db4 100644
--- a/includes/objectcache/SqlBagOStuff.php
+++ b/includes/objectcache/SqlBagOStuff.php
@@ -695,6 +695,7 @@ class SqlBagOStuff extends BagOStuff {
 	 *
 	 * @param DBError $exception
 	 * @param int $serverIndex
+	 * @throws Exception
 	 */
 	protected function handleWriteError( DBError $exception, $serverIndex ) {
 		if ( $exception instanceof DBConnectionError ) {
@@ -702,6 +703,13 @@ class SqlBagOStuff extends BagOStuff {
 		}
 		if ( $exception->db && $exception->db->wasReadOnlyError() ) {
 			if ( $exception->db->trxLevel() ) {
+				if ( !$this->serverInfos ) {
+					// Errors like deadlocks and connection drops already cause rollback.
+					// For consistency, we have no choice but to throw an error and trigger
+					// complete rollback if the main DB is also being used as the cache DB.
+					throw $exception;
+				}
+
 				try {
 					$exception->db->rollback( __METHOD__ );
 				} catch ( DBError $e ) {
diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index 0344756f0f..033eb67eb9 100644
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -1770,7 +1770,6 @@ class WikiPage implements Page, IDBAccessObject {
 			$revisionId = $revision->insertOn( $dbw );
 			// Update page_latest and friends to reflect the new revision
 			if ( !$this->updateRevisionOn( $dbw, $revision, null, $meta['oldIsRedirect'] ) ) {
-				$dbw->rollback( __METHOD__ ); // sanity; this should never happen
 				throw new MWException( "Failed to update page row to use new revision." );
 			}
 
@@ -1920,7 +1919,6 @@ class WikiPage implements Page, IDBAccessObject {
 		$revisionId = $revision->insertOn( $dbw );
 		// Update the page record with revision data
 		if ( !$this->updateRevisionOn( $dbw, $revision, 0 ) ) {
-			$dbw->rollback( __METHOD__ ); // sanity; this should never happen
 			throw new MWException( "Failed to update page row to use new revision." );
 		}
 
-- 
2.20.1