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
