From 3bbccc8da64b65038aed309a03458e064e6dad14 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Sat, 27 Feb 2016 19:35:21 +0000 Subject: [PATCH] User: Simplify process cache by using WANObjectCache::getWithSetCallback Follows-up 7d67b4d919, 9c733318. * Convert loadFromId() to use getWithSetCallback() and centralise cache access logic there instead of spread between loadFromCache() and saveToCache(). * Remove process cache from User class (added in 9c733318). Instead, tell WANObjectCache to process-cache the key for 30 seconds. * No need to deal with process cache in purge() because load uses slaves by default and may be lagged. Reads that require READ_LATEST already bypass the cache. * Remove saveToCache() and move logic to loadFromCache(). It was technically a public method, but marked private and no longer used in any extensions. * Remove redundant isAnon() check in loadFromCache(). This is already done by loadFromId() and loadFromDatabase(). * Remove hasOrMadeRecentMasterChanges() check. It was used to add READ_LATEST to the flags. However, this check only occurred if either READ_LATEST was already set, or after consulting cache. Which means in general, it never does anything. If we want to keep this, we should probably move it higher up. * Let WANObjectCache handle cache version. That way, there is no longer separate logic for "populate cache" and "cache lookup failed". Instead, there is just "get data" that tries cache first. I've considered moving the version into the cache key (like we do elsewhere) but that would be problematic here since User cache must be purgeable cross-wiki and other wikis may run a different version (either in general, or even just during a deployment). As such, the key must remain unchanged when the version changes so that purges from newer wikis affect what older wikis see and vice versa. Change-Id: Icfbc54dfd0ea594dd52fc1cfd403a7f66f1dd0b0 --- includes/user/User.php | 109 +++++++--------------------- tests/phpunit/MediaWikiTestCase.php | 6 -- 2 files changed, 28 insertions(+), 87 deletions(-) diff --git a/includes/user/User.php b/includes/user/User.php index ff3171ef20..9e50f36a23 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -198,12 +198,6 @@ class User implements IDBAccessObject { */ protected static $mAllRights = false; - /** - * An in-process cache for user data lookup - * @var HashBagOStuff - */ - protected static $inProcessCache; - /** Cache variables */ // @{ /** @var int */ @@ -425,6 +419,7 @@ class User implements IDBAccessObject { */ public function loadFromId( $flags = self::READ_NORMAL ) { if ( $this->mId == 0 ) { + // Anonymous users are not in the database (don't need cache) $this->loadDefaults(); return false; } @@ -432,17 +427,13 @@ class User implements IDBAccessObject { // Try cache (unless this needs data from the master DB). // NOTE: if this thread called saveSettings(), the cache was cleared. $latest = DBAccessObjectUtils::hasFlags( $flags, self::READ_LATEST ); - if ( $latest || !$this->loadFromCache() ) { - wfDebug( "User: cache miss for user {$this->mId}\n" ); - // Load from DB (make sure this thread sees its own changes) - if ( wfGetLB()->hasOrMadeRecentMasterChanges() ) { - $flags |= self::READ_LATEST; - } + if ( $latest ) { if ( !$this->loadFromDatabase( $flags ) ) { - // Can't load from ID, user is anonymous + // Can't load from ID return false; } - $this->saveToCache(); + } else { + $this->loadFromCache(); } $this->mLoadedItems = true; @@ -458,10 +449,8 @@ class User implements IDBAccessObject { */ public static function purge( $wikiId, $userId ) { $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); $key = $cache->makeGlobalKey( 'user', 'id', $wikiId, $userId ); $cache->delete( $key ); - $processCache->delete( $key ); } /** @@ -473,81 +462,42 @@ class User implements IDBAccessObject { return $cache->makeGlobalKey( 'user', 'id', wfWikiID(), $this->mId ); } - /** - * @since 1.27 - * @return HashBagOStuff - */ - protected static function getInProcessCache() { - if ( !self::$inProcessCache ) { - self::$inProcessCache = new HashBagOStuff( [ 'maxKeys' => 10 ] ); - } - return self::$inProcessCache; - } - /** * Load user data from shared cache, given mId has already been set. * - * @return bool false if the ID does not exist or data is invalid, true otherwise + * @return bool True * @since 1.25 */ protected function loadFromCache() { - if ( $this->mId == 0 ) { - $this->loadDefaults(); - return false; - } - $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); - $key = $this->getCacheKey( $cache ); - $data = $processCache->get( $key ); - if ( !is_array( $data ) ) { - $data = $cache->get( $key ); - if ( !is_array( $data ) - || !isset( $data['mVersion'] ) - || $data['mVersion'] < self::VERSION - ) { - // Object is expired - return false; - } - $processCache->set( $key, $data ); - } - wfDebug( "User: got user {$this->mId} from cache\n" ); + $data = $cache->getWithSetCallback( + $this->getCacheKey( $cache ), + $cache::TTL_HOUR, + function ( $oldValue, &$ttl, array &$setOpts ) { + $setOpts += Database::getCacheSetOptions( wfGetDB( DB_SLAVE ) ); + wfDebug( "User: cache miss for user {$this->mId}\n" ); - // Restore from cache - foreach ( self::$mCacheVars as $name ) { - $this->$name = $data[$name]; - } + $this->loadFromDatabase(); + $this->loadGroups(); + $this->loadOptions(); - return true; - } + $data = []; + foreach ( self::$mCacheVars as $name ) { + $data[$name] = $this->$name; + } - /** - * Save user data to the shared cache - * - * This method should not be called outside the User class - */ - public function saveToCache() { - $this->load(); - $this->loadGroups(); - $this->loadOptions(); + return $data; - if ( $this->isAnon() ) { - // Anonymous users are uncached - return; - } + }, + [ 'pcTTL' => $cache::TTL_PROC_LONG, 'version' => self::VERSION ] + ); - $data = []; + // Restore from cache foreach ( self::$mCacheVars as $name ) { - $data[$name] = $this->$name; + $this->$name = $data[$name]; } - $data['mVersion'] = self::VERSION; - $opts = Database::getCacheSetOptions( wfGetDB( DB_SLAVE ) ); - $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); - $key = $this->getCacheKey( $cache ); - $cache->set( $key, $data, $cache::TTL_HOUR, $opts ); - $processCache->set( $key, $data ); + return true; } /** @name newFrom*() static factory methods */ @@ -1269,8 +1219,8 @@ class User implements IDBAccessObject { // Paranoia $this->mId = intval( $this->mId ); - // Anonymous user if ( !$this->mId ) { + // Anonymous users are not in the database $this->loadDefaults(); return false; } @@ -2396,16 +2346,13 @@ class User implements IDBAccessObject { } $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); $key = $this->getCacheKey( $cache ); if ( $mode === 'refresh' ) { $cache->delete( $key, 1 ); - $processCache->delete( $key ); } else { wfGetDB( DB_MASTER )->onTransactionPreCommitOrIdle( - function() use ( $cache, $processCache, $key ) { + function() use ( $cache, $key ) { $cache->delete( $key ); - $processCache->delete( $key ); } ); } diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php index 47d96e1878..e0a7ea367e 100644 --- a/tests/phpunit/MediaWikiTestCase.php +++ b/tests/phpunit/MediaWikiTestCase.php @@ -1215,12 +1215,6 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase { // If any of the user tables were marked as used, we should clear all of them. if ( array_intersect( $tablesUsed, $userTables ) ) { $tablesUsed = array_unique( array_merge( $tablesUsed, $userTables ) ); - - // Totally clear User class in-process cache to avoid CAS errors - TestingAccessWrapper::newFromClass( 'User' ) - ->getInProcessCache() - ->clear(); - TestUserRegistry::clear(); } -- 2.20.1