Make BagOStuff::incr abstract to discourage bad implementations

Callers should really use atomic TTL-preserving implementations
so that calling code works correctly. The old default base class
code did not do either.

Change-Id: Icf66db05e48b86c8d481dc08dc9041bd1fa6dbe9

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index c439f9b..27c6167 100644 (file)
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -636,29 +636,15 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface {
        /**
         * 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)
+        * @param int $value Value to add to $key (default: 1) [optional]
         * @return int|bool New value or false on failure
         */
-       public function incr( $key, $value = 1 ) {
-               if ( !$this->lock( $key, 1 ) ) {
-                       return false;
-               }
-               $n = $this->get( $key, self::READ_LATEST );
-               if ( $this->isInteger( $n ) ) { // key exists?
-                       $n += intval( $value );
-                       $this->set( $key, max( 0, $n ) ); // exptime?
-               } else {
-                       $n = false;
-               }
-               $this->unlock( $key );
-
-               return $n;
-       }
+       abstract public function incr( $key, $value = 1 );
 
        /**
         * Decrease stored value of $key by $value while preserving its TTL
         * @param string $key
-        * @param int $value
+        * @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 ) {

diff --git a/includes/libs/objectcache/CachedBagOStuff.php b/includes/libs/objectcache/CachedBagOStuff.php
index 95b12b4..95dda71 100644 (file)
--- a/includes/libs/objectcache/CachedBagOStuff.php
+++ b/includes/libs/objectcache/CachedBagOStuff.php
@@ -109,6 +109,13 @@ class CachedBagOStuff extends HashBagOStuff {
                return false; // key already set
        }
 
+       public function incr( $key, $value = 1 ) {
+               $n = $this->backend->incr( $key, $value );
+               parent::delete( $key );
+
+               return $n;
+       }
+
        public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) {
                return $this->backend->lock( $key, $timeout, $expiry, $rclass );
        }

diff --git a/includes/libs/objectcache/EmptyBagOStuff.php b/includes/libs/objectcache/EmptyBagOStuff.php
index 9300dc2..e0f4364 100644 (file)
--- a/includes/libs/objectcache/EmptyBagOStuff.php
+++ b/includes/libs/objectcache/EmptyBagOStuff.php
@@ -43,6 +43,10 @@ class EmptyBagOStuff extends BagOStuff {
                return true;
        }
 
+       public function incr( $key, $value = 1 ) {
+               return false;
+       }
+
        public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
                return true; // faster
        }

diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php
index f88f567..4793ce0 100644 (file)
--- a/includes/libs/objectcache/HashBagOStuff.php
+++ b/includes/libs/objectcache/HashBagOStuff.php
@@ -120,6 +120,18 @@ class HashBagOStuff extends BagOStuff {
                return true;
        }
 
+       public function incr( $key, $value = 1 ) {
+               $n = $this->get( $key );
+               if ( $this->isInteger( $n ) ) {
+                       $n = max( $n + intval( $value ), 0 );
+                       $this->bag[$key][self::KEY_VAL] = $n;
+
+                       return $n;
+               }
+
+               return false;
+       }
+
        public function clear() {
                $this->bag = [];
        }

diff --git a/includes/libs/objectcache/MemcachedBagOStuff.php b/includes/libs/objectcache/MemcachedBagOStuff.php
index 06e90a0..5453862 100644 (file)
--- a/includes/libs/objectcache/MemcachedBagOStuff.php
+++ b/includes/libs/objectcache/MemcachedBagOStuff.php
@@ -79,6 +79,18 @@ class MemcachedBagOStuff extends BagOStuff {
                        $this->fixExpiry( $exptime ) );
        }
 
+       public function incr( $key, $value = 1 ) {
+               $n = $this->client->incr( $this->validateKeyEncoding( $key ), $value );
+
+               return ( $n !== false && $n !== null ) ? $n : false;
+       }
+
+       public function decr( $key, $value = 1 ) {
+               $n = $this->client->decr( $this->validateKeyEncoding( $key ), $value );
+
+               return ( $n !== false && $n !== null ) ? $n : false;
+       }
+
        public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
                return $this->mergeViaCas( $key, $callback, $exptime, $attempts );
        }

diff --git a/includes/libs/objectcache/MemcachedPhpBagOStuff.php b/includes/libs/objectcache/MemcachedPhpBagOStuff.php
index 3ff390b..8f190c3 100644 (file)
--- a/includes/libs/objectcache/MemcachedPhpBagOStuff.php
+++ b/includes/libs/objectcache/MemcachedPhpBagOStuff.php
@@ -58,16 +58,4 @@ class MemcachedPhpBagOStuff extends MemcachedBagOStuff {
 
                return $this->client->get_multi( $keys );
        }
-
-       public function incr( $key, $value = 1 ) {
-               $this->validateKeyEncoding( $key );
-
-               return $this->client->incr( $key, $value ) ?? false;
-       }
-
-       public function decr( $key, $value = 1 ) {
-               $this->validateKeyEncoding( $key );
-
-               return $this->client->decr( $key, $value ) ?? false;
-       }
 }

diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php
index 135556a..c127ec6 100644 (file)
--- a/includes/libs/objectcache/RESTBagOStuff.php
+++ b/includes/libs/objectcache/RESTBagOStuff.php
@@ -126,6 +126,7 @@ class RESTBagOStuff extends BagOStuff {
 
        public function set( $key, $value, $exptime = 0, $flags = 0 ) {
                // @TODO: respect WRITE_SYNC (e.g. EACH_QUORUM)
+               // @TODO: respect $exptime
                $req = [
                        'method' => 'PUT',
                        'url' => $this->url . rawurlencode( $key ),
@@ -139,6 +140,7 @@ class RESTBagOStuff extends BagOStuff {
        }
 
        public function add( $key, $value, $exptime = 0, $flags = 0 ) {
+               // @TODO: make this atomic
                if ( $this->get( $key ) === false ) {
                        return $this->set( $key, $value, $exptime, $flags );
                }
@@ -158,4 +160,16 @@ class RESTBagOStuff extends BagOStuff {
                }
                return $this->handleError( "Failed to delete $key", $rcode, $rerr );
        }
+
+       public function incr( $key, $value = 1 ) {
+               // @TODO: make this atomic
+               $n = $this->get( $key, self::READ_LATEST );
+               if ( $this->isInteger( $n ) ) { // key exists?
+                       $n = max( $n + intval( $value ), 0 );
+                       // @TODO: respect $exptime
+                       return $this->set( $key, $n ) ? $n : false;
+               }
+
+               return false;
+       }
 }