From aa0f6ead348d0aa3f59d51df0806e95eb76babf8 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Tue, 6 Sep 2016 18:34:43 -0700
Subject: [PATCH] Revision: Simplify loadText() with nested getWithSetCallback

The logic was a bit hard to follow as there were two layers of
caching and a conditional (expiry being set).

* Move fetch logic for revision text to a private method.
* Call directly when cache is disabled.
* Simplify cache has/get/set by using getWithSetCallback.

Change-Id: Icd74cd8e944266c20dc7c5cb25f56300636dce1e
---
 includes/Revision.php         | 55 ++++++++++++++++++-----------------
 includes/libs/MapCacheLRU.php | 22 ++++++++++++++
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/includes/Revision.php b/includes/Revision.php
index 03c3e6d99f..bc760a3609 100644
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -1079,13 +1079,14 @@ class Revision implements IDBAccessObject {
 	}
 
 	/**
-	 * Fetch original serialized data without regard for view restrictions
+	 * Get original serialized data (without checking view restrictions)
 	 *
 	 * @since 1.21
 	 * @return string
 	 */
 	public function getSerializedData() {
 		if ( $this->mText === null ) {
+			// Revision is immutable. Load on demand.
 			$this->mText = $this->loadText();
 		}
 
@@ -1103,17 +1104,14 @@ class Revision implements IDBAccessObject {
 	 */
 	protected function getContentInternal() {
 		if ( $this->mContent === null ) {
-			// Revision is immutable. Load on demand:
-			if ( $this->mText === null ) {
-				$this->mText = $this->loadText();
-			}
+			$text = $this->getSerializedData();
 
-			if ( $this->mText !== null && $this->mText !== false ) {
+			if ( $text !== null && $text !== false ) {
 				// Unserialize content
 				$handler = $this->getContentHandler();
 				$format = $this->getContentFormat();
 
-				$this->mContent = $handler->unserializeContent( $this->mText, $format );
+				$this->mContent = $handler->unserializeContent( $text, $format );
 			}
 		}
 
@@ -1576,29 +1574,38 @@ class Revision implements IDBAccessObject {
 	 *
 	 * @return string|bool The revision's text, or false on failure
 	 */
-	protected function loadText() {
+	private function loadText() {
 		// Caching may be beneficial for massive use of external storage
 		global $wgRevisionCacheExpiry;
 		static $processCache = null;
 
+		if ( !$wgRevisionCacheExpiry ) {
+			return $this->fetchText();
+		}
+
 		if ( !$processCache ) {
 			$processCache = new MapCacheLRU( 10 );
 		}
 
 		$cache = ObjectCache::getMainWANInstance();
-		$textId = $this->getTextId();
-		$key = wfMemcKey( 'revisiontext', 'textid', $textId );
+		$key = $cache->makeKey( 'revisiontext', 'textid', $this->getTextId() );
 
-		if ( $wgRevisionCacheExpiry ) {
-			if ( $processCache->has( $key ) ) {
-				return $processCache->get( $key );
-			}
-			$text = $cache->get( $key );
-			if ( is_string( $text ) ) {
-				$processCache->set( $key, $text );
-				return $text;
-			}
-		}
+		// No negative caching; negative hits on text rows may be due to corrupted replica DBs
+		return $processCache->getWithSetCallback( $key, function () use ( $cache, $key ) {
+			global $wgRevisionCacheExpiry;
+
+			return $cache->getWithSetCallback(
+				$key,
+				$wgRevisionCacheExpiry,
+				function () {
+					return $this->fetchText();
+				}
+			);
+		} );
+	}
+
+	private function fetchText() {
+		$textId = $this->getTextId();
 
 		// If we kept data for lazy extraction, use it now...
 		if ( $this->mTextRow !== null ) {
@@ -1638,13 +1645,7 @@ class Revision implements IDBAccessObject {
 			wfDebugLog( 'Revision', "No blob for text row '$textId' (revision {$this->getId()})." );
 		}
 
-		# No negative caching -- negative hits on text rows may be due to corrupted replica DB servers
-		if ( $wgRevisionCacheExpiry && $text !== false ) {
-			$processCache->set( $key, $text );
-			$cache->set( $key, $text, $wgRevisionCacheExpiry );
-		}
-
-		return $text;
+		return is_string( $text ) ? $text : false;
 	}
 
 	/**
diff --git a/includes/libs/MapCacheLRU.php b/includes/libs/MapCacheLRU.php
index 2370ed3a8a..553ef723e4 100644
--- a/includes/libs/MapCacheLRU.php
+++ b/includes/libs/MapCacheLRU.php
@@ -102,6 +102,28 @@ class MapCacheLRU {
 		return array_keys( $this->cache );
 	}
 
+	/**
+	 * Get an item with the given key, producing and setting it if not found.
+	 *
+	 * If the callback returns false, then nothing is stored.
+	 *
+	 * @since 1.28
+	 * @param string $key
+	 * @param callable $callback Callback that will produce the value
+	 * @return mixed The cached value if found or the result of $callback otherwise
+	 */
+	public function getWithSetCallback( $key, callable $callback ) {
+		$value = $this->get( $key );
+		if ( $value === false ) {
+			$value = call_user_func( $callback );
+			if ( $value !== false ) {
+				$this->set( $key, $value );
+			}
+		}
+
+		return $value;
+	}
+
 	/**
 	 * Clear one or several cache entries, or all cache entries
 	 *
-- 
2.20.1