jenkins-bot has submitted this change and it was merged.
Change subject: Fix get()/getMulti() check key race condition in WANObjectCache
......................................................................
Fix get()/getMulti() check key race condition in WANObjectCache
If a set() happened around the exact same time as check key was
initialized via get(), the curTTL in future get() calls could
sometimes be 0 instead of a negative value, even though hold-off
should apply.
Change-Id: Ide1fd65112aff425a4798e2ec406d71f2a8e84a7
---
M includes/libs/objectcache/WANObjectCache.php
M tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
2 files changed, 45 insertions(+), 11 deletions(-)
Approvals:
Krinkle: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/libs/objectcache/WANObjectCache.php
b/includes/libs/objectcache/WANObjectCache.php
index 4005abb..95bf743 100644
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -251,6 +251,7 @@
// Fetch all of the raw values
$wrappedValues = $this->cache->getMulti( array_merge(
$valueKeys, $checkKeysFlat ) );
+ // Time used to compare/init "check" keys (derived after
getMulti() to be pessimistic)
$now = microtime( true );
// Collect timestamps from all "check" keys
@@ -282,7 +283,10 @@
foreach ( $purgeValues as $purge ) {
$safeTimestamp = $purge[self::FLD_TIME]
+ $purge[self::FLD_HOLDOFF];
if ( $safeTimestamp >=
$wrappedValues[$vKey][self::FLD_TIME] ) {
- $curTTL = min( $curTTL,
$purge[self::FLD_TIME] - $now );
+ // How long ago this value was
expired by *this* check key
+ $ago = min(
$purge[self::FLD_TIME] - $now, self::TINY_NEGATIVE );
+ // How long ago this value was
expired by *any* known check key
+ $curTTL = min( $curTTL, $ago );
}
}
}
diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
index 1511557..efc37d2 100644
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -303,9 +303,9 @@
// Fake initial check key to be set in the past. Otherwise we'd
have to sleep for
// several seconds during the test to assert the behaviour.
foreach ( array( $checkAll, $check1, $check2 ) as $checkKey ) {
- $this->internalCache->set( $cache::TIME_KEY_PREFIX .
$checkKey,
- $cache::PURGE_VAL_PREFIX . microtime( true ) -
$cache::HOLDOFF_TTL, $cache::CHECK_KEY_TTL );
+ $cache->touchCheckKey( $checkKey,
WANObjectCache::HOLDOFF_NONE );
}
+ usleep( 100 );
$cache->set( 'key1', $value1, 10 );
$cache->set( 'key2', $value2, 10 );
@@ -322,14 +322,12 @@
$result,
'Initial values'
);
- $this->assertEquals(
- array( 'key1' => 0, 'key2' => 0 ),
- $curTTLs,
- 'Initial ttls'
- );
+ $this->assertGreaterThanOrEqual( 9.5, $curTTLs['key1'],
'Initial ttls' );
+ $this->assertLessThanOrEqual( 10.5, $curTTLs['key1'], 'Initial
ttls' );
+ $this->assertGreaterThanOrEqual( 9.5, $curTTLs['key2'],
'Initial ttls' );
+ $this->assertLessThanOrEqual( 10.5, $curTTLs['key2'], 'Initial
ttls' );
$cache->touchCheckKey( $check1 );
- usleep( 100 );
$curTTLs = array();
$result = $cache->getMulti( array( 'key1', 'key2', 'key3' ),
$curTTLs, array(
@@ -344,10 +342,9 @@
'key1 expired by check1, but value still provided'
);
$this->assertLessThan( 0, $curTTLs['key1'], 'key1 TTL expired'
);
- $this->assertEquals( 0, $curTTLs['key2'], 'key2 still valid' );
+ $this->assertGreaterThan( 0, $curTTLs['key2'], 'key2 still
valid' );
$cache->touchCheckKey( $checkAll );
- usleep( 100 );
$curTTLs = array();
$result = $cache->getMulti( array( 'key1', 'key2', 'key3' ),
$curTTLs, array(
@@ -366,6 +363,39 @@
}
/**
+ * @covers WANObjectCache::get()
+ * @covers WANObjectCache::processCheckKeys()
+ */
+ public function testCheckKeyInitHoldoff() {
+ $cache = $this->cache;
+
+ for ( $i = 0; $i < 500; ++$i ) {
+ $key = wfRandomString();
+ $checkKey = wfRandomString();
+ // miss, set, hit
+ $cache->get( $key, $curTTL, array( $checkKey ) );
+ $cache->set( $key, 'val', 10 );
+ $curTTL = null;
+ $v = $cache->get( $key, $curTTL, array( $checkKey ) );
+
+ $this->assertEquals( 'val', $v );
+ $this->assertLessThan( 0, $curTTL, "Step $i: CTL < 0
(miss/set/hit)" );
+ }
+
+ for ( $i = 0; $i < 500; ++$i ) {
+ $key = wfRandomString();
+ $checkKey = wfRandomString();
+ // set, hit
+ $cache->set( $key, 'val', 10 );
+ $curTTL = null;
+ $v = $cache->get( $key, $curTTL, array( $checkKey ) );
+
+ $this->assertEquals( 'val', $v );
+ $this->assertLessThan( 0, $curTTL, "Step $i: CTL < 0
(set/hit)" );
+ }
+ }
+
+ /**
* @covers WANObjectCache::delete()
*/
public function testDelete() {
--
To view, visit https://gerrit.wikimedia.org/r/257012
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ide1fd65112aff425a4798e2ec406d71f2a8e84a7
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@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
