Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

objectcache: fix failing tests for non-HashBagOStuff backends

Browse source 
Use real time for testing absolute expirations with changeTTL().
Otherwise, backends like memcached or redis will fail since
they do not use the mock time.

Also:
* Make SqlBagOStuff actually override changeTTLMulti() by
  using the right method name
* Check TTL_INDEFINITE more explicitly for clarity
* Rename TTL conversion methods for clarity
* Use isRelativeExpiration() in MemcachedBagOStuff

Change-Id: I9365ceb31d4e7bef65906363d42b8c3020a66346
This commit is contained in:
Aaron Schulz 2019-07-26 15:33:40 -04:00 committed by Krinkle
parent c793bcece4
commit 88640fd902
7 changed files with 82 additions and 65 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/libs/objectcache/BagOStuff.php
View file

@ -130,7 +130,7 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar
if ( $value === false ) {
$value = $callback( $ttl );
if ( $value !== false ) {
if ( $value !== false && $ttl >= 0 ) {
$this->set( $key, $value, $ttl, $flags );
}
}

2
includes/libs/objectcache/HashBagOStuff.php
View file

@ -81,7 +81,7 @@ class HashBagOStuff extends MediumSpecificBagOStuff {
unset( $this->bag[$key] );
$this->bag[$key] = [
self::KEY_VAL => $value,
self::KEY_EXP => $this->convertToExpiry( $exptime ),
self::KEY_EXP => $this->getExpirationAsTimestamp( $exptime ),
self::KEY_CAS => $this->token . ':' . ++self::$casCounter
];

40
includes/libs/objectcache/MediumSpecificBagOStuff.php
View file

@ -407,12 +407,13 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
* @return bool
*/
protected function doChangeTTL( $key, $exptime, $flags ) {
$expiry = $this->convertToExpiry( $exptime );
$delete = ( $expiry != 0 && $expiry < $this->getCurrentTime() );
if ( !$this->lock( $key, 0 ) ) {
return false;
}
$expiry = $this->getExpirationAsTimestamp( $exptime );
$delete = ( $expiry != self::TTL_INDEFINITE && $expiry < $this->getCurrentTime() );
// Use doGet() to avoid having to trigger resolveSegments()
$blob = $this->doGet( $key, self::READ_LATEST );
if ( $blob ) {
@ -784,9 +785,10 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
/**
* @param int $exptime
* @return bool
* @since 1.34
*/
final protected function expiryIsRelative( $exptime ) {
return ( $exptime != 0 && $exptime < ( 10 * self::TTL_YEAR ) );
final protected function isRelativeExpiration( $exptime ) {
return ( $exptime != self::TTL_INDEFINITE && $exptime < ( 10 * self::TTL_YEAR ) );
}
/**
@ -799,11 +801,16 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
* - positive (>= 10 years): absolute UNIX timestamp; return this value
*
* @param int $exptime
* @return int Absolute TTL or 0 for indefinite
* @return int Expiration timestamp or TTL_INDEFINITE for indefinite
* @since 1.34
*/
final protected function convertToExpiry( $exptime ) {
return $this->expiryIsRelative( $exptime )
? (int)$this->getCurrentTime() + $exptime
final protected function getExpirationAsTimestamp( $exptime ) {
if ( $exptime == self::TTL_INDEFINITE ) {
return $exptime;
}
return $this->isRelativeExpiration( $exptime )
? intval( $this->getCurrentTime() + $exptime )
: $exptime;
}
@ -818,12 +825,17 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
* - positive (>= 10 years): absolute UNIX timestamp; return offset to current time
*
* @param int $exptime
* @return int Relative TTL or 0 for indefinite
* @return int Relative TTL or TTL_INDEFINITE for indefinite
* @since 1.34
*/
final protected function convertToRelative( $exptime ) {
return $this->expiryIsRelative( $exptime ) || !$exptime
? (int)$exptime
: max( $exptime - (int)$this->getCurrentTime(), 1 );
final protected function getExpirationAsTTL( $exptime ) {
if ( $exptime == self::TTL_INDEFINITE ) {
return $exptime;
}
return $this->isRelativeExpiration( $exptime )
? $exptime
: (int)max( $exptime - $this->getCurrentTime(), 1 );
}
/**

11
includes/libs/objectcache/MemcachedBagOStuff.php
View file

@ -101,13 +101,12 @@ abstract class MemcachedBagOStuff extends MediumSpecificBagOStuff {
* discarded immediately because the expiry is in the past.
* Clamp expires >30d at 30d, unless they're >=1e9 in which
* case they are likely to really be absolute (1e9 = 2011-09-09)
* @param int $expiry
* @param int $exptime
* @return int
*/
function fixExpiry( $expiry ) {
if ( $expiry > 2592000 && $expiry < 1000000000 ) {
$expiry = 2592000;
}
return (int)$expiry;
protected function fixExpiry( $exptime ) {
return ( $exptime > self::TTL_MONTH && !$this->isRelativeExpiration( $exptime ) )
? self::TTL_MONTH
: (int)$exptime;
}
}

26
includes/libs/objectcache/RedisBagOStuff.php
View file

@ -115,7 +115,7 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
return false;
}
$ttl = $this->convertToRelative( $exptime );
$ttl = $this->getExpirationAsTTL( $exptime );
$e = null;
try {
@ -212,7 +212,7 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
}
}
$ttl = $this->convertToRelative( $exptime );
$ttl = $this->getExpirationAsTTL( $exptime );
$op = $ttl ? 'setex' : 'set';
$result = true;
@ -302,8 +302,10 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
}
}
$relative = $this->expiryIsRelative( $exptime );
$op = ( $exptime == 0 ) ? 'persist' : ( $relative ? 'expire' : 'expireAt' );
$relative = $this->isRelativeExpiration( $exptime );
$op = ( $exptime == self::TTL_INDEFINITE )
? 'persist'
: ( $relative ? 'expire' : 'expireAt' );
$result = true;
foreach ( $batches as $server => $batchKeys ) {
@ -313,12 +315,12 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
try {
$conn->multi( Redis::PIPELINE );
foreach ( $batchKeys as $key ) {
if ( $exptime == 0 ) {
if ( $exptime == self::TTL_INDEFINITE ) {
$conn->persist( $key );
} elseif ( $relative ) {
$conn->expire( $key, $this->convertToRelative( $exptime ) );
$conn->expire( $key, $this->getExpirationAsTTL( $exptime ) );
} else {
$conn->expireAt( $key, $this->convertToExpiry( $exptime ) );
$conn->expireAt( $key, $this->getExpirationAsTimestamp( $exptime ) );
}
}
$batchResult = $conn->exec();
@ -344,7 +346,7 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
return false;
}
$ttl = $this->convertToRelative( $expiry );
$ttl = $this->getExpirationAsTTL( $expiry );
try {
$result = $conn->set(
$key,
@ -389,16 +391,16 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
return false;
}
$relative = $this->expiryIsRelative( $exptime );
$relative = $this->isRelativeExpiration( $exptime );
try {
if ( $exptime == 0 ) {
if ( $exptime == self::TTL_INDEFINITE ) {
$result = $conn->persist( $key );
$this->logRequest( 'persist', $key, $conn->getServer(), $result );
} elseif ( $relative ) {
$result = $conn->expire( $key, $this->convertToRelative( $exptime ) );
$result = $conn->expire( $key, $this->getExpirationAsTTL( $exptime ) );
$this->logRequest( 'expire', $key, $conn->getServer(), $result );
} else {
$result = $conn->expireAt( $key, $this->convertToExpiry( $exptime ) );
$result = $conn->expireAt( $key, $this->getExpirationAsTimestamp( $exptime ) );
$this->logRequest( 'expireAt', $key, $conn->getServer(), $result );
}
} catch ( RedisException $e ) {

30
includes/objectcache/SqlBagOStuff.php
View file

@ -67,7 +67,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
protected $connFailureErrors = [];
/** @var int */
private static $GARBAGE_COLLECT_DELAY_SEC = 1;
private static $GC_DELAY_SEC = 1;
/** @var string */
private static $OP_SET = 'set';
@ -177,7 +177,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
# Don't keep timing out trying to connect for each call if the DB is down
if (
isset( $this->connFailureErrors[$serverIndex] ) &&
( time() - $this->connFailureTimes[$serverIndex] ) < 60
( $this->getCurrentTime() - $this->connFailureTimes[$serverIndex] ) < 60
) {
throw $this->connFailureErrors[$serverIndex];
}
@ -356,7 +356,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
$keysByTable[$serverIndex][$tableName][] = $key;
}
$exptime = $this->convertToExpiry( $exptime );
$exptime = $this->getExpirationAsTimestamp( $exptime );
$result = true;
/** @noinspection PhpUnusedLocalVariableInspection */
@ -473,7 +473,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
protected function cas( $casToken, $key, $value, $exptime = 0, $flags = 0 ) {
list( $serverIndex, $tableName ) = $this->getTableByKey( $key );
$exptime = $this->convertToExpiry( $exptime );
$exptime = $this->getExpirationAsTimestamp( $exptime );
/** @noinspection PhpUnusedLocalVariableInspection */
$silenceScope = $this->silenceTransactionProfiler();
@ -563,7 +563,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
return $ok;
}
protected function doChangeTTLMulti( array $keys, $exptime, $flags = 0 ) {
public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) {
return $this->modifyMulti(
array_fill_keys( $keys, null ),
$exptime,
@ -584,7 +584,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
protected function isExpired( $db, $exptime ) {
return (
$exptime != $this->getMaxDateTime( $db ) &&
wfTimestamp( TS_UNIX, $exptime ) < time()
wfTimestamp( TS_UNIX, $exptime ) < $this->getCurrentTime()
);
}
@ -593,7 +593,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
* @return string
*/
protected function getMaxDateTime( $db ) {
if ( time() > 0x7fffffff ) {
if ( (int)$this->getCurrentTime() > 0x7fffffff ) {
return $db->timestamp( 1 << 62 );
} else {
return $db->timestamp( 0x7fffffff );
@ -611,14 +611,18 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
// Only purge on one in every $this->purgePeriod writes
mt_rand( 0, $this->purgePeriod - 1 ) == 0 &&
// Avoid repeating the delete within a few seconds
( time() - $this->lastGarbageCollect ) > self::$GARBAGE_COLLECT_DELAY_SEC
( $this->getCurrentTime() - $this->lastGarbageCollect ) > self::$GC_DELAY_SEC
) {
$garbageCollector = function () use ( $db ) {
$this->deleteServerObjectsExpiringBefore( $db, time(), null, $this->purgeLimit );
$this->deleteServerObjectsExpiringBefore(
$db, $this->getCurrentTime(),
null,
$this->purgeLimit
);
$this->lastGarbageCollect = time();
};
if ( $this->asyncHandler ) {
$this->lastGarbageCollect = time(); // avoid duplicate enqueues
$this->lastGarbageCollect = $this->getCurrentTime(); // avoid duplicate enqueues
( $this->asyncHandler )( $garbageCollector );
} else {
$garbageCollector();
@ -627,7 +631,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
}
public function expireAll() {
$this->deleteObjectsExpiringBefore( time() );
$this->deleteObjectsExpiringBefore( $this->getCurrentTime() );
}
public function deleteObjectsExpiringBefore(
@ -927,8 +931,9 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
protected function markServerDown( DBError $exception, $serverIndex ) {
unset( $this->conns[$serverIndex] ); // bug T103435
$now = $this->getCurrentTime();
if ( isset( $this->connFailureTimes[$serverIndex] ) ) {
if ( time() - $this->connFailureTimes[$serverIndex] >= 60 ) {
if ( $now - $this->connFailureTimes[$serverIndex] >= 60 ) {
unset( $this->connFailureTimes[$serverIndex] );
unset( $this->connFailureErrors[$serverIndex] );
} else {
@ -936,7 +941,6 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
return;
}
}
$now = time();
$this->logger->info( __METHOD__ . ": Server #$serverIndex down until " . ( $now + 60 ) );
$this->connFailureTimes[$serverIndex] = $now;
$this->connFailureErrors[$serverIndex] = $exception;

36
tests/phpunit/includes/libs/objectcache/BagOStuffTest.php
View file

@ -146,10 +146,7 @@ class BagOStuffTest extends MediaWikiTestCase {
$key4 = $this->cache->makeKey( 'test-key4' );
// cleanup
$this->cache->delete( $key1 );
$this->cache->delete( $key2 );
$this->cache->delete( $key3 );
$this->cache->delete( $key4 );
$this->cache->deleteMulti( [ $key1, $key2, $key3, $key4 ] );
$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], 30 );
$this->assertFalse( $ok, "No keys found" );
@ -158,7 +155,6 @@ class BagOStuffTest extends MediaWikiTestCase {
$this->assertFalse( $this->cache->get( $key3 ) );
$ok = $this->cache->setMulti( [ $key1 => 1, $key2 => 2, $key3 => 3 ] );
$this->assertTrue( $ok, "setMulti() succeeded" );
$this->assertEquals(
3,
@ -172,21 +168,24 @@ class BagOStuffTest extends MediaWikiTestCase {
$this->assertEquals( 2, $this->cache->get( $key2 ) );
$this->assertEquals( 3, $this->cache->get( $key3 ) );
$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], $now + 86400 );
$this->assertTrue( $ok, "Expiry set for all keys" );
$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3, $key4 ], 300 );
$this->assertFalse( $ok, "One key missing" );
$this->assertEquals( 1, $this->cache->get( $key1 ), "Key still live" );
$now = microtime( true ); // real time
$ok = $this->cache->setMulti( [ $key1 => 1, $key2 => 2, $key3 => 3 ] );
$this->assertTrue( $ok, "setMulti() succeeded" );
$ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], $now + 86400 );
$this->assertTrue( $ok, "Expiry set for all keys" );
$this->assertEquals( 1, $this->cache->get( $key1 ), "Key still live" );
$this->assertEquals( 2, $this->cache->incr( $key1 ) );
$this->assertEquals( 3, $this->cache->incr( $key2 ) );
$this->assertEquals( 4, $this->cache->incr( $key3 ) );
// cleanup
$this->cache->delete( $key1 );
$this->cache->delete( $key2 );
$this->cache->delete( $key3 );
$this->cache->delete( $key4 );
$this->cache->deleteMulti( [ $key1, $key2, $key3, $key4 ] );
}
/**
@ -217,12 +216,13 @@ class BagOStuffTest extends MediaWikiTestCase {
*/
public function testGetWithSetCallback() {
$now = 1563892142;
$this->cache->setMockTime( $now );
$key = $this->cache->makeKey( self::TEST_KEY );
$cache = new HashBagOStuff( [] );
$cache->setMockTime( $now );
$key = $cache->makeKey( self::TEST_KEY );
$this->assertFalse( $this->cache->get( $key ), "No value" );
$this->assertFalse( $cache->get( $key ), "No value" );
$value = $this->cache->getWithSetCallback(
$value = $cache->getWithSetCallback(
$key,
30,
function ( &$ttl ) {
@ -233,11 +233,11 @@ class BagOStuffTest extends MediaWikiTestCase {
);
$this->assertEquals( 'hello kitty', $value );
$this->assertEquals( $value, $this->cache->get( $key ), "Value set" );
$this->assertEquals( $value, $cache->get( $key ), "Value set" );
$now += 11;
$this->assertFalse( $this->cache->get( $key ), "Value expired" );
$this->assertFalse( $cache->get( $key ), "Value expired" );
}
/**