From 962495b17af30ba91dd9f774d2a0449782dfea45 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 26 Mar 2019 16:23:26 -0700 Subject: [PATCH] objectcache: merge getWithToken() into doGet() for simplicity Change-Id: I581f866521e1086ca350973d9cdeff6656f48fe8 --- includes/libs/objectcache/APCBagOStuff.php | 6 +--- includes/libs/objectcache/APCUBagOStuff.php | 6 +--- includes/libs/objectcache/BagOStuff.php | 26 +++------------ includes/libs/objectcache/CachedBagOStuff.php | 7 ++-- includes/libs/objectcache/EmptyBagOStuff.php | 4 ++- includes/libs/objectcache/HashBagOStuff.php | 21 +++--------- .../libs/objectcache/MemcachedBagOStuff.php | 8 +---- .../objectcache/MemcachedPeclBagOStuff.php | 2 +- .../libs/objectcache/MultiWriteBagOStuff.php | 6 +++- includes/libs/objectcache/RESTBagOStuff.php | 8 +---- includes/libs/objectcache/RedisBagOStuff.php | 6 +--- .../libs/objectcache/ReplicatedBagOStuff.php | 6 +++- .../libs/objectcache/WinCacheBagOStuff.php | 16 +++------- includes/objectcache/SqlBagOStuff.php | 32 +++++++++++++------ .../libs/objectcache/CachedBagOStuffTest.php | 5 +-- 15 files changed, 62 insertions(+), 97 deletions(-) diff --git a/includes/libs/objectcache/APCBagOStuff.php b/includes/libs/objectcache/APCBagOStuff.php index 0d4ca11ed9..902cd6a0f8 100644 --- a/includes/libs/objectcache/APCBagOStuff.php +++ b/includes/libs/objectcache/APCBagOStuff.php @@ -72,11 +72,7 @@ class APCBagOStuff extends BagOStuff { } } - protected function doGet( $key, $flags = 0 ) { - return $this->unserialize( apc_fetch( $key . self::KEY_SUFFIX ) ); - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $casToken = null; $blob = apc_fetch( $key . self::KEY_SUFFIX ); diff --git a/includes/libs/objectcache/APCUBagOStuff.php b/includes/libs/objectcache/APCUBagOStuff.php index c36a1dc894..da6544b3d1 100644 --- a/includes/libs/objectcache/APCUBagOStuff.php +++ b/includes/libs/objectcache/APCUBagOStuff.php @@ -39,11 +39,7 @@ class APCUBagOStuff extends APCBagOStuff { parent::__construct( $params ); } - protected function doGet( $key, $flags = 0 ) { - return $this->unserialize( apcu_fetch( $key . self::KEY_SUFFIX ) ); - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $casToken = null; $blob = apcu_fetch( $key . self::KEY_SUFFIX ); diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index bdfed82f05..5669366595 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -175,13 +175,9 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { * * @param string $key * @param int $flags Bitfield of BagOStuff::READ_* constants [optional] - * @param int|null $oldFlags [unused] * @return mixed Returns false on failure or if the item does not exist */ - public function get( $key, $flags = 0, $oldFlags = null ) { - // B/C for ( $key, &$casToken = null, $flags = 0 ) - $flags = is_int( $oldFlags ) ? $oldFlags : $flags; - + public function get( $key, $flags = 0 ) { $this->trackDuplicateKeys( $key ); return $this->doGet( $key, $flags ); @@ -223,22 +219,10 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { /** * @param string $key * @param int $flags Bitfield of BagOStuff::READ_* constants [optional] + * @param mixed|null &$casToken Token to use for check-and-set comparisons * @return mixed Returns false on failure or if the item does not exist */ - abstract protected function doGet( $key, $flags = 0 ); - - /** - * @note This method is only needed if merge() uses mergeViaCas() - * - * @param string $key - * @param mixed &$casToken - * @param int $flags Bitfield of BagOStuff::READ_* constants [optional] - * @return mixed Returns false on failure or if the item does not exist - * @throws Exception - */ - protected function getWithToken( $key, &$casToken, $flags = 0 ) { - throw new Exception( __METHOD__ . ' not implemented.' ); - } + abstract protected function doGet( $key, $flags = 0, &$casToken = null ); /** * Set an item @@ -308,7 +292,7 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { $reportDupes = $this->reportDupes; $this->reportDupes = false; $casToken = null; // passed by reference - $currentValue = $this->getWithToken( $key, $casToken, self::READ_LATEST ); + $currentValue = $this->doGet( $key, self::READ_LATEST, $casToken ); $this->reportDupes = $reportDupes; if ( $this->getLastError() ) { @@ -365,7 +349,7 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { } $curCasToken = null; // passed by reference - $this->getWithToken( $key, $curCasToken, self::READ_LATEST ); + $this->doGet( $key, self::READ_LATEST, $curCasToken ); if ( $casToken === $curCasToken ) { $success = $this->set( $key, $value, $exptime, $flags ); } else { diff --git a/includes/libs/objectcache/CachedBagOStuff.php b/includes/libs/objectcache/CachedBagOStuff.php index 95dda71f2a..8892f73899 100644 --- a/includes/libs/objectcache/CachedBagOStuff.php +++ b/includes/libs/objectcache/CachedBagOStuff.php @@ -32,6 +32,7 @@ * up going to the HashBagOStuff used for the in-memory cache). * * @ingroup Cache + * @TODO: Make this class use composition instead of calling super */ class CachedBagOStuff extends HashBagOStuff { /** @var BagOStuff */ @@ -50,10 +51,10 @@ class CachedBagOStuff extends HashBagOStuff { $this->attrMap = $backend->attrMap; } - protected function doGet( $key, $flags = 0 ) { - $ret = parent::doGet( $key, $flags ); + public function get( $key, $flags = 0 ) { + $ret = parent::get( $key, $flags ); if ( $ret === false && !$this->hasKey( $key ) ) { - $ret = $this->backend->doGet( $key, $flags ); + $ret = $this->backend->get( $key, $flags ); $this->set( $key, $ret, 0, self::WRITE_CACHE_ONLY ); } return $ret; diff --git a/includes/libs/objectcache/EmptyBagOStuff.php b/includes/libs/objectcache/EmptyBagOStuff.php index e0f4364228..ffe3a4c53e 100644 --- a/includes/libs/objectcache/EmptyBagOStuff.php +++ b/includes/libs/objectcache/EmptyBagOStuff.php @@ -27,7 +27,9 @@ * @ingroup Cache */ class EmptyBagOStuff extends BagOStuff { - protected function doGet( $key, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { + $casToken = null; + return false; } diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php index d1f312a6ce..d24f40854e 100644 --- a/includes/libs/objectcache/HashBagOStuff.php +++ b/includes/libs/objectcache/HashBagOStuff.php @@ -58,12 +58,10 @@ class HashBagOStuff extends BagOStuff { } } - protected function doGet( $key, $flags = 0 ) { - if ( !$this->hasKey( $key ) ) { - return false; - } + protected function doGet( $key, $flags = 0, &$casToken = null ) { + $casToken = null; - if ( $this->expire( $key ) ) { + if ( !$this->hasKey( $key ) || $this->expire( $key ) ) { return false; } @@ -72,18 +70,9 @@ class HashBagOStuff extends BagOStuff { unset( $this->bag[$key] ); $this->bag[$key] = $temp; - return $this->bag[$key][self::KEY_VAL]; - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { - $casToken = null; + $casToken = $this->bag[$key][self::KEY_CAS]; - $value = $this->doGet( $key ); - if ( $value !== false ) { - $casToken = $this->bag[$key][self::KEY_CAS]; - } - - return $value; + return $this->bag[$key][self::KEY_VAL]; } public function set( $key, $value, $exptime = 0, $flags = 0 ) { diff --git a/includes/libs/objectcache/MemcachedBagOStuff.php b/includes/libs/objectcache/MemcachedBagOStuff.php index b0fbecef97..71e3331708 100644 --- a/includes/libs/objectcache/MemcachedBagOStuff.php +++ b/includes/libs/objectcache/MemcachedBagOStuff.php @@ -50,13 +50,7 @@ class MemcachedBagOStuff extends BagOStuff { ]; } - protected function doGet( $key, $flags = 0 ) { - $casToken = null; - - return $this->getWithToken( $key, $casToken, $flags ); - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { return $this->client->get( $this->validateKeyEncoding( $key ), $casToken ); } diff --git a/includes/libs/objectcache/MemcachedPeclBagOStuff.php b/includes/libs/objectcache/MemcachedPeclBagOStuff.php index 62a57b6c36..489f00102e 100644 --- a/includes/libs/objectcache/MemcachedPeclBagOStuff.php +++ b/includes/libs/objectcache/MemcachedPeclBagOStuff.php @@ -138,7 +138,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff { return $params; } - protected function getWithToken( $key, &$casToken, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $this->debugLog( "get($key)" ); if ( defined( Memcached::class . '::GET_EXTENDED' ) ) { // v3.0.0 $flags = Memcached::GET_EXTENDED; diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php index fb9971734b..adb9bb8ae4 100644 --- a/includes/libs/objectcache/MultiWriteBagOStuff.php +++ b/includes/libs/objectcache/MultiWriteBagOStuff.php @@ -103,7 +103,7 @@ class MultiWriteBagOStuff extends BagOStuff { } } - protected function doGet( $key, $flags = 0 ) { + public function get( $key, $flags = 0 ) { if ( ( $flags & self::READ_LATEST ) == self::READ_LATEST ) { // If the latest write was a delete(), we do NOT want to fallback // to the other tiers and possibly see the old value. Also, this @@ -349,4 +349,8 @@ class MultiWriteBagOStuff extends BagOStuff { public function makeGlobalKey( $class, $component = null ) { return $this->caches[0]->makeGlobalKey( ...func_get_args() ); } + + protected function doGet( $key, $flags = 0, &$casToken = null ) { + throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); + } } diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php index 3303692adc..4d8ae596d9 100644 --- a/includes/libs/objectcache/RESTBagOStuff.php +++ b/includes/libs/objectcache/RESTBagOStuff.php @@ -84,13 +84,7 @@ class RESTBagOStuff extends BagOStuff { $this->client->setLogger( $logger ); } - protected function doGet( $key, $flags = 0 ) { - $casToken = null; - - return $this->getWithToken( $key, $casToken, $flags ); - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $casToken = null; $req = [ diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php index af90c5c742..79859dba45 100644 --- a/includes/libs/objectcache/RedisBagOStuff.php +++ b/includes/libs/objectcache/RedisBagOStuff.php @@ -86,13 +86,9 @@ class RedisBagOStuff extends BagOStuff { $this->attrMap[self::ATTR_SYNCWRITES] = self::QOS_SYNCWRITES_NONE; } - protected function doGet( $key, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $casToken = null; - return $this->getWithToken( $key, $casToken, $flags ); - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { list( $server, $conn ) = $this->getConnection( $key ); if ( !$conn ) { return false; diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php index eb3f6f8555..70f9096001 100644 --- a/includes/libs/objectcache/ReplicatedBagOStuff.php +++ b/includes/libs/objectcache/ReplicatedBagOStuff.php @@ -74,7 +74,7 @@ class ReplicatedBagOStuff extends BagOStuff { $this->readStore->setDebug( $debug ); } - protected function doGet( $key, $flags = 0 ) { + public function get( $key, $flags = 0 ) { return ( $flags & self::READ_LATEST ) ? $this->writeStore->get( $key, $flags ) : $this->readStore->get( $key, $flags ); @@ -160,4 +160,8 @@ class ReplicatedBagOStuff extends BagOStuff { public function makeGlobalKey( $class, $component = null ) { return $this->writeStore->makeGlobalKey( ...func_get_args() ); } + + protected function doGet( $key, $flags = 0, &$casToken = null ) { + throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); + } } diff --git a/includes/libs/objectcache/WinCacheBagOStuff.php b/includes/libs/objectcache/WinCacheBagOStuff.php index fb64d77f66..818f6f1dc2 100644 --- a/includes/libs/objectcache/WinCacheBagOStuff.php +++ b/includes/libs/objectcache/WinCacheBagOStuff.php @@ -28,13 +28,7 @@ * @ingroup Cache */ class WinCacheBagOStuff extends BagOStuff { - protected function doGet( $key, $flags = 0 ) { - $blob = wincache_ucache_get( $key ); - - return is_string( $blob ) ? unserialize( $blob ) : false; - } - - protected function getWithToken( $key, &$casToken, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $casToken = null; $blob = wincache_ucache_get( $key ); @@ -43,12 +37,10 @@ class WinCacheBagOStuff extends BagOStuff { } $value = unserialize( $blob ); - if ( $value === false ) { - return false; + if ( $value !== false ) { + $casToken = (string)$blob; // don't bother hashing this } - $casToken = $blob; // don't bother hashing this - return $value; } @@ -58,7 +50,7 @@ class WinCacheBagOStuff extends BagOStuff { } $curCasToken = null; // passed by reference - $this->getWithToken( $key, $curCasToken, self::READ_LATEST ); + $this->doGet( $key, self::READ_LATEST, $curCasToken ); if ( $casToken === $curCasToken ) { $success = $this->set( $key, $value, $exptime, $flags ); } else { diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index 4fa99993c5..088f94e37c 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -234,22 +234,34 @@ class SqlBagOStuff extends BagOStuff { } } - protected function doGet( $key, $flags = 0 ) { + protected function doGet( $key, $flags = 0, &$casToken = null ) { $casToken = null; - return $this->getWithToken( $key, $casToken, $flags ); - } + $blobs = $this->fetchBlobMulti( [ $key ] ); + if ( array_key_exists( $key, $blobs ) ) { + $blob = $blobs[$key]; + $value = $this->unserialize( $blob ); + + $casToken = ( $value !== false ) ? $blob : null; - protected function getWithToken( $key, &$casToken, $flags = 0 ) { - $values = $this->getMulti( [ $key ] ); - if ( array_key_exists( $key, $values ) ) { - $casToken = $values[$key]; - return $values[$key]; + return $value; } + return false; } public function getMulti( array $keys, $flags = 0 ) { + $values = []; + + $blobs = $this->fetchBlobMulti( $keys ); + foreach ( $blobs as $key => $blob ) { + $values[$key] = $this->unserialize( $blob ); + } + + return $values; + } + + public function fetchBlobMulti( array $keys, $flags = 0 ) { $values = []; // array of (key => value) $keysByTable = []; @@ -298,7 +310,7 @@ class SqlBagOStuff extends BagOStuff { if ( $this->isExpired( $db, $row->exptime ) ) { // MISS $this->debug( "get: key has expired" ); } else { // HIT - $values[$key] = $this->unserialize( $db->decodeBlob( $row->value ) ); + $values[$key] = $db->decodeBlob( $row->value ); } } catch ( DBQueryError $e ) { $this->handleWriteError( $e, $db, $row->serverIndex ); @@ -420,7 +432,7 @@ class SqlBagOStuff extends BagOStuff { ], [ 'keyname' => $key, - 'value' => $db->encodeBlob( $this->serialize( $casToken ) ) + 'value' => $db->encodeBlob( $casToken ) ], __METHOD__ ); diff --git a/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php index d0360a99f2..f953319e67 100644 --- a/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php @@ -11,7 +11,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase { /** * @covers CachedBagOStuff::__construct - * @covers CachedBagOStuff::doGet + * @covers CachedBagOStuff::get */ public function testGetFromBackend() { $backend = new HashBagOStuff; @@ -36,6 +36,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase { $cache->set( "key$i", 1 ); $this->assertEquals( 1, $cache->get( "key$i" ) ); $this->assertEquals( 1, $backend->get( "key$i" ) ); + $cache->delete( "key$i" ); $this->assertEquals( false, $cache->get( "key$i" ) ); $this->assertEquals( false, $backend->get( "key$i" ) ); @@ -67,7 +68,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase { } /** - * @covers CachedBagOStuff::doGet + * @covers CachedBagOStuff::get */ public function testCacheBackendMisses() { $backend = new HashBagOStuff; -- 2.20.1