From 88640fd9029008a864fe9f342b58d665a5342871 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 26 Jul 2019 15:33:40 -0400 Subject: [PATCH] objectcache: fix failing tests for non-HashBagOStuff backends 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 --- includes/libs/objectcache/BagOStuff.php | 2 +- includes/libs/objectcache/HashBagOStuff.php | 2 +- .../objectcache/MediumSpecificBagOStuff.php | 40 ++++++++++++------- .../libs/objectcache/MemcachedBagOStuff.php | 11 +++-- includes/libs/objectcache/RedisBagOStuff.php | 26 ++++++------ includes/objectcache/SqlBagOStuff.php | 30 ++++++++------ .../libs/objectcache/BagOStuffTest.php | 36 ++++++++--------- 7 files changed, 82 insertions(+), 65 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index e9fd7d90be..da60c01959 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -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 ); } } diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php index 83c8004c26..1cfa0c7921 100644 --- a/includes/libs/objectcache/HashBagOStuff.php +++ b/includes/libs/objectcache/HashBagOStuff.php @@ -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 ]; diff --git a/includes/libs/objectcache/MediumSpecificBagOStuff.php b/includes/libs/objectcache/MediumSpecificBagOStuff.php index 23cf607c7c..fe17628e81 100644 --- a/includes/libs/objectcache/MediumSpecificBagOStuff.php +++ b/includes/libs/objectcache/MediumSpecificBagOStuff.php @@ -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 ); } /** diff --git a/includes/libs/objectcache/MemcachedBagOStuff.php b/includes/libs/objectcache/MemcachedBagOStuff.php index 9f1c98ab58..ff9dedf3a1 100644 --- a/includes/libs/objectcache/MemcachedBagOStuff.php +++ b/includes/libs/objectcache/MemcachedBagOStuff.php @@ -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; } } diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php index 87d26ef8fd..bc298cae1a 100644 --- a/includes/libs/objectcache/RedisBagOStuff.php +++ b/includes/libs/objectcache/RedisBagOStuff.php @@ -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 ) { diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index 6cc63bffb3..e97dc41af7 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -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; diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php index 4a56fc580f..aeb0a70d7d 100644 --- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php @@ -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" ); } /** -- 2.20.1