From 560774814d136dd12c781383858bfb468cf10a92 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 18 Mar 2019 16:09:26 -0700 Subject: [PATCH] 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 --- includes/libs/objectcache/BagOStuff.php | 20 +++---------------- includes/libs/objectcache/CachedBagOStuff.php | 7 +++++++ includes/libs/objectcache/EmptyBagOStuff.php | 4 ++++ includes/libs/objectcache/HashBagOStuff.php | 12 +++++++++++ .../libs/objectcache/MemcachedBagOStuff.php | 12 +++++++++++ .../objectcache/MemcachedPhpBagOStuff.php | 12 ----------- includes/libs/objectcache/RESTBagOStuff.php | 14 +++++++++++++ 7 files changed, 52 insertions(+), 29 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index c439f9ba34..27c616725d 100644 --- 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 95b12b4047..95dda71f2a 100644 --- 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 9300dc274d..e0f4364228 100644 --- 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 f88f5671ad..4793ce046e 100644 --- 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 06e90a0b4e..5453862cf2 100644 --- 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 3ff390b813..8f190c3b76 100644 --- 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 135556adb5..c127ec6910 100644 --- 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; + } } -- 2.20.1