From b406c52adb0440fdf92ee98c13e276606304938b Mon Sep 17 00:00:00 2001 From: Martin Urbanec Date: Mon, 1 Nov 2021 14:11:33 +0100 Subject: [PATCH] LinkCache: Try invalidating cache before throwing The code that was previously in LinkCache::getGoodLinkRow may return a cached row object (either from the in-process cache, or memcached). In some cases, this caused LinkCache::addGoodLinkObjFromRow to throw, because a field was missing (although the field is missing only from the cached copy, not from the database). To avoid this, try to invalidate the cache in LinkCache::getGoodLinkRow and retry, before letting the exception propagate. Bug: T205349 Change-Id: Ie9e90bf32964047c1831f575cc260d7d62e9e848 --- includes/cache/LinkCache.php | 87 ++++++++++++++----- .../phpunit/includes/cache/LinkCacheTest.php | 24 +++++ 2 files changed, 89 insertions(+), 22 deletions(-) diff --git a/includes/cache/LinkCache.php b/includes/cache/LinkCache.php index 4275067ba02..32ef5611df2 100644 --- a/includes/cache/LinkCache.php +++ b/includes/cache/LinkCache.php @@ -425,49 +425,46 @@ class LinkCache implements LoggerAwareInterface { } /** - * Returns the row for the page if the page exists (subject to race conditions). - * The row will be returned from local cache or WAN cache if possible, or it - * will be looked up using the callback provided. - * - * @param int $ns - * @param string $dbkey - * @param callable|null $fetchCallback A callback that will retrieve the link row with the - * signature ( IDatabase $db, int $ns, string $dbkey, array $queryOptions ): ?stdObj. - * @param int $queryFlags IDBAccessObject::READ_XXX - * - * @return stdClass|null - * @internal for use by PageStore. Other code should use a PageLookup instead. + * @param TitleValue|null $link + * @param callable|null $fetchCallback + * @param int $queryFlags + * @return array [ $shouldAddGoodLink, $row ], $shouldAddGoodLink is a bool indicating + * whether addGoodLinkObjFromRow should be called, and $row is the row the caller was looking + * for (or false, when it was not found). */ - public function getGoodLinkRow( - int $ns, - string $dbkey, + private function getGoodLinkRowInternal( + ?TitleValue $link, callable $fetchCallback = null, int $queryFlags = IDBAccessObject::READ_NORMAL - ): ?stdClass { - $link = TitleValue::tryNew( $ns, $dbkey ); + ): array { $key = $link ? $this->getCacheKey( $link ) : null; if ( $key === null ) { - return null; + return [ false, false ]; } + $ns = $link->getNamespace(); + $dbkey = $link->getDBkey(); + $callerShouldAddGoodLink = false; + if ( $this->mForUpdate ) { $queryFlags |= IDBAccessObject::READ_LATEST; } $forUpdate = $queryFlags & IDBAccessObject::READ_LATEST; if ( !$forUpdate && $this->isBadLink( $key ) ) { - return null; + return [ $callerShouldAddGoodLink, false ]; } [ $row, $rowFlags ] = $this->goodLinks->get( $key ); if ( $row && $rowFlags >= $queryFlags ) { - return $row; + return [ $callerShouldAddGoodLink, $row ]; } if ( !$fetchCallback ) { - return null; + return [ $callerShouldAddGoodLink, false ]; } + $callerShouldAddGoodLink = true; if ( $this->usePersistentCache( $ns ) && !$forUpdate ) { // Some pages are often transcluded heavily, so use persistent caching $wanCacheKey = $this->wanCache->makeKey( 'page', $ns, sha1( $dbkey ) ); @@ -493,8 +490,54 @@ class LinkCache implements LoggerAwareInterface { $row = $fetchCallback( $dbr, $ns, $dbkey, $options ); } + return [ $callerShouldAddGoodLink, $row ]; + } + + /** + * Returns the row for the page if the page exists (subject to race conditions). + * The row will be returned from local cache or WAN cache if possible, or it + * will be looked up using the callback provided. + * + * @param int $ns + * @param string $dbkey + * @param callable|null $fetchCallback A callback that will retrieve the link row with the + * signature ( IDatabase $db, int $ns, string $dbkey, array $queryOptions ): ?stdObj. + * @param int $queryFlags IDBAccessObject::READ_XXX + * + * @return stdClass|null + * @internal for use by PageStore. Other code should use a PageLookup instead. + */ + public function getGoodLinkRow( + int $ns, + string $dbkey, + callable $fetchCallback = null, + int $queryFlags = IDBAccessObject::READ_NORMAL + ): ?stdClass { + $link = TitleValue::tryNew( $ns, $dbkey ); + if ( $link === null ) { + return null; + } + [ $shouldAddGoodLink, $row ] = $this->getGoodLinkRowInternal( + $link, + $fetchCallback, + $queryFlags + ); + if ( $row ) { - $this->addGoodLinkObjFromRow( $link, $row ); + if ( $shouldAddGoodLink ) { + try { + $this->addGoodLinkObjFromRow( $link, $row, $queryFlags ); + } catch ( InvalidArgumentException $e ) { + // a field is missing from $row; maybe we used a cache?; invalidate it and try again + $this->invalidateTitle( $link ); + [ $shouldAddGoodLink, $row ] = $this->getGoodLinkRowInternal( + $link, + $fetchCallback, + $queryFlags + ); + $this->addGoodLinkObjFromRow( $link, $row, $queryFlags ); + } + } } else { $this->addBadLinkObj( $link ); } diff --git a/tests/phpunit/includes/cache/LinkCacheTest.php b/tests/phpunit/includes/cache/LinkCacheTest.php index 019134c73ef..ec2994db135 100644 --- a/tests/phpunit/includes/cache/LinkCacheTest.php +++ b/tests/phpunit/includes/cache/LinkCacheTest.php @@ -492,6 +492,30 @@ class LinkCacheTest extends MediaWikiIntegrationTestCase { ); } + /** + * @covers LinkCache::getGoodLinkRow() + */ + public function testGetGoodLinkRowGetsIncompleteCachedInfo() { + // Pages in some namespaces use the WAN cache: Template, File, Category, MediaWiki + $existing = new TitleValue( NS_TEMPLATE, 'Existing' ); + $brokenRow = $this->getPageRow( 3 ); + unset( $brokenRow->page_len ); // make incomplete row + + $cache = new HashBagOStuff(); + $wanCache = new WANObjectCache( [ 'cache' => $cache ] ); + $linkCache = $this->newLinkCache( $wanCache ); + + // force the incomplete row into the cache + $keys = $linkCache->getMutableCacheKeys( $wanCache, $existing ); + $wanCache->set( $keys[0], $brokenRow ); + + // check that we are not getting the broken row, but load a good row + $callback = [ $this, 'getRowIfExisting' ]; + $row = $linkCache->getGoodLinkRow( $existing->getNamespace(), $existing->getDBkey(), $callback ); + + $this->assertNotEquals( $brokenRow, $row ); + } + /** * @covers LinkCache::getGoodLinkRow() * @covers LinkCache::getMutableCacheKeys()