From: daniel <daniel.kinzler@wikimedia.de>
Date: Thu, 19 May 2016 12:51:04 +0000 (+0200)
Subject: Disable CAS check when saving TestUser data.
X-Git-Tag: 1.31.0-rc.0~6844^2
X-Git-Url: http://git.cyclocoop.org/%24image?a=commitdiff_plain;h=8f4587b34381926188f3d30a0f981cedd0f69235;p=lhc%2Fweb%2Fwiklou.git

Disable CAS check when saving TestUser data.

During testing, we are not worried about data loss, so we can safely
bypass the CAS check when setting up a test fixture.

This change was added to address sporadic test failures like the following:

18:03:38 1) ApiEchoMarkReadTest::testMarkReadWithList
18:03:38 MWException: CAS update failed on user_touched for user ID '2' (read from slave); the version of the user to be saved is older than the current version.
18:03:38
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/includes/user/User.php:3931
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/includes/TestUser.php:83
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/includes/api/ApiTestCase.php:30
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/Echo/tests/phpunit/api/ApiEchoMarkReadTest.php:11
18:03:38 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:370

Bug: T131178
Change-Id: I99b43e0db85bc2c1cd335c82971df4e95520d34b
---

diff --git a/includes/user/User.php b/includes/user/User.php
index ce2ac830f0..eb5fdafa14 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1467,6 +1467,24 @@ class User implements IDBAccessObject {
 		return $toPromote;
 	}
 
+	/**
+	 * Builds update conditions. Additional conditions may be added to $conditions to
+	 * protected against race conditions using a compare-and-set (CAS) mechanism
+	 * based on comparing $this->mTouched with the user_touched field.
+	 *
+	 * @param DatabaseBase $db
+	 * @param array $conditions WHERE conditions for use with DatabaseBase::update
+	 * @return array WHERE conditions for use with DatabaseBase::update
+	 */
+	protected function makeUpdateConditions( DatabaseBase $db, array $conditions ) {
+		if ( $this->mTouched ) {
+			// CAS check: only update if the row wasn't changed sicne it was loaded.
+			$conditions['user_touched'] = $db->timestamp( $this->mTouched );
+		}
+
+		return $conditions;
+	}
+
 	/**
 	 * Bump user_touched if it didn't change since this object was loaded
 	 *
@@ -1484,16 +1502,14 @@ class User implements IDBAccessObject {
 		}
 
 		// Get a new user_touched that is higher than the old one
-		$oldTouched = $this->mTouched;
 		$newTouched = $this->newTouchedTimestamp();
 
 		$dbw = wfGetDB( DB_MASTER );
 		$dbw->update( 'user',
 			[ 'user_touched' => $dbw->timestamp( $newTouched ) ],
-			[
+			$this->makeUpdateConditions( $dbw, [
 				'user_id' => $this->mId,
-				'user_touched' => $dbw->timestamp( $oldTouched ) // CAS check
-			],
+			] ),
 			__METHOD__
 		);
 		$success = ( $dbw->affectedRows() > 0 );
@@ -3908,7 +3924,6 @@ class User implements IDBAccessObject {
 		// Get a new user_touched that is higher than the old one.
 		// This will be used for a CAS check as a last-resort safety
 		// check against race conditions and slave lag.
-		$oldTouched = $this->mTouched;
 		$newTouched = $this->newTouchedTimestamp();
 
 		$dbw = wfGetDB( DB_MASTER );
@@ -3922,10 +3937,9 @@ class User implements IDBAccessObject {
 				'user_token' => strval( $this->mToken ),
 				'user_email_token' => $this->mEmailToken,
 				'user_email_token_expires' => $dbw->timestampOrNull( $this->mEmailTokenExpires ),
-			], [ /* WHERE */
+			], $this->makeUpdateConditions( $dbw, [ /* WHERE */
 				'user_id' => $this->mId,
-				'user_touched' => $dbw->timestamp( $oldTouched ) // CAS check
-			], __METHOD__
+			] ), __METHOD__
 		);
 
 		if ( !$dbw->affectedRows() ) {
diff --git a/tests/phpunit/includes/TestUser.php b/tests/phpunit/includes/TestUser.php
index 247b6e4461..8b8cbcd4d9 100644
--- a/tests/phpunit/includes/TestUser.php
+++ b/tests/phpunit/includes/TestUser.php
@@ -78,6 +78,12 @@ class TestUser {
 			$this->user->removeGroup( $group );
 		}
 		if ( $change ) {
+			// Disable CAS check before saving. The User object may have been initialized from cached
+			// information that may be out of whack with the database during testing. If tests were
+			// perfectly isolated, this would not happen. But if it does happen, let's just ignore the
+			// inconsistency, and just write the data we want - during testing, we are not worried
+			// about data loss.
+			$this->user->mTouched = '';
 			$this->user->saveSettings();
 		}
 	}