Aaron Schulz has uploaded a new change for review. https://gerrit.wikimedia.org/r/318489
Change subject: [WIP] Avoid races in MessageCache::replace() ...................................................................... [WIP] 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. Change-Id: I462554b300d4688b09ab80cd1bb8a4340ffaa786 --- M includes/cache/MessageCache.php 1 file changed, 108 insertions(+), 85 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/89/318489/1 diff --git a/includes/cache/MessageCache.php b/includes/cache/MessageCache.php index 0af8511..ece2479 100644 --- a/includes/cache/MessageCache.php +++ b/includes/cache/MessageCache.php @@ -447,7 +447,7 @@ * @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 ); @@ -478,7 +478,8 @@ } else { # Effectively disallows use of '/' character in NS_MEDIAWIKI for uses # other than language code. - $conds[] = 'page_title NOT' . $dbr->buildLike( $dbr->anyString(), '/', $dbr->anyString() ); + $conds[] = 'page_title NOT' . + $dbr->buildLike( $dbr->anyString(), '/', $dbr->anyString() ); } # Conditions to fetch oversized pages to ignore them @@ -536,7 +537,7 @@ * @param mixed $text New contents of the page. */ public function replace( $title, $text ) { - global $wgMaxMsgCacheEntrySize, $wgContLang, $wgLanguageCode; + global $wgLanguageCode; if ( $this->mDisable ) { return; @@ -548,66 +549,77 @@ 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 process cache values and set the per-title cache keys. - $titleKey = $this->wanCache->makeKey( 'messages', 'individual', $title ); + // (a) Update the process cache with the new message text if ( $text === false ) { - // Article was deleted + // Page deleted $this->mCache[$code][$title] = '!NONEXISTENT'; - $this->wanCache->delete( $titleKey ); - } elseif ( strlen( $text ) > $wgMaxMsgCacheEntrySize ) { - $this->mCache[$code][$title] = '!TOO BIG'; - $scopedLock - ? $this->wanCache->set( $titleKey, " $text", $this->mExpiry ) - // Avoid the risk of set() collisions on the same title key - : $this->wanCache->delete( $titleKey ); } else { + // Ignore $wgMaxMsgCacheEntrySize so the process cache is up to date $this->mCache[$code][$title] = ' ' . $text; - $this->wanCache->delete( $titleKey ); } - // 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(); - // Update caches if the lock was acquired - if ( $scopedLock ) { + // (b) Update the shared caches in a deferred update with a fresh DB snapshot + DeferredUpdates::addCallableUpdate( function () use ( $title, $msg, $code ) { + global $wgContLang, $wgMaxMsgCacheEntrySize; + // 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 ) ); + if ( !$scopedLock ) { + wfDebugLog( 'MessageCache', __METHOD__ . + ": could not acquire lock to update {$title} ($code)" ); + return; + } + // Load the messages from the master DB to avoid race conditions + $this->load( $code, self::FOR_UPDATE ); + // Load the process cache values and set the per-title cache keys + if ( isset( $this->mCache[$code][$title] ) ) { + $page = WikiPage::factory( Title::makeTitle( NS_MEDIAWIKI, $title ) ); + $page->loadPageData( $page::READ_LATEST ); + $text = $this->getMessageTextFromContent( $page->getContent() ); + } else { + $text = false; + } + // Check if an individual cache key should exist and update cache accordingly + $titleKey = $this->wanCache->makeKey( 'messages', 'individual', $title ); + if ( is_string( $text ) && strlen( $text ) > $wgMaxMsgCacheEntrySize ) { + $this->wanCache->set( $titleKey, ' ' . $text, $this->mExpiry ); + } else { + $this->wanCache->delete( $titleKey ); + } + // 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 handles edits. $this->saveToCaches( $this->mCache[$code], 'all', $code ); - } else { - wfDebugLog( 'MessageCache', __METHOD__ . - ": could not acquire lock to update {$title} ($code)" ); - } + // Release the lock now that the cache is saved + ScopedCallback::consume( $scopedLock ); - ScopedCallback::consume( $scopedLock ); - // Relay the purge to APC and other DCs - $this->wanCache->touchCheckKey( wfMemcKey( 'messages', $code ) ); + // Relay the purge to APC and other 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 ) ); + } + // Purge the message in the message blob store + $resourceloader = RequestContext::getMain()->getOutput()->getResourceLoader(); + $blobStore = $resourceloader->getMessageBlobStore(); + $blobStore->updateMessage( $wgContLang->lcfirst( $msg ) ); - // 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 ); - } - - // Update 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 ] ); + } ); } /** @@ -751,7 +763,7 @@ * @return string|bool False if the message doesn't exist, otherwise the * message (which can be empty) */ - function get( $key, $useDB = true, $langcode = true, $isFullKey = false ) { + public function get( $key, $useDB = true, $langcode = true, $isFullKey = false ) { if ( is_int( $key ) ) { // Fix numerical strings that somehow become ints // on their way here @@ -940,7 +952,7 @@ * * @param string $title Message cache key with initial uppercase letter. * @param string $code Code denoting the language to try. - * @return string|bool The message, or false if it does not exist or on error + * @return string|bool|null The message, or false if it does not exist, or null on error */ public function getMsgFromNamespace( $title, $code ) { $this->load( $code ); @@ -1009,20 +1021,8 @@ if ( $revision ) { $content = $revision->getContent(); if ( $content ) { - // XXX: Is this the right way to turn a Content object into a message? - // NOTE: $content is typically either WikitextContent, JavaScriptContent or - // CssContent. MessageContent is *not* used for storing messages, it's - // only used for wrapping them when needed. - $message = $content->getWikitextForTransclusion(); - if ( $message === false || $message === null ) { - wfDebugLog( - 'MessageCache', - __METHOD__ . ": message content doesn't provide wikitext " - . "(content model: " . $content->getModel() . ")" - ); - - $message = false; // negative caching - } else { + $message = $this->getMessageTextFromContent( $content ); + if ( is_string( $message ) ) { $this->mCache[$code][$title] = ' ' . $message; $this->wanCache->set( $titleKey, ' ' . $message, $this->mExpiry, $cacheOpts ); } @@ -1051,7 +1051,7 @@ * @param Title $title * @return string */ - function transform( $message, $interface = false, $language = null, $title = null ) { + public function transform( $message, $interface = false, $language = null, $title = null ) { // Avoid creating parser if nothing to transform if ( strpos( $message, '{{' ) === false ) { return $message; @@ -1080,7 +1080,7 @@ /** * @return Parser */ - function getParser() { + private function getParser() { global $wgParser, $wgParserConf; if ( !$this->mParser && isset( $wgParser ) ) { @@ -1144,11 +1144,11 @@ return $res; } - function disable() { + public function disable() { $this->mDisable = true; } - function enable() { + public function enable() { $this->mDisable = false; } @@ -1171,7 +1171,7 @@ /** * Clear all stored messages. Mainly used after a mass rebuild. */ - function clear() { + public function clear() { $langs = Language::fetchLanguageNames( null, 'mw' ); foreach ( array_keys( $langs ) as $code ) { # Global and local caches @@ -1240,16 +1240,9 @@ public function updateMessageOverride( Title $title, Content $content ) { global $wgContLang; - // @TODO: could skip pseudo-messages like js/css here, based on content model - if ( $content ) { - // Message page edited - $msgText = $content ? $content->getWikitextForTransclusion() : null; - if ( $msgText === false || $msgText === null ) { - $msgText = ''; - } - } else { - // Message page deleted or text is inaccesible - $msgText = false; + $msgText = $this->getMessageTextFromContent( $content ); + if ( $msgText === null ) { + $msgText = false; // treat as not existing } $this->replace( $title->getDBkey(), $msgText ); @@ -1258,4 +1251,34 @@ $wgContLang->updateConversionTable( $title ); } } + + /** + * @param Content|null $content Content or null if the message page does not exist + * @return string|bool|null Returns false if $content is null and null on error + */ + private function getMessageTextFromContent( Content $content = null ) { + // @TODO: could skip pseudo-messages like js/css here, based on content model + if ( $content ) { + // Message page exists... + // XXX: Is this the right way to turn a Content object into a message? + // NOTE: $content is typically either WikitextContent, JavaScriptContent or + // CssContent. MessageContent is *not* used for storing messages, it's + // only used for wrapping them when needed. + $msgText = $content->getWikitextForTransclusion(); + if ( $msgText === false || $msgText === null ) { + // This might be due to some kind of misconfiguration... + $msgText = null; + wfDebugLog( + 'MessageCache', + __METHOD__ . ": message content doesn't provide wikitext " + . "(content model: " . $content->getModel() . ")" + ); + } + } else { + // Message page does not exist... + $msgText = false; + } + + return $msgText; + } } -- To view, visit https://gerrit.wikimedia.org/r/318489 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I462554b300d4688b09ab80cd1bb8a4340ffaa786 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits