jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/318489 )
Change subject: Avoid races in MessageCache::replace()
......................................................................
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
---
M includes/cache/MessageCache.php
M tests/phpunit/includes/cache/MessageCacheTest.php
2 files changed, 97 insertions(+), 53 deletions(-)
Approvals:
Krinkle: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/cache/MessageCache.php b/includes/cache/MessageCache.php
index 352a94c..d40156d 100644
--- a/includes/cache/MessageCache.php
+++ b/includes/cache/MessageCache.php
@@ -444,7 +444,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 );
@@ -518,7 +518,7 @@
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 @@
/**
* 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 @@
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 ] );
- }
+ // (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 );
- 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 )
);
+ // 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 ) );
+ }
- // 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() );
- }
+ // Purge the message in the message blob store
+ $resourceloader =
RequestContext::getMain()->getOutput()->getResourceLoader();
+ $blobStore =
$resourceloader->getMessageBlobStore();
+ $blobStore->updateMessage(
$wgContLang->lcfirst( $msg ) );
- 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 ] );
+ },
+ DeferredUpdates::PRESEND
+ );
}
/**
@@ -845,7 +857,7 @@
$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 @@
*/
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 bd744c0..a12c8b2 100644
--- a/tests/phpunit/includes/cache/MessageCacheTest.php
+++ b/tests/phpunit/includes/cache/MessageCacheTest.php
@@ -99,6 +99,37 @@
];
}
+ 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
--
To view, visit https://gerrit.wikimedia.org/r/318489
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I462554b300d4688b09ab80cd1bb8a4340ffaa786
Gerrit-PatchSet: 17
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: AndyRussG <andrew.green...@gmail.com>
Gerrit-Reviewer: Ejegg <eeggles...@wikimedia.org>
Gerrit-Reviewer: Gilles <gdu...@wikimedia.org>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
