From 60013aa328bb0332730fdc4c0e1d79096b474bfa Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sun, 17 Jan 2016 02:40:53 -0800
Subject: [PATCH] Add IDatabase::getScopedLockAndFlush() method

This method is less manual and avoids the usual pitfalls of
not unlocking for a return statement or not flushing out any
prior transaction.

Change-Id: Ib1681244767de860105a68210e181e2f024ee525
---
 includes/db/DBConnRef.php                     |  4 +++
 includes/db/Database.php                      | 15 +++++++++++
 includes/db/IDatabase.php                     | 25 ++++++++++++++++---
 .../jobs/CategoryMembershipChangeJob.php      | 13 ++--------
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php
index f09de4fb39..264ee11318 100644
--- a/includes/db/DBConnRef.php
+++ b/includes/db/DBConnRef.php
@@ -505,6 +505,10 @@ class DBConnRef implements IDatabase {
 		return $this->__call( __FUNCTION__, func_get_args() );
 	}
 
+	public function getScopedLockAndFlush( $lockKey, $fname, $timeout ) {
+		return $this->__call( __FUNCTION__, func_get_args() );
+	}
+
 	public function namedLocksEnqueue() {
 		return $this->__call( __FUNCTION__, func_get_args() );
 	}
diff --git a/includes/db/Database.php b/includes/db/Database.php
index 183595820c..dc1570e1a0 100644
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -3167,6 +3167,21 @@ abstract class DatabaseBase implements IDatabase {
 		return true;
 	}
 
+	public function getScopedLockAndFlush( $lockKey, $fname, $timeout ) {
+		if ( !$this->lock( $lockKey, $fname, $timeout ) ) {
+			return null;
+		}
+
+		$that = $this;
+		$unlocker = new ScopedCallback( function () use ( $that, $lockKey, $fname ) {
+			$that->unlock( $lockKey, $fname );
+		} );
+
+		$this->commit( __METHOD__, 'flush' );
+
+		return $unlocker;
+	}
+
 	public function namedLocksEnqueue() {
 		return false;
 	}
diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index cecb6438c7..1a6e779329 100644
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -1493,8 +1493,8 @@ interface IDatabase {
 	 * Named locks are not related to transactions
 	 *
 	 * @param string $lockName Name of lock to aquire
-	 * @param string $method Name of method calling us
-	 * @param int $timeout
+	 * @param string $method Name of the calling method
+	 * @param int $timeout Acquisition timeout in seconds
 	 * @return bool
 	 */
 	public function lock( $lockName, $method, $timeout = 5 );
@@ -1505,7 +1505,7 @@ interface IDatabase {
 	 * Named locks are not related to transactions
 	 *
 	 * @param string $lockName Name of lock to release
-	 * @param string $method Name of method calling us
+	 * @param string $method Name of the calling method
 	 *
 	 * @return int Returns 1 if the lock was released, 0 if the lock was not established
 	 * by this thread (in which case the lock is not released), and NULL if the named
@@ -1513,6 +1513,25 @@ interface IDatabase {
 	 */
 	public function unlock( $lockName, $method );
 
+	/**
+	 * Acquire a named lock, flush any transaction, and return an RAII style unlocker object
+	 *
+	 * This is suitiable for transactions that need to be serialized using cooperative locks,
+	 * where each transaction can see each others' changes. Any transaction is flushed to clear
+	 * out stale REPEATABLE-READ snapshot data. Once the returned object falls out of PHP scope,
+	 * the lock will be released.
+	 *
+	 * If the lock acquisition failed, then no transaction flush happens, and null is returned.
+	 *
+	 * @param string $lockKey Name of lock to release
+	 * @param string $fname Name of the calling method
+	 * @param int $timeout Acquisition timeout in seconds
+	 * @return ScopedCallback|null
+	 * @throws DBUnexpectedError
+	 * @since 1.27
+	 */
+	public function getScopedLockAndFlush( $lockKey, $fname, $timeout );
+
 	/**
 	 * Check to see if a named lock used by lock() use blocking queues
 	 *
diff --git a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php
index 98c87a5ace..a6869c1e11 100644
--- a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php
+++ b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php
@@ -51,20 +51,13 @@ class CategoryMembershipChangeJob extends Job {
 		$dbw = wfGetDB( DB_MASTER );
 
 		// Use a named lock so that jobs for this page see each others' changes
-		$fname = __METHOD__;
 		$lockKey = "CategoryMembershipUpdates:{$page->getId()}";
-		if ( !$dbw->lock( $lockKey, $fname, 10 ) ) {
+		$scopedLock = $dbw->getScopedLockAndFlush( $lockKey, __METHOD__, 10 );
+		if ( !$scopedLock ) {
 			$this->setLastError( "Could not acquire lock '$lockKey'" );
 			return false;
 		}
 
-		$unlocker = new ScopedCallback( function () use ( $dbw, $lockKey, $fname ) {
-			$dbw->unlock( $lockKey, $fname );
-		} );
-
-		// Sanity: clear any DB transaction snapshot
-		$dbw->commit( __METHOD__, 'flush' );
-
 		$cutoffUnix = wfTimestamp( TS_UNIX, $this->params['revTimestamp'] );
 		// Using ENQUEUE_FUDGE_SEC handles jobs inserted out of revision order due to the delay
 		// between COMMIT and actual enqueueing of the CategoryMembershipChangeJob job.
@@ -121,8 +114,6 @@ class CategoryMembershipChangeJob extends Job {
 			$this->notifyUpdatesForRevision( $page, Revision::newFromRow( $row ) );
 		}
 
-		ScopedCallback::consume( $unlocker );
-
 		return true;
 	}
 
-- 
2.20.1