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

diff --git a/includes/user/User.php b/includes/user/User.php
index ff3171e..9e50f36 100644 (file)
--- 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 47d96e1..e0a7ea3 100644 (file)
--- 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();
                        }