From f50c097b9a04a5260d03c70ff8eb58a0f1ccdf6c Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 15 Apr 2021 21:16:55 -0700 Subject: [PATCH] Keep ActorStore caches consistent on user rename Multi-key in-memory keys for the actors is complicated enough to have it's own small abstraction. Change-Id: Id0e091504b71a44ce52d418c5737d64ac70495e9 --- includes/user/ActorCache.php | 146 ++++++++++++++++++ includes/user/ActorStore.php | 121 +++++---------- includes/user/User.php | 1 + .../includes/user/ActorStoreTest.php | 17 ++ .../unit/includes/user/ActorCacheTest.php | 116 ++++++++++++++ 5 files changed, 317 insertions(+), 84 deletions(-) create mode 100644 includes/user/ActorCache.php create mode 100644 tests/phpunit/unit/includes/user/ActorCacheTest.php diff --git a/includes/user/ActorCache.php b/includes/user/ActorCache.php new file mode 100644 index 00000000000..82ad40a3397 --- /dev/null +++ b/includes/user/ActorCache.php @@ -0,0 +1,146 @@ + [], self::KEY_USER_NAME => [], self::KEY_USER_ID => [] ]; + + /** + * @param int $maxSize hold up to this many UserIdentity values + */ + public function __construct( int $maxSize ) { + $this->maxSize = $maxSize; + } + + /** + * Get user identity which has $keyType equal to $keyValue + * @param string $keyType one of self::KEY_* constants. + * @param string|int $keyValue + * @return UserIdentity|null + */ + public function getActor( string $keyType, $keyValue ): ?UserIdentity { + return $this->getCachedValue( $keyType, $keyValue )['actor'] ?? null; + } + + /** + * Get actor ID of the user which has $keyType equal to $keyValue. + * @param string $keyType one of self::KEY_* constants. + * @param string|int $keyValue + * @return int|null + */ + public function getActorId( string $keyType, $keyValue ): ?int { + return $this->getCachedValue( $keyType, $keyValue )['actorId'] ?? null; + } + + /** + * Add $actor with $actorId to the cache. + * @param int $actorId + * @param UserIdentity $actor + */ + public function add( int $actorId, UserIdentity $actor ) { + while ( count( $this->cache[self::KEY_ACTOR_ID] ) >= $this->maxSize ) { + reset( $this->cache[self::KEY_ACTOR_ID] ); + $evictId = key( $this->cache[self::KEY_ACTOR_ID] ); + $this->remove( $this->cache[self::KEY_ACTOR_ID][$evictId]['actor'] ); + } + $value = [ 'actorId' => $actorId, 'actor' => $actor ]; + $this->cache[self::KEY_ACTOR_ID][$actorId] = $value; + $userId = $actor->getId( $actor->getWikiId() ); + if ( $userId ) { + $this->cache[self::KEY_USER_ID][$userId] = $value; + } + $this->cache[self::KEY_USER_NAME][$actor->getName()] = $value; + } + + /** + * Remove $actor from cache. + * @param UserIdentity $actor + */ + public function remove( UserIdentity $actor ) { + $oldByName = $this->cache[self::KEY_USER_NAME][$actor->getName()] ?? null; + $oldByUserId = $this->cache[self::KEY_USER_ID][$actor->getId( $actor->getWikiId() )] ?? null; + if ( $oldByName ) { + unset( $this->cache[self::KEY_USER_ID][$oldByName['actor']->getId( $oldByName['actor']->getWikiId() )] ); + unset( $this->cache[self::KEY_ACTOR_ID][$oldByName['actorId']] ); + } + if ( $oldByUserId ) { + unset( $this->cache[self::KEY_USER_NAME][$oldByUserId['actor']->getName()] ); + unset( $this->cache[self::KEY_ACTOR_ID][$oldByUserId['actorId']] ); + } + unset( $this->cache[self::KEY_USER_NAME][$actor->getName()] ); + unset( $this->cache[self::KEY_USER_ID][$actor->getId( $actor->getWikiId() )] ); + } + + /** + * @param string $keyType one of self::KEY_* constants. + * @param string|int $keyValue + * @return array|null [ 'actor' => UserIdentity, 'actorId' => int ] + */ + private function getCachedValue( string $keyType, $keyValue ): ?array { + if ( isset( $this->cache[$keyType][$keyValue] ) ) { + $cached = $this->cache[$keyType][$keyValue]; + $this->ping( $cached['actorId'] ); + return $cached; + } + return null; + } + + /** + * Record the actor with $actorId was recently used. + * @param int $actorId + */ + private function ping( int $actorId ) { + $item = $this->cache[self::KEY_ACTOR_ID][$actorId]; + unset( $this->cache[self::KEY_ACTOR_ID][$actorId] ); + $this->cache[self::KEY_ACTOR_ID][$actorId] = $item; + } +} diff --git a/includes/user/ActorStore.php b/includes/user/ActorStore.php index 5bc02f936cd..43145c0c3c2 100644 --- a/includes/user/ActorStore.php +++ b/includes/user/ActorStore.php @@ -24,7 +24,6 @@ use CannotCreateActorException; use DBAccessObjectUtils; use ExternalUserNames; use InvalidArgumentException; -use MapCacheLRU; use MediaWiki\DAO\WikiAwareEntity; use Psr\Log\LoggerInterface; use stdClass; @@ -59,14 +58,8 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { /** @var string|false */ private $wikiId; - /** @var MapCacheLRU int actor ID => [ UserIdentity, int actor ID ] */ - private $actorsByActorId; - - /** @var MapCacheLRU int user ID => [ UserIdentity, int actor ID ] */ - private $actorsByUserId; - - /** @var MapCacheLRU string user name => [ UserIdentity, int actor ID ] */ - private $actorsByName; + /** @var ActorCache */ + private $cache; /** * @param ILoadBalancer $loadBalancer @@ -88,9 +81,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { $this->logger = $logger; $this->wikiId = $wikiId; - $this->actorsByActorId = new MapCacheLRU( self::LOCAL_CACHE_SIZE ); - $this->actorsByUserId = new MapCacheLRU( self::LOCAL_CACHE_SIZE ); - $this->actorsByName = new MapCacheLRU( self::LOCAL_CACHE_SIZE ); + $this->cache = new ActorCache( self::LOCAL_CACHE_SIZE ); } /** @@ -130,7 +121,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { } $actor = new UserIdentityValue( $userId, $row->actor_name, $this->wikiId ); - $this->addUserIdentityToCache( $actorId, $actor ); + $this->cache->add( $actorId, $actor ); return $actor; } @@ -186,34 +177,16 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { $this->wikiId ); - $this->addUserIdentityToCache( $actorId, $actor ); + $this->cache->add( $actorId, $actor ); return $actor; } /** - * @param int $actorId * @param UserIdentity $actor + * @internal for use in User object only */ - private function addUserIdentityToCache( int $actorId, UserIdentity $actor ) { - $this->actorsByActorId->set( $actorId, [ $actor, $actorId ] ); - $userId = $actor->getId( $this->wikiId ); - if ( $userId ) { - $this->actorsByUserId->set( $userId, [ $actor, $actorId ] ); - } - $this->actorsByName->set( $actor->getName(), [ $actor, $actorId ] ); - } - - /** - * @param int $actorId - * @param UserIdentity $actor - */ - private function deleteUserIdentityFromCache( int $actorId, UserIdentity $actor ) { - $this->actorsByActorId->clear( $actorId ); - $userId = $actor->getId( $this->wikiId ); - if ( $userId ) { - $this->actorsByUserId->clear( $userId ); - } - $this->actorsByName->clear( $actor->getName() ); + public function deleteUserIdentityFromCache( UserIdentity $actor ) { + $this->cache->remove( $actor ); } /** @@ -231,26 +204,18 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { return null; } - $cachedValue = $this->actorsByActorId->get( $actorId ); - if ( $cachedValue ) { - return $cachedValue[0]; - } - - $actor = $this->newSelectQueryBuilder( $db ) - ->caller( __METHOD__ ) - ->conds( [ 'actor_id' => $actorId ] ) - ->fetchUserIdentity(); - - // The actor ID mostly comes from DB, so if we can't find an actor by ID, - // it's most likely due to lagged replica and not cause it doesn't actually exist. - // Probably we just inserted it? Try master. - if ( !$actor ) { - $actor = $this->newSelectQueryBuilderForQueryFlags( self::READ_LATEST ) + return $this->cache->getActor( ActorCache::KEY_ACTOR_ID, $actorId ) ?? + $this->newSelectQueryBuilder( $db ) + ->caller( __METHOD__ ) + ->conds( [ 'actor_id' => $actorId ] ) + ->fetchUserIdentity() ?? + // The actor ID mostly comes from DB, so if we can't find an actor by ID, + // it's most likely due to lagged replica and not cause it doesn't actually exist. + // Probably we just inserted it? Try master. + $this->newSelectQueryBuilderForQueryFlags( self::READ_LATEST ) ->caller( __METHOD__ ) ->conds( [ 'actor_id' => $actorId ] ) ->fetchUserIdentity(); - } - return $actor; } /** @@ -273,15 +238,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { ); } - $cachedValue = $this->actorsByName->get( $normalizedName ); - if ( $cachedValue ) { - return $cachedValue[0]; - } - - return $this->newSelectQueryBuilderForQueryFlags( $queryFlags ) - ->caller( __METHOD__ ) - ->userNames( $normalizedName ) - ->fetchUserIdentity(); + return $this->cache->getActor( ActorCache::KEY_USER_NAME, $normalizedName ) ?? + $this->newSelectQueryBuilderForQueryFlags( $queryFlags ) + ->caller( __METHOD__ ) + ->userNames( $normalizedName ) + ->fetchUserIdentity(); } /** @@ -296,15 +257,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { return null; } - $cachedValue = $this->actorsByUserId->get( $userId ); - if ( $cachedValue ) { - return $cachedValue[0]; - } - - return $this->newSelectQueryBuilderForQueryFlags( $queryFlags ) - ->caller( __METHOD__ ) - ->userIds( $userId ) - ->fetchUserIdentity(); + return $this->cache->getActor( ActorCache::KEY_USER_ID, $userId ) ?? + $this->newSelectQueryBuilderForQueryFlags( $queryFlags ) + ->caller( __METHOD__ ) + ->userIds( $userId ) + ->fetchUserIdentity(); } /** @@ -388,9 +345,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { return null; } - $id = $this->findActorIdInternal( $name, $db ); - - return $id; + return $this->findActorIdInternal( $name, $db ); } /** @@ -412,9 +367,9 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { // User always thinks it's local, so we could end up fetching the ID // from the wrong database. - $cachedValue = $this->actorsByName->get( $name ); + $cachedValue = $this->cache->getActorId( ActorCache::KEY_USER_NAME, $name ); if ( $cachedValue ) { - return $cachedValue[1]; + return $cachedValue; } $row = $db->selectRow( @@ -504,7 +459,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { $this->attachActorId( $user, $actorId, true ); // Cache row we've just created $cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId ); - $this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user ); + $this->setUpRollbackHandler( $dbw, $cachedUserIdentity, $user ); return $actorId; } @@ -544,7 +499,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { $this->attachActorId( $user, $actorId, true ); // Cache row we've just created $cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId ); - $this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user ); + $this->setUpRollbackHandler( $dbw, $cachedUserIdentity, $user ); return $actorId; } @@ -573,7 +528,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { $existingActorId = $this->findActorIdInternal( $userName, $dbw ); if ( $existingActorId ) { // It certainly will be cached if we just found it. - $existingActor = $this->actorsByActorId->get( $existingActorId )[0]; + $existingActor = $this->cache->getActor( ActorCache::KEY_ACTOR_ID, $existingActorId ); // If we already have an existing actor with a matching user ID // just return it, nothing to do here. @@ -605,11 +560,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { } $actorId = $dbw->insertId() ?: $existingActorId; - $this->deleteUserIdentityFromCache( $actorId, $user ); + $this->cache->remove( $user ); $this->attachActorId( $user, $actorId, true ); // Cache row we've just created $cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId ); - $this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user ); + $this->setUpRollbackHandler( $dbw, $cachedUserIdentity, $user ); return $actorId; } @@ -674,13 +629,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { * Clear in-process caches if transaction gets rolled back. * * @param IDatabase $dbw - * @param int $actorId * @param UserIdentity $cachedActor * @param UserIdentity $originalActor */ private function setUpRollbackHandler( IDatabase $dbw, - int $actorId, UserIdentity $cachedActor, UserIdentity $originalActor ) { @@ -688,9 +641,9 @@ class ActorStore implements UserIdentityLookup, ActorNormalization { // If called within a transaction and it was rolled back, the cached actor ID // becomes invalid, so cache needs to be invalidated as well. See T277795. $dbw->onTransactionResolution( - function ( int $trigger ) use ( $actorId, $cachedActor, $originalActor ) { + function ( int $trigger ) use ( $cachedActor, $originalActor ) { if ( $trigger === IDatabase::TRIGGER_ROLLBACK ) { - $this->deleteUserIdentityFromCache( $actorId, $cachedActor ); + $this->cache->remove( $cachedActor ); $this->detachActorId( $originalActor ); } } ); diff --git a/includes/user/User.php b/includes/user/User.php index 08b9760d89b..23bcb09fca1 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -3426,6 +3426,7 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact [ 'actor_user' => $this->mId ], $fname ); + MediaWikiServices::getInstance()->getActorStore()->deleteUserIdentityFromCache( $this ); } ); $this->mTouched = $newTouched; diff --git a/tests/phpunit/integration/includes/user/ActorStoreTest.php b/tests/phpunit/integration/includes/user/ActorStoreTest.php index 79bd4d040f3..c0524353b36 100644 --- a/tests/phpunit/integration/includes/user/ActorStoreTest.php +++ b/tests/phpunit/integration/includes/user/ActorStoreTest.php @@ -750,6 +750,23 @@ class ActorStoreTest extends ActorStoreTestBase { $this->assertSameActors( $anotherActor, $store->getActorById( $anotherActorId, $this->db ) ); } + public function testUserRenameUpdatesActor() { + $user = $this->getMutableTestUser()->getUser(); + $oldName = $user->getName(); + + $store = $this->getStore(); + $actorId = $store->findActorId( $user, $this->db ); + $this->assertTrue( $actorId > 0 ); + + $user->setName( 'NewName' ); + $user->saveSettings(); + + $this->assertSameActors( $user, $store->getActorById( $actorId, $this->db ) ); + $this->assertSameActors( $user, $store->getUserIdentityByName( 'NewName' ) ); + $this->assertSameActors( $user, $store->getUserIdentityByUserId( $user->getId() ) ); + $this->assertNull( $store->getUserIdentityByName( $oldName ) ); + } + /** * @covers ::acquireSystemActorId */ diff --git a/tests/phpunit/unit/includes/user/ActorCacheTest.php b/tests/phpunit/unit/includes/user/ActorCacheTest.php new file mode 100644 index 00000000000..a165de52e96 --- /dev/null +++ b/tests/phpunit/unit/includes/user/ActorCacheTest.php @@ -0,0 +1,116 @@ +assertSame( $actor, + $cache->getActor( ActorCache::KEY_ACTOR_ID, $actorId ) ); + $this->assertSame( $actor, + $cache->getActor( ActorCache::KEY_USER_NAME, $actor->getName() ) ); + $this->assertSame( $actor, + $cache->getActor( ActorCache::KEY_USER_ID, $actor->getId( $actor->getWikiId() ) ) ); + $this->assertSame( $actorId, + $cache->getActorId( ActorCache::KEY_ACTOR_ID, 1 ) ); + $this->assertSame( $actorId, + $cache->getActorId( ActorCache::KEY_USER_NAME, $actor->getName() ) ); + $this->assertSame( $actorId, + $cache->getActorId( ActorCache::KEY_USER_ID, $actor->getId( $actor->getWikiId() ) ) ); + } + + /** + * @param ActorCache $cache + * @param int $actorId + * @param string $actorName + * @param int $userId + */ + private function assertCacheNotContains( ActorCache $cache, int $actorId, string $actorName, int $userId ) { + $this->assertNull( $cache->getActor( ActorCache::KEY_ACTOR_ID, $actorId ) ); + $this->assertNull( $cache->getActor( ActorCache::KEY_USER_NAME, $actorName ) ); + $this->assertNull( $cache->getActor( ActorCache::KEY_USER_ID, $userId ) ); + $this->assertNull( $cache->getActorId( ActorCache::KEY_ACTOR_ID, $actorId ) ); + $this->assertNull( $cache->getActorId( ActorCache::KEY_USER_NAME, $actorName ) ); + $this->assertNull( $cache->getActorId( ActorCache::KEY_USER_ID, $userId ) ); + } + + public function provideGetActor() { + yield 'local' => [ + 'actor' => new UserIdentityValue( 10, 'Hello' ), + ]; + yield 'foreign' => [ + 'actor' => new UserIdentityValue( 10, 'Hello', 'acmewiki' ), + ]; + } + + /** + * @dataProvider provideGetActor + */ + public function testGetActor( UserIdentity $actor ) { + $cache = new ActorCache( 5 ); + $cache->add( 1, $actor ); + $this->assertCacheContains( $cache, 1, $actor ); + } + + public function provideRemove() { + yield 'Same actor' => [ + 'addedActor' => new UserIdentityValue( 10, 'Hello' ), + 'removedActor' => new UserIdentityValue( 10, 'Hello' ) + ]; + yield 'Different name' => [ + 'addedActor' => new UserIdentityValue( 10, 'Hello' ), + 'removedActor' => new UserIdentityValue( 10, 'Goodbye' ) + ]; + yield 'Different user ID' => [ + 'addedActor' => new UserIdentityValue( 10, 'Hello' ), + 'removedActor' => new UserIdentityValue( 11, 'Hello' ) + ]; + } + + /** + * @dataProvider provideRemove + */ + public function testRemove( UserIdentity $added, UserIdentity $removed ) { + $cache = new ActorCache( 5 ); + $cache->add( 1, $added ); + $this->assertCacheContains( $cache, 1, $added ); + $cache->remove( $removed ); + $this->assertCacheNotContains( $cache, 1, $added->getName(), $added->getId() ); + $this->assertCacheNotContains( $cache, 1, $removed->getName(), $removed->getId() ); + } + + public function testEvict() { + $cache = new ActorCache( 1 ); + $actor1 = new UserIdentityValue( 10, 'Hello' ); + $cache->add( 2, new UserIdentityValue( 12, 'Goodbye' ) ); + $cache->add( 1, $actor1 ); + $this->assertCacheContains( $cache, 1, $actor1 ); + $this->assertCacheNotContains( $cache, 2, 'Goodbye', 12 ); + } + + public function testDoesNotCacheAnonUserId() { + $cache = new ActorCache( 5 ); + $actor = new UserIdentityValue( 0, '127.0.0.1' ); + $cache->add( 1, $actor ); + $this->assertNull( $cache->getActor( ActorCache::KEY_USER_ID, 0 ) ); + $this->assertNull( $cache->getActorId( ActorCache::KEY_USER_ID, 0 ) ); + $this->assertSame( $actor, $cache->getActor( ActorCache::KEY_USER_NAME, '127.0.0.1' ) ); + $this->assertSame( 1, $cache->getActorId( ActorCache::KEY_USER_NAME, '127.0.0.1' ) ); + $this->assertSame( $actor, $cache->getActor( ActorCache::KEY_ACTOR_ID, 1 ) ); + $this->assertSame( 1, $cache->getActorId( ActorCache::KEY_ACTOR_ID, 1 ) ); + } +}