From c962b480568ea13634a682c66f49c266148c3806 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 27 Oct 2016 22:53:51 -0700 Subject: [PATCH] Avoid races in MessageCache::replace() Do the process cache update immediately (as before) but push the shared cache updates to a deferred update. This update will thus start with a clear transaction snapshot, so it can acquire the lock before the first SELECT as is proper. Also added some missing method visibilities. Bug: T144952 Change-Id: I462554b300d4688b09ab80cd1bb8a4340ffaa786 --- includes/cache/MessageCache.php | 121 ++++++++++-------- .../includes/cache/MessageCacheTest.php | 31 +++++ 2 files changed, 98 insertions(+), 54 deletions(-) diff --git a/includes/cache/MessageCache.php b/includes/cache/MessageCache.php index 352a94c5fb..d40156d8c7 100644 --- a/includes/cache/MessageCache.php +++ b/includes/cache/MessageCache.php @@ -444,7 +444,7 @@ class MessageCache { * @param integer $mode Use MessageCache::FOR_UPDATE to skip process cache * @return array Loaded messages for storing in caches */ - function loadFromDB( $code, $mode = null ) { + protected function loadFromDB( $code, $mode = null ) { global $wgMaxMsgCacheEntrySize, $wgLanguageCode, $wgAdaptiveMessageCache; $dbr = wfGetDB( ( $mode == self::FOR_UPDATE ) ? DB_MASTER : DB_REPLICA ); @@ -518,7 +518,7 @@ class MessageCache { wfDebugLog( 'MessageCache', __METHOD__ - . ": failed to load message page text for {$row->page_title} ($code)" + . ": failed to load message page text for {$row->page_title} ($code)" ); } else { $entry = ' ' . $text; @@ -541,11 +541,11 @@ class MessageCache { /** * Updates cache as necessary when message page is changed * - * @param string|bool $title Name of the page changed (false if deleted) + * @param string $title Message cache key with initial uppercase letter. * @param string|bool $text New contents of the page (false if deleted) */ public function replace( $title, $text ) { - global $wgMaxMsgCacheEntrySize, $wgContLang, $wgLanguageCode; + global $wgLanguageCode; if ( $this->mDisable ) { return; @@ -557,63 +557,75 @@ class MessageCache { return; } - // Note that if the cache is volatile, load() may trigger a DB fetch. - // In that case we reenter/reuse the existing cache key lock to avoid - // a self-deadlock. This is safe as no reads happen *directly* in this - // method between getReentrantScopedLock() and load() below. There is - // no risk of data "changing under our feet" for replace(). - $scopedLock = $this->getReentrantScopedLock( wfMemcKey( 'messages', $code ) ); - // Load the messages from the master DB to avoid race conditions - $this->load( $code, self::FOR_UPDATE ); - - // Load the new value into the process cache... + // (a) Update the process cache with the new message text if ( $text === false ) { + // Page deleted $this->mCache[$code][$title] = '!NONEXISTENT'; - } elseif ( strlen( $text ) > $wgMaxMsgCacheEntrySize ) { - $this->mCache[$code][$title] = '!TOO BIG'; - // Pre-fill the individual key cache with the known latest message text - $key = $this->wanCache->makeKey( 'messages-big', $this->mCache[$code]['HASH'], $title ); - $this->wanCache->set( $key, " $text", $this->mExpiry ); } else { + // Ignore $wgMaxMsgCacheEntrySize so the process cache is up to date $this->mCache[$code][$title] = ' ' . $text; } - // Mark this cache as definitely being "latest" (non-volatile) so - // load() calls do not try to refresh the cache with replica DB data - $this->mCache[$code]['LATEST'] = time(); - - // Update caches if the lock was acquired - if ( $scopedLock ) { - $this->saveToCaches( $this->mCache[$code], 'all', $code ); - } else { - LoggerFactory::getInstance( 'MessageCache' )->error( - __METHOD__ . ': could not acquire lock to update {title} ({code})', - [ 'title' => $title, 'code' => $code ] ); - } - - ScopedCallback::consume( $scopedLock ); - // Relay the purge. Touching this check key expires cache contents - // and local cache (APC) validation hash across all datacenters. - $this->wanCache->touchCheckKey( wfMemcKey( 'messages', $code ) ); - - // Also delete cached sidebar... just in case it is affected - $codes = [ $code ]; - if ( $code === 'en' ) { - // Delete all sidebars, like for example on action=purge on the - // sidebar messages - $codes = array_keys( Language::fetchLanguageNames() ); - } - foreach ( $codes as $code ) { - $sidebarKey = wfMemcKey( 'sidebar', $code ); - $this->wanCache->delete( $sidebarKey ); - } + // (b) Update the shared caches in a deferred update with a fresh DB snapshot + DeferredUpdates::addCallableUpdate( + function () use ( $title, $msg, $code ) { + global $wgContLang, $wgMaxMsgCacheEntrySize; + // Allow one caller at a time to avoid race conditions + $scopedLock = $this->getReentrantScopedLock( wfMemcKey( 'messages', $code ) ); + if ( !$scopedLock ) { + LoggerFactory::getInstance( 'MessageCache' )->error( + __METHOD__ . ': could not acquire lock to update {title} ({code})', + [ 'title' => $title, 'code' => $code ] ); + return; + } + // Load the messages from the master DB to avoid race conditions + $this->loadFromDB( $code, self::FOR_UPDATE ); + // Load the process cache values and set the per-title cache keys + $page = WikiPage::factory( Title::makeTitle( NS_MEDIAWIKI, $title ) ); + $page->loadPageData( $page::READ_LATEST ); + $text = $this->getMessageTextFromContent( $page->getContent() ); + // Check if an individual cache key should exist and update cache accordingly + $titleKey = $this->wanCache->makeKey( + 'messages-big', $this->mCache[$code]['HASH'], $title ); + if ( is_string( $text ) && strlen( $text ) > $wgMaxMsgCacheEntrySize ) { + $this->wanCache->set( $titleKey, ' ' . $text, $this->mExpiry ); + } + // Mark this cache as definitely being "latest" (non-volatile) so + // load() calls do try to refresh the cache with replica DB data + $this->mCache[$code]['LATEST'] = time(); + // Pre-emptively update the local datacenter cache so things like edit filter and + // blacklist changes are reflect immediately, as these often use MediaWiki: pages. + // The datacenter handling replace() calls should be the same one handling edits + // as they require HTTP POST. + $this->saveToCaches( $this->mCache[$code], 'all', $code ); + // Release the lock now that the cache is saved + ScopedCallback::consume( $scopedLock ); + + // Relay the purge. Touching this check key expires cache contents + // and local cache (APC) validation hash across all datacenters. + $this->wanCache->touchCheckKey( wfMemcKey( 'messages', $code ) ); + // Also delete cached sidebar... just in case it is affected + // @TODO: shouldn't this be $code === $wgLanguageCode? + if ( $code === 'en' ) { + // Purge all language sidebars, e.g. on ?action=purge to the sidebar messages + $codes = array_keys( Language::fetchLanguageNames() ); + } else { + // Purge only the sidebar for this language + $codes = [ $code ]; + } + foreach ( $codes as $code ) { + $this->wanCache->delete( wfMemcKey( 'sidebar', $code ) ); + } - // Update the message in the message blob store - $resourceloader = RequestContext::getMain()->getOutput()->getResourceLoader(); - $blobStore = $resourceloader->getMessageBlobStore(); - $blobStore->updateMessage( $wgContLang->lcfirst( $msg ) ); + // Purge the message in the message blob store + $resourceloader = RequestContext::getMain()->getOutput()->getResourceLoader(); + $blobStore = $resourceloader->getMessageBlobStore(); + $blobStore->updateMessage( $wgContLang->lcfirst( $msg ) ); - Hooks::run( 'MessageCacheReplace', [ $title, $text ] ); + Hooks::run( 'MessageCacheReplace', [ $title, $text ] ); + }, + DeferredUpdates::PRESEND + ); } /** @@ -845,7 +857,7 @@ class MessageCache { $alreadyTried = []; - // First try the requested language. + // First try the requested language. $message = $this->getMessageForLang( $lang, $lckey, $useDB, $alreadyTried ); if ( $message !== false ) { return $message; @@ -950,6 +962,7 @@ class MessageCache { */ public function getMsgFromNamespace( $title, $code ) { $this->load( $code ); + if ( isset( $this->mCache[$code][$title] ) ) { $entry = $this->mCache[$code][$title]; if ( substr( $entry, 0, 1 ) === ' ' ) { diff --git a/tests/phpunit/includes/cache/MessageCacheTest.php b/tests/phpunit/includes/cache/MessageCacheTest.php index bd744c01ca..a12c8b2d09 100644 --- a/tests/phpunit/includes/cache/MessageCacheTest.php +++ b/tests/phpunit/includes/cache/MessageCacheTest.php @@ -99,6 +99,37 @@ class MessageCacheTest extends MediaWikiLangTestCase { ]; } + public function testReplaceMsg() { + global $wgContLang; + + $messageCache = MessageCache::singleton(); + $message = 'go'; + $uckey = $wgContLang->ucfirst( $message ); + $oldText = $messageCache->get( $message ); // "Ausführen" + + $dbw = wfGetDB( DB_MASTER ); + $dbw->startAtomic( __METHOD__ ); // simulate request and block deferred updates + $messageCache->replace( $uckey, 'Allez!' ); + $this->assertEquals( 'Allez!', + $messageCache->getMsgFromNamespace( $uckey, 'de' ), + 'Updates are reflected in-process immediately' ); + $this->assertEquals( 'Allez!', + $messageCache->get( $message ), + 'Updates are reflected in-process immediately' ); + $this->makePage( 'Go', 'de', 'Race!' ); + $dbw->endAtomic( __METHOD__ ); + + $this->assertEquals( 0, + DeferredUpdates::pendingUpdatesCount(), + 'Post-commit deferred update triggers a run of all updates' ); + + $this->assertEquals( 'Race!', $messageCache->get( $message ), 'Correct final contents' ); + + $this->makePage( 'Go', 'de', $oldText ); + $messageCache->replace( $uckey, $oldText ); // deferred update runs immediately + $this->assertEquals( $oldText, $messageCache->get( $message ), 'Content restored' ); + } + /** * There's a fallback case where the message key is given as fully qualified -- this * should ignore the passed $lang and use the language from the key -- 2.20.1