From: Aaron Schulz <aschulz@wikimedia.org>
Date: Tue, 19 Mar 2019 03:31:54 +0000 (-0700)
Subject: Alter two uses of hasOrMadeRecentMasterChanges() for consistency
X-Git-Tag: 1.34.0-rc.0~1891^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=7a69b598f2450c32ba988bdbcef262752e298485

Alter two uses of hasOrMadeRecentMasterChanges() for consistency

Rather than have the behavior vary and possibly break code or tests
when small changes happen, make User/NameTableStore more explicit
about when cache key purges happens.

This should reduce problems with certain fragile tests, such as those
that could be affected by 03908112635f when --use-normal tables is not
used. Ideally, any fragility should be ironed out of effected code.

Change-Id: Ibe5d1bb4bece2526bc0da99648f7ba73bdc0ffa5
---

diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php
index bf88b20203..a14e339020 100644
--- a/includes/Storage/NameTableStore.php
+++ b/includes/Storage/NameTableStore.php
@@ -183,11 +183,10 @@ class NameTableStore {
 				$searchResult = $id;
 
 				// As store returned an ID we know we inserted so delete from WAN cache
-				$this->purgeWANCache(
-					function () {
-						$this->cache->delete( $this->getCacheKey() );
-					}
-				);
+				$dbw = $this->getDBConnection( DB_MASTER );
+				$dbw->onTransactionPreCommitOrIdle( function () {
+					$this->cache->delete( $this->getCacheKey() );
+				} );
 			}
 			$this->tableCache = $table;
 		}
@@ -208,14 +207,11 @@ class NameTableStore {
 	 * @return string[] The freshly reloaded name map
 	 */
 	public function reloadMap( $connFlags = 0 ) {
-		$this->tableCache = $this->loadTable(
-			$this->getDBConnection( DB_MASTER, $connFlags )
-		);
-		$this->purgeWANCache(
-			function () {
-				$this->cache->reap( $this->getCacheKey(), INF );
-			}
-		);
+		$dbw = $this->getDBConnection( DB_MASTER, $connFlags );
+		$this->tableCache = $this->loadTable( $dbw );
+		$dbw->onTransactionPreCommitOrIdle( function () {
+			$this->cache->reap( $this->getCacheKey(), INF );
+		} );
 
 		return $this->tableCache;
 	}
@@ -342,22 +338,6 @@ class NameTableStore {
 		return $table;
 	}
 
-	/**
-	 * Reap the WANCache entry for this table.
-	 *
-	 * @param callable $purgeCallback Callback to 'purge' the WAN cache
-	 */
-	private function purgeWANCache( $purgeCallback ) {
-		// If the LB has no DB changes don't bother with onTransactionPreCommitOrIdle
-		if ( !$this->loadBalancer->hasOrMadeRecentMasterChanges() ) {
-			$purgeCallback();
-			return;
-		}
-
-		$this->getDBConnection( DB_MASTER )
-			->onTransactionPreCommitOrIdle( $purgeCallback, __METHOD__ );
-	}
-
 	/**
 	 * Gets the table from the db
 	 *
diff --git a/includes/user/User.php b/includes/user/User.php
index 33d216d19c..ba6bb08a73 100644
--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -1706,7 +1706,7 @@ class User implements IDBAccessObject, UserIdentity {
 
 		if ( $success ) {
 			$this->mTouched = $newTouched;
-			$this->clearSharedCache();
+			$this->clearSharedCache( 'changed' );
 		} else {
 			// Clears on failure too since that is desired if the cache is stale
 			$this->clearSharedCache( 'refresh' );
@@ -2762,29 +2762,26 @@ class User implements IDBAccessObject, UserIdentity {
 	 *
 	 * Called implicitly from invalidateCache() and saveSettings().
 	 *
-	 * @param string $mode Use 'refresh' to clear now; otherwise before DB commit
+	 * @param string $mode Use 'refresh' to clear now or 'changed' to clear before DB commit
 	 */
-	public function clearSharedCache( $mode = 'changed' ) {
+	public function clearSharedCache( $mode = 'refresh' ) {
 		if ( !$this->getId() ) {
 			return;
 		}
 
+		$lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
 		$cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
 		$key = $this->getCacheKey( $cache );
+
 		if ( $mode === 'refresh' ) {
-			$cache->delete( $key, 1 );
+			$cache->delete( $key, 1 ); // low tombstone/"hold-off" TTL
 		} else {
-			$lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
-			if ( $lb->hasOrMadeRecentMasterChanges() ) {
-				$lb->getConnection( DB_MASTER )->onTransactionPreCommitOrIdle(
-					function () use ( $cache, $key ) {
-						$cache->delete( $key );
-					},
-					__METHOD__
-				);
-			} else {
-				$cache->delete( $key );
-			}
+			$lb->getConnection( DB_MASTER )->onTransactionPreCommitOrIdle(
+				function () use ( $cache, $key ) {
+					$cache->delete( $key );
+				},
+				__METHOD__
+			);
 		}
 	}
 
@@ -2795,7 +2792,7 @@ class User implements IDBAccessObject, UserIdentity {
 	 */
 	public function invalidateCache() {
 		$this->touch();
-		$this->clearSharedCache();
+		$this->clearSharedCache( 'changed' );
 	}
 
 	/**
@@ -4246,7 +4243,7 @@ class User implements IDBAccessObject, UserIdentity {
 		$this->saveOptions();
 
 		Hooks::run( 'UserSaveSettings', [ $this ] );
-		$this->clearSharedCache();
+		$this->clearSharedCache( 'changed' );
 		$this->getUserPage()->purgeSquid();
 	}
 
diff --git a/tests/phpunit/includes/Storage/NameTableStoreTest.php b/tests/phpunit/includes/Storage/NameTableStoreTest.php
index f42e557b77..ca87b49a37 100644
--- a/tests/phpunit/includes/Storage/NameTableStoreTest.php
+++ b/tests/phpunit/includes/Storage/NameTableStoreTest.php
@@ -80,6 +80,14 @@ class NameTableStoreTest extends MediaWikiTestCase {
 			->willReturnCallback( function ( ...$args ) {
 				return call_user_func_array( [ $this->db, 'insertId' ], $args );
 			} );
+		$mock->expects( $this->any() )
+			->method( 'query' )
+			->willReturn( [] );
+		$mock->expects( $this->any() )
+			->method( 'isOpen' )
+			->willReturn( true );
+		$wrapper = TestingAccessWrapper::newFromObject( $mock );
+		$wrapper->queryLogger = new NullLogger();
 		return $mock;
 	}