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

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index e9fd7d9..da60c01 100644 (file)
--- 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 83c8004..1cfa0c7 100644 (file)
--- 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 23cf607..fe17628 100644 (file)
--- 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 9f1c98a..ff9dedf 100644 (file)
--- 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 87d26ef..bc298ca 100644 (file)
--- 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 6cc63bf..e97dc41 100644 (file)
--- 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 4a56fc5..aeb0a70 100644 (file)
--- 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" );
        }
 
        /**