objectcache: improve BagOStuff arithmetic method signatures
authorAaron Schulz <aschulz@wikimedia.org>
Wed, 17 Jul 2019 19:19:05 +0000 (12:19 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Sat, 24 Aug 2019 19:50:37 +0000 (12:50 -0700)
Make the default $init value for incrWithInit() be $value.
This is far less suprising and also makes the operation
easier to replicate without conflicts.

Make decr() definitions more explicit since various cache
drivers do not handle negative incr() values (e.g. memcached).

Change-Id: I2b8d642656cc91c841abbd7a55d97eba101b027a

16 files changed:
includes/libs/objectcache/APCBagOStuff.php
includes/libs/objectcache/APCUBagOStuff.php
includes/libs/objectcache/BagOStuff.php
includes/libs/objectcache/CachedBagOStuff.php
includes/libs/objectcache/EmptyBagOStuff.php
includes/libs/objectcache/HashBagOStuff.php
includes/libs/objectcache/MediumSpecificBagOStuff.php
includes/libs/objectcache/MemcachedPeclBagOStuff.php
includes/libs/objectcache/MemcachedPhpBagOStuff.php
includes/libs/objectcache/MultiWriteBagOStuff.php
includes/libs/objectcache/RESTBagOStuff.php
includes/libs/objectcache/RedisBagOStuff.php
includes/libs/objectcache/ReplicatedBagOStuff.php
includes/libs/objectcache/WinCacheBagOStuff.php
includes/objectcache/SqlBagOStuff.php
tests/phpunit/includes/libs/objectcache/BagOStuffTest.php

index aa83b1f..e9bd7be 100644 (file)
@@ -87,11 +87,11 @@ class APCBagOStuff extends MediumSpecificBagOStuff {
                return true;
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                return apc_inc( $key . self::KEY_SUFFIX, $value );
        }
 
-       public function decr( $key, $value = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
                return apc_dec( $key . self::KEY_SUFFIX, $value );
        }
 }
index 80383d1..2b26500 100644 (file)
@@ -85,7 +85,7 @@ class APCUBagOStuff extends MediumSpecificBagOStuff {
                return true;
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                // https://github.com/krakjoe/apcu/issues/166
                if ( apcu_exists( $key . self::KEY_SUFFIX ) ) {
                        return apcu_inc( $key . self::KEY_SUFFIX, $value );
@@ -94,7 +94,7 @@ class APCUBagOStuff extends MediumSpecificBagOStuff {
                }
        }
 
-       public function decr( $key, $value = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
                // https://github.com/krakjoe/apcu/issues/166
                if ( apcu_exists( $key . self::KEY_SUFFIX ) ) {
                        return apcu_dec( $key . self::KEY_SUFFIX, $value );
index 73904e4..42da5f0 100644 (file)
@@ -362,31 +362,36 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar
         * Increase stored value of $key by $value while preserving its TTL
         * @param string $key Key to increase
         * @param int $value Value to add to $key (default: 1) [optional]
+        * @param int $flags Bit field of class WRITE_* constants [optional]
         * @return int|bool New value or false on failure
         */
-       abstract public function incr( $key, $value = 1 );
+       abstract public function incr( $key, $value = 1, $flags = 0 );
 
        /**
         * Decrease stored value of $key by $value while preserving its TTL
         * @param string $key
         * @param int $value Value to subtract from $key (default: 1) [optional]
+        * @param int $flags Bit field of class WRITE_* constants [optional]
         * @return int|bool New value or false on failure
         */
-       abstract public function decr( $key, $value = 1 );
+       abstract public function decr( $key, $value = 1, $flags = 0 );
 
        /**
-        * Increase stored value of $key by $value while preserving its TTL
+        * Increase the value of the given key (no TTL change) if it exists or create it otherwise
         *
-        * This will create the key with value $init and TTL $ttl instead if not present
+        * This will create the key with the value $init and TTL $ttl instead if not present.
+        * Callers should make sure that both ($init - $value) and $ttl are invariants for all
+        * operations to any given key. The value of $init should be at least that of $value.
         *
-        * @param string $key
-        * @param int $ttl
-        * @param int $value
-        * @param int $init
+        * @param string $key Key built via makeKey() or makeGlobalKey()
+        * @param int $exptime Time-to-live (in seconds) or a UNIX timestamp expiration
+        * @param int $value Amount to increase the key value by [default: 1]
+        * @param int|null $init Value to initialize the key to if it does not exist [default: $value]
+        * @param int $flags Bit field of class WRITE_* constants [optional]
         * @return int|bool New value or false on failure
         * @since 1.24
         */
-       abstract public function incrWithInit( $key, $ttl, $value = 1, $init = 1 );
+       abstract public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 );
 
        /**
         * Get the "last error" registered; clearLastError() should be called manually
index b1d2b6c..8b4c9c6 100644 (file)
@@ -87,6 +87,7 @@ class CachedBagOStuff extends BagOStuff {
 
        public function set( $key, $value, $exptime = 0, $flags = 0 ) {
                $this->procCache->set( $key, $value, $exptime, $flags );
+
                if ( !$this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) {
                        $this->backend->set( $key, $value, $exptime, $flags );
                }
@@ -96,6 +97,7 @@ class CachedBagOStuff extends BagOStuff {
 
        public function delete( $key, $flags = 0 ) {
                $this->procCache->delete( $key, $flags );
+
                if ( !$this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) {
                        $this->backend->delete( $key, $flags );
                }
@@ -194,22 +196,22 @@ class CachedBagOStuff extends BagOStuff {
                return true;
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                $this->procCache->delete( $key );
 
-               return $this->backend->incr( $key, $value );
+               return $this->backend->incr( $key, $value, $flags );
        }
 
-       public function decr( $key, $value = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
                $this->procCache->delete( $key );
 
-               return $this->backend->decr( $key, $value );
+               return $this->backend->decr( $key, $value, $flags );
        }
 
-       public function incrWithInit( $key, $ttl, $value = 1, $init = 1 ) {
+       public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) {
                $this->procCache->delete( $key );
 
-               return $this->backend->incrWithInit( $key, $ttl, $value, $init );
+               return $this->backend->incrWithInit( $key, $exptime, $value, $init, $flags );
        }
 
        public function addBusyCallback( callable $workCallback ) {
index b2613b2..9723cad 100644 (file)
@@ -45,11 +45,15 @@ class EmptyBagOStuff extends MediumSpecificBagOStuff {
                return true;
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                return false;
        }
 
-       public function incrWithInit( $key, $ttl, $value = 1, $init = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               return false;
+       }
+
+       public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) {
                return false; // faster
        }
 
index b4087be..6d0940b 100644 (file)
@@ -108,10 +108,10 @@ class HashBagOStuff extends MediumSpecificBagOStuff {
                return true;
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                $n = $this->get( $key );
                if ( $this->isInteger( $n ) ) {
-                       $n = max( $n + intval( $value ), 0 );
+                       $n = max( $n + (int)$value, 0 );
                        $this->bag[$key][self::KEY_VAL] = $n;
 
                        return $n;
@@ -120,6 +120,10 @@ class HashBagOStuff extends MediumSpecificBagOStuff {
                return false;
        }
 
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               return $this->incr( $key, -$value, $flags );
+       }
+
        /**
         * Clear all values in cache
         */
index 1242501..cb3f621 100644 (file)
@@ -208,7 +208,7 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
                        $mainValue->{SerializedValueContainer::SEGMENTED_HASHES}
                );
 
-               return $this->deleteMulti( $orderedKeys, $flags );
+               return $this->deleteMulti( $orderedKeys, $flags & ~self::WRITE_PRUNE_SEGMENTS );
        }
 
        /**
@@ -269,12 +269,12 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
        final protected function mergeViaCas( $key, callable $callback, $exptime, $attempts, $flags ) {
                $attemptsLeft = $attempts;
                do {
-                       $casToken = null; // passed by reference
+                       $token = null; // passed by reference
                        // Get the old value and CAS token from cache
                        $this->clearLastError();
                        $currentValue = $this->resolveSegments(
                                $key,
-                               $this->doGet( $key, self::READ_LATEST, $casToken )
+                               $this->doGet( $key, $flags, $token )
                        );
                        if ( $this->getLastError() ) {
                                // Don't spam slow retries due to network problems (retry only on races)
@@ -302,7 +302,7 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
                                $success = $this->add( $key, $value, $exptime, $flags );
                        } else {
                                // Try to update the key, failing if it gets changed in the meantime
-                               $success = $this->cas( $casToken, $key, $value, $exptime, $flags );
+                               $success = $this->cas( $token, $key, $value, $exptime, $flags );
                        }
                        if ( $this->getLastError() ) {
                                // Don't spam slow retries due to network problems (retry only on races)
@@ -671,37 +671,16 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
                return $res;
        }
 
-       /**
-        * Decrease stored value of $key by $value while preserving its TTL
-        * @param string $key
-        * @param int $value Value to subtract from $key (default: 1) [optional]
-        * @return int|bool New value or false on failure
-        */
-       public function decr( $key, $value = 1 ) {
-               return $this->incr( $key, -$value );
-       }
-
-       /**
-        * Increase stored value of $key by $value while preserving its TTL
-        *
-        * This will create the key with value $init and TTL $ttl instead if not present
-        *
-        * @param string $key
-        * @param int $ttl
-        * @param int $value
-        * @param int $init
-        * @return int|bool New value or false on failure
-        * @since 1.24
-        */
-       public function incrWithInit( $key, $ttl, $value = 1, $init = 1 ) {
+       public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) {
+               $init = is_int( $init ) ? $init : $value;
                $this->clearLastError();
-               $newValue = $this->incr( $key, $value );
+               $newValue = $this->incr( $key, $value, $flags );
                if ( $newValue === false && !$this->getLastError() ) {
                        // No key set; initialize
-                       $newValue = $this->add( $key, (int)$init, $ttl ) ? $init : false;
+                       $newValue = $this->add( $key, (int)$init, $exptime, $flags ) ? $init : false;
                        if ( $newValue === false && !$this->getLastError() ) {
                                // Raced out initializing; increment
-                               $newValue = $this->incr( $key, $value );
+                               $newValue = $this->incr( $key, $value, $flags );
                        }
                }
 
@@ -771,26 +750,6 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
                $this->lastError = $err;
        }
 
-       /**
-        * Let a callback be run to avoid wasting time on special blocking calls
-        *
-        * The callbacks may or may not be called ever, in any particular order.
-        * They are likely to be invoked when something WRITE_SYNC is used used.
-        * They should follow a caching pattern as shown below, so that any code
-        * using the work will get it's result no matter what happens.
-        * @code
-        *     $result = null;
-        *     $workCallback = function () use ( &$result ) {
-        *         if ( !$result ) {
-        *             $result = ....
-        *         }
-        *         return $result;
-        *     }
-        * @endcode
-        *
-        * @param callable $workCallback
-        * @since 1.28
-        */
        final public function addBusyCallback( callable $workCallback ) {
                $this->busyCallbacks[] = $workCallback;
        }
@@ -922,14 +881,6 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
                return ( $value === (string)$integer );
        }
 
-       /**
-        * Construct a cache key.
-        *
-        * @param string $keyspace
-        * @param array $args
-        * @return string Colon-delimited list of $keyspace followed by escaped components of $args
-        * @since 1.27
-        */
        public function makeKeyInternal( $keyspace, $args ) {
                $key = $keyspace;
                foreach ( $args as $arg ) {
@@ -971,18 +922,10 @@ abstract class MediumSpecificBagOStuff extends BagOStuff {
                return $this->attrMap[$flag] ?? self::QOS_UNKNOWN;
        }
 
-       /**
-        * @return int|float The chunk size, in bytes, of segmented objects (INF for no limit)
-        * @since 1.34
-        */
        public function getSegmentationSize() {
                return $this->segmentationSize;
        }
 
-       /**
-        * @return int|float Maximum total segmented object size in bytes (INF for no limit)
-        * @since 1.34
-        */
        public function getSegmentedValueMaxSize() {
                return $this->segmentedValueMaxSize;
        }
index 12c1a7a..9bf3f42 100644 (file)
@@ -250,7 +250,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
                return $this->checkResult( $key, $result );
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                $this->debug( "incr($key)" );
 
                $result = $this->acquireSyncClient()->increment( $key, $value );
@@ -258,7 +258,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
                return $this->checkResult( $key, $result );
        }
 
-       public function decr( $key, $value = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
                $this->debug( "decr($key)" );
 
                $result = $this->acquireSyncClient()->decrement( $key, $value );
index 8144231..fc6deef 100644 (file)
@@ -93,13 +93,13 @@ class MemcachedPhpBagOStuff extends MemcachedBagOStuff {
                );
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                $n = $this->client->incr( $this->validateKeyEncoding( $key ), $value );
 
                return ( $n !== false && $n !== null ) ? $n : false;
        }
 
-       public function decr( $key, $value = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
                $n = $this->client->decr( $this->validateKeyEncoding( $key ), $value );
 
                return ( $n !== false && $n !== null ) ? $n : false;
index 31d73e1..d0aa380 100644 (file)
@@ -123,9 +123,10 @@ class MultiWriteBagOStuff extends BagOStuff {
                        $missIndexes[] = $i;
                }
 
-               if ( $value !== false
-                       && $missIndexes
-                       && $this->fieldHasFlags( $flags, self::READ_VERIFIED )
+               if (
+                       $value !== false &&
+                       $this->fieldHasFlags( $flags, self::READ_VERIFIED ) &&
+                       $missIndexes
                ) {
                        // Backfill the value to the higher (and often faster/smaller) cache tiers
                        $this->doWrite(
@@ -265,7 +266,7 @@ class MultiWriteBagOStuff extends BagOStuff {
                );
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                return $this->doWrite(
                        $this->cacheIndexes,
                        $this->asyncWrites,
@@ -274,7 +275,7 @@ class MultiWriteBagOStuff extends BagOStuff {
                );
        }
 
-       public function decr( $key, $value = 1 ) {
+       public function decr( $key, $value = 1, $flags = 0 ) {
                return $this->doWrite(
                        $this->cacheIndexes,
                        $this->asyncWrites,
@@ -283,7 +284,7 @@ class MultiWriteBagOStuff extends BagOStuff {
                );
        }
 
-       public function incrWithInit( $key, $ttl, $value = 1, $init = 1 ) {
+       public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) {
                return $this->doWrite(
                        $this->cacheIndexes,
                        $this->asyncWrites,
index b8ce38b..82b5ac0 100644 (file)
@@ -188,11 +188,11 @@ class RESTBagOStuff extends MediumSpecificBagOStuff {
                return $this->handleError( "Failed to delete $key", $rcode, $rerr, $rhdrs, $rbody );
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                // @TODO: make this atomic
                $n = $this->get( $key, self::READ_LATEST );
                if ( $this->isInteger( $n ) ) { // key exists?
-                       $n = max( $n + intval( $value ), 0 );
+                       $n = max( $n + (int)$value, 0 );
                        // @TODO: respect $exptime
                        return $this->set( $key, $n ) ? $n : false;
                }
@@ -200,6 +200,10 @@ class RESTBagOStuff extends MediumSpecificBagOStuff {
                return false;
        }
 
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               return $this->incr( $key, -$value, $flags );
+       }
+
        /**
         * Processes the response body.
         *
index 252b1fa..57a2507 100644 (file)
@@ -364,7 +364,7 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
                return $result;
        }
 
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                $conn = $this->getConnection( $key );
                if ( !$conn ) {
                        return false;
@@ -386,6 +386,28 @@ class RedisBagOStuff extends MediumSpecificBagOStuff {
                return $result;
        }
 
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               $conn = $this->getConnection( $key );
+               if ( !$conn ) {
+                       return false;
+               }
+
+               try {
+                       if ( !$conn->exists( $key ) ) {
+                               return false;
+                       }
+                       // @FIXME: on races, the key may have a 0 TTL
+                       $result = $conn->decrBy( $key, $value );
+               } catch ( RedisException $e ) {
+                       $result = false;
+                       $this->handleException( $conn, $e );
+               }
+
+               $this->logRequest( 'decr', $key, $conn->getServer(), $result );
+
+               return $result;
+       }
+
        protected function doChangeTTL( $key, $exptime, $flags ) {
                $conn = $this->getConnection( $key );
                if ( !$conn ) {
index e3ced0d..0b5ac46 100644 (file)
@@ -135,16 +135,16 @@ class ReplicatedBagOStuff extends BagOStuff {
                return $this->writeStore->changeTTLMulti( $keys, $exptime, $flags );
        }
 
-       public function incr( $key, $value = 1 ) {
-               return $this->writeStore->incr( $key, $value );
+       public function incr( $key, $value = 1, $flags = 0 ) {
+               return $this->writeStore->incr( $key, $value, $flags );
        }
 
-       public function decr( $key, $value = 1 ) {
-               return $this->writeStore->decr( $key, $value );
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               return $this->writeStore->decr( $key, $value, $flags );
        }
 
-       public function incrWithInit( $key, $ttl, $value = 1, $init = 1 ) {
-               return $this->writeStore->incrWithInit( $key, $ttl, $value, $init );
+       public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) {
+               return $this->writeStore->incrWithInit( $key, $exptime, $value, $init, $flags );
        }
 
        public function getLastError() {
index 3c4efbb..5b38628 100644 (file)
@@ -100,14 +100,6 @@ class WinCacheBagOStuff extends MediumSpecificBagOStuff {
                return true;
        }
 
-       /**
-        * Construct a cache key.
-        *
-        * @since 1.27
-        * @param string $keyspace
-        * @param array $args
-        * @return string
-        */
        public function makeKeyInternal( $keyspace, $args ) {
                // WinCache keys have a maximum length of 150 characters. From that,
                // subtract the number of characters we need for the keyspace and for
@@ -136,13 +128,7 @@ class WinCacheBagOStuff extends MediumSpecificBagOStuff {
                return $keyspace . ':' . implode( ':', $args );
        }
 
-       /**
-        * Increase stored value of $key by $value while preserving its original TTL
-        * @param string $key Key to increase
-        * @param int $value Value to add to $key (Default 1)
-        * @return int|bool New value or false on failure
-        */
-       public function incr( $key, $value = 1 ) {
+       public function incr( $key, $value = 1, $flags = 0 ) {
                if ( !wincache_lock( $key ) ) { // optimize with FIFO lock
                        return false;
                }
@@ -160,4 +146,8 @@ class WinCacheBagOStuff extends MediumSpecificBagOStuff {
 
                return $n;
        }
+
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               return $this->incr( $key, -$value, $flags );
+       }
 }
index a1820ab..b0db53b 100644 (file)
@@ -22,6 +22,7 @@
  */
 
 use MediaWiki\MediaWikiServices;
+use Wikimedia\AtEase\AtEase;
 use Wikimedia\Rdbms\Database;
 use Wikimedia\Rdbms\IDatabase;
 use Wikimedia\Rdbms\DBError;
@@ -525,7 +526,7 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
                return $this->modifyMulti( [ $key => null ], 0, $flags, self::$OP_DELETE );
        }
 
-       public function incr( $key, $step = 1 ) {
+       public function incr( $key, $step = 1, $flags = 0 ) {
                list( $shardIndex, $tableName ) = $this->getTableByKey( $key );
 
                $newCount = false;
@@ -559,6 +560,10 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
                return $newCount;
        }
 
+       public function decr( $key, $value = 1, $flags = 0 ) {
+               return $this->incr( $key, -$value, $flags );
+       }
+
        public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) {
                return $this->modifyMulti(
                        array_fill_keys( $keys, null ),
@@ -862,9 +867,9 @@ class SqlBagOStuff extends MediumSpecificBagOStuff {
                }
 
                if ( function_exists( 'gzinflate' ) ) {
-                       Wikimedia\suppressWarnings();
+                       AtEase::suppressWarnings();
                        $decomp = gzinflate( $serial );
-                       Wikimedia\restoreWarnings();
+                       AtEase::restoreWarnings();
 
                        if ( $decomp !== false ) {
                                $serial = $decomp;
index f496fa3..d239ac1 100644 (file)
@@ -290,6 +290,10 @@ class BagOStuffTest extends MediaWikiTestCase {
 
                $val = $this->cache->incrWithInit( $key, 0, 1, 3 );
                $this->assertEquals( 4, $val, "Correct init value" );
+               $this->cache->delete( $key );
+
+               $val = $this->cache->incrWithInit( $key, 0, 5 );
+               $this->assertEquals( 5, $val, "Correct init value" );
        }
 
        /**