From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Wed, 26 Sep 2018 16:56:58 +0000 (-0400)
Subject: MessageCache: replace should actually replace, not reload
X-Git-Tag: 1.34.0-rc.0~3800^2
X-Git-Url: https://git.cyclocoop.org/%27.WWW_URL.%27admin/?a=commitdiff_plain;h=08e9eb1fef635a00a738e8785b24f3d4ae385390;p=lhc%2Fweb%2Fwiklou.git

MessageCache: replace should actually replace, not reload

Prior to I462554b30, MessageCache::replace() did just that: it took the
existing cache and updated the one entry.

In I462554b30, the rearrangement of work into a DeferredUpdate
introduced a bug: the in-process cache was updated, but when the shared
cache was loaded later the entry was never updated in there so the
shared caches kept the old value. This was found in code review and
worked around by reloading all the messages from the database instead of
updating the existing cache.

But all that extra work reloading everything from the database causes
major slowness saving any MediaWiki-namespace page when the wiki has
many such small pages. Let's go back and fix the bug so replace() can
again replace instead of reloading everything.

Bug: T203925
Change-Id: Ife8e1bd6f143f480eb8da09b817c85aadf33a923
---

diff --git a/includes/cache/MessageCache.php b/includes/cache/MessageCache.php
index 76d31ff871..934e4a6ea6 100644
--- a/includes/cache/MessageCache.php
+++ b/includes/cache/MessageCache.php
@@ -613,11 +613,17 @@ class MessageCache {
 			return;
 		}
 
-		// Reload messages from the database and pre-populate dc-local caches
-		// as optimisation. Use the master DB to avoid race conditions.
-		$cache = $this->loadFromDB( $code, self::FOR_UPDATE );
+		// Load the existing cache to update it in the local DC cache.
+		// The other DCs will see a hash mismatch.
+		if ( $this->load( $code, self::FOR_UPDATE ) ) {
+			$cache = $this->cache->get( $code );
+		} else {
+			// Err? Fall back to loading from the database.
+			$cache = $this->loadFromDB( $code, self::FOR_UPDATE );
+		}
 		// Check if individual cache keys should exist and update cache accordingly
 		$newTextByTitle = []; // map of (title => content)
+		$newBigTitles = []; // map of (title => latest revision ID), like EXCESSIVE in loadFromDB()
 		foreach ( $replacements as list( $title ) ) {
 			$page = WikiPage::factory( Title::makeTitle( NS_MEDIAWIKI, $title ) );
 			$page->loadPageData( $page::READ_LATEST );
@@ -625,15 +631,29 @@ class MessageCache {
 			// Remember the text for the blob store update later on
 			$newTextByTitle[$title] = $text;
 			// Note that if $text is false, then $cache should have a !NONEXISTANT entry
-			if ( is_string( $text ) && strlen( $text ) > $wgMaxMsgCacheEntrySize ) {
-				// Match logic of loadCachedMessagePageEntry()
-				$this->wanCache->set(
-					$this->bigMessageCacheKey( $cache['HASH'], $title ),
-					' ' . $text,
-					$this->mExpiry
-				);
+			if ( !is_string( $text ) ) {
+				$cache[$title] = '!NONEXISTENT';
+			} elseif ( strlen( $text ) > $wgMaxMsgCacheEntrySize ) {
+				$cache[$title] = '!TOO BIG';
+				$newBigTitles[$title] = $page->getLatest();
+			} else {
+				$cache[$title] = ' ' . $text;
 			}
 		}
+		// Update HASH for the new key. Incorporates various administrative keys,
+		// including the old HASH (and thereby the EXCESSIVE value from loadFromDB()
+		// and previous replace() calls), but that doesn't really matter since we
+		// only ever compare it for equality with a copy saved by saveToCaches().
+		$cache['HASH'] = md5( serialize( $cache + [ 'EXCESSIVE' => $newBigTitles ] ) );
+		// Update the too-big WAN cache entries now that we have the new HASH
+		foreach ( $newBigTitles as $title => $id ) {
+			// Match logic of loadCachedMessagePageEntry()
+			$this->wanCache->set(
+				$this->bigMessageCacheKey( $cache['HASH'], $title ),
+				' ' . $newTextByTitle[$title],
+				$this->mExpiry
+			);
+		}
 		// Mark this cache as definitely being "latest" (non-volatile) so
 		// load() calls do not try to refresh the cache with replica DB data
 		$cache['LATEST'] = time();
diff --git a/tests/phpunit/includes/cache/MessageCacheTest.php b/tests/phpunit/includes/cache/MessageCacheTest.php
index 16448eedc8..661f325149 100644
--- a/tests/phpunit/includes/cache/MessageCacheTest.php
+++ b/tests/phpunit/includes/cache/MessageCacheTest.php
@@ -129,6 +129,51 @@ class MessageCacheTest extends MediaWikiLangTestCase {
 		$this->assertEquals( $oldText, $messageCache->get( $message ), 'Content restored' );
 	}
 
+	public function testReplaceCache() {
+		global $wgWANObjectCaches;
+
+		// We need a WAN cache for this.
+		$this->setMwGlobals( [
+			'wgMainWANCache' => 'hash',
+			'wgWANObjectCaches' => $wgWANObjectCaches + [
+				'hash' => [
+					'class'    => WANObjectCache::class,
+					'cacheId'  => 'hash',
+					'channels' => []
+				]
+			]
+		] );
+		$this->overrideMwServices();
+
+		MessageCache::destroyInstance();
+		$messageCache = MessageCache::singleton();
+		$messageCache->enable();
+
+		// Populate one key
+		$this->makePage( 'Key1', 'de', 'Value1' );
+		$this->assertEquals( 0,
+			DeferredUpdates::pendingUpdatesCount(),
+			'Post-commit deferred update triggers a run of all updates' );
+		$this->assertEquals( 'Value1', $messageCache->get( 'Key1' ), 'Key1 was successfully edited' );
+
+		// Screw up the database so MessageCache::loadFromDB() will
+		// produce the wrong result for reloading Key1
+		$this->db->delete(
+			'page', [ 'page_namespace' => NS_MEDIAWIKI, 'page_title' => 'Key1' ], __METHOD__
+		);
+
+		// Populate the second key
+		$this->makePage( 'Key2', 'de', 'Value2' );
+		$this->assertEquals( 0,
+			DeferredUpdates::pendingUpdatesCount(),
+			'Post-commit deferred update triggers a run of all updates' );
+		$this->assertEquals( 'Value2', $messageCache->get( 'Key2' ), 'Key2 was successfully edited' );
+
+		// Now test that the second edit didn't reload Key1
+		$this->assertEquals( 'Value1', $messageCache->get( 'Key1' ),
+			'Key1 wasn\'t reloaded by edit of Key2' );
+	}
+
 	/**
 	 * @dataProvider provideNormalizeKey
 	 */