From 5c335f9d77b878138d67c2c145ade9baba7a5d44 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 14 Apr 2020 16:17:45 -0700 Subject: [PATCH] objectcache: make BagOStuff key encoding more consistent Add "generic" key methods for quickly deriving keys from key component lists in a bijective manor. This is useful for BagOStuff classes that wrap other BagOStuff instances or for parsing keys to get stats. Make the proxy BagOStuff classes (ReplicatedBagOStuff, MultiWriteBagOStuff, CachedBagOStuff) use "generic" keys so that they can convert to appropriate keys when making backing cache instance method calls. Make EmptyBagOStuff, HashBagOStuff, APCUBagOStuff, RedisBagOStuff, and RESTBagOStuff use "generic" keys rather than those of MediumSpecificBagOStuff::makeKeyInternal(). This lets proxy BagOStuff classes bypass key conversions when used with instances of these classes as backing stores. Also: * Fix missing incr(), incrWithInit(), and decr() return values in MultiWriteBagOStuff. * Make MultiWriteBagOfStuff, ReplicatedBagOStuff, and CachedBagOStuff use similar backend method forwarding styles by using a new BagOStuff method. * Improved various related bits of documentation. Bug: T250239 Bug: T235705 Change-Id: I1eb897c2cea3f5b756dd1e3c457b7cbd817599f5 --- includes/libs/objectcache/APCUBagOStuff.php | 8 + includes/libs/objectcache/BagOStuff.php | 188 ++++++++++-- includes/libs/objectcache/CachedBagOStuff.php | 109 ++++--- includes/libs/objectcache/EmptyBagOStuff.php | 8 + includes/libs/objectcache/HashBagOStuff.php | 8 + .../libs/objectcache/IStoreKeyEncoder.php | 16 +- .../objectcache/MediumSpecificBagOStuff.php | 58 ++-- .../libs/objectcache/MultiWriteBagOStuff.php | 277 ++++++++++++------ includes/libs/objectcache/RESTBagOStuff.php | 8 + includes/libs/objectcache/RedisBagOStuff.php | 8 + .../libs/objectcache/ReplicatedBagOStuff.php | 141 +++++++-- .../objectcache/wancache/WANObjectCache.php | 4 +- .../libs/objectcache/BagOStuffTestBase.php | 13 + .../libs/objectcache/CachedBagOStuffTest.php | 18 +- .../objectcache/MultiWriteBagOStuffTest.php | 84 +++++- .../includes/session/TestBagOStuff.php | 2 +- .../objectcache/ReplicatedBagOStuffTest.php | 91 ++++-- 17 files changed, 756 insertions(+), 285 deletions(-) diff --git a/includes/libs/objectcache/APCUBagOStuff.php b/includes/libs/objectcache/APCUBagOStuff.php index 6f2d8db93c4..d516aad3aaf 100644 --- a/includes/libs/objectcache/APCUBagOStuff.php +++ b/includes/libs/objectcache/APCUBagOStuff.php @@ -161,4 +161,12 @@ class APCUBagOStuff extends MediumSpecificBagOStuff { return $result; } + + public function makeKeyInternal( $keyspace, $components ) { + return $this->genericKeyFromComponents( $keyspace, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys + } } diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index a9e88a84afb..a82296345ce 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -71,13 +71,16 @@ abstract class BagOStuff implements { /** @var LoggerInterface */ protected $logger; - /** @var callable|null */ protected $asyncHandler; + /** @var int[] Map of (ATTR_* class constant => QOS_* class constant) */ protected $attrMap = []; - /** @var bool */ + /** @var string Default keyspace; used by makeKey() */ + protected $keyspace; + + /** @var bool Whether to send debug log entries to the SPI logger instance */ protected $debugMode = false; /** @var float|null */ @@ -93,15 +96,40 @@ abstract class BagOStuff implements public const WRITE_PRUNE_SEGMENTS = 32; // delete all the segments if the value is partitioned public const WRITE_BACKGROUND = 64; // if supported, do not block on completion until the next read + /** @var string Global keyspace; used by makeGlobalKey() */ + protected const GLOBAL_KEYSPACE = 'global'; + /** @var string Precomputed global cache key prefix (needs no encoding) */ + protected const GLOBAL_PREFIX = 'global:'; + /** @var string Precomputed global cache key prefix length */ + protected const GLOBAL_PREFIX_LEN = 7; + + /** @var int Item is a single cache key */ + protected const ARG0_KEY = 0; + /** @var int Item is an array of cache keys */ + protected const ARG0_KEYARR = 1; + /** @var int Item is an array indexed by cache keys */ + protected const ARG0_KEYMAP = 2; + /** @var int Item does not involve any keys */ + protected const ARG0_NONKEY = 3; + + /** @var int Item is an array indexed by cache keys */ + protected const RES_KEYMAP = 0; + /** @var int Item does not involve any keys */ + protected const RES_NONKEY = 1; + /** * Parameters include: - * - logger: Psr\Log\LoggerInterface instance + * - keyspace: Keyspace to use for keys in makeKey(). [Default: "local"] * - asyncHandler: Callable to use for scheduling tasks after the web request ends. - * In CLI mode, it should run the task immediately. + * In CLI mode, it should run the task immediately. [Default: null] + * - logger: Psr\Log\LoggerInterface instance. [optional] * @param array $params - * @phan-param array{logger?:Psr\Log\LoggerInterface,asyncHandler?:callable} $params + * @codingStandardsIgnoreStart + * @phan-param array{keyspace?:string,logger?:Psr\Log\LoggerInterface,asyncHandler?:callable} $params + * @codingStandardsIgnoreEnd */ public function __construct( array $params = [] ) { + $this->keyspace = $params['keyspace'] ?? 'local'; $this->setLogger( $params['logger'] ?? new NullLogger() ); $this->asyncHandler = $params['asyncHandler'] ?? null; } @@ -310,13 +338,13 @@ abstract class BagOStuff implements } /** - * Delete all objects expiring before a certain date. + * Delete all objects expiring before a certain date + * * @param string|int $timestamp The reference date in MW or TS_UNIX format * @param callable|null $progress Optional, a function which will be called * regularly during long-running operations with the percentage progress * as the first parameter. [optional] * @param int $limit Maximum number of keys to delete [default: INF] - * * @return bool Success; false if unimplemented */ abstract public function deleteObjectsExpiringBefore( @@ -326,7 +354,8 @@ abstract class BagOStuff implements ); /** - * Get an associative array containing the item for each of the keys that have items. + * Get an associative array containing the item for each of the keys that have items + * * @param string[] $keys List of keys * @param int $flags Bitfield; supports READ_LATEST [optional] * @return mixed[] Map of (key => value) for existing keys @@ -340,13 +369,13 @@ abstract class BagOStuff implements * * WRITE_BACKGROUND can be used for bulk insertion where the response is not vital * - * @param mixed[] $data Map of (key => value) + * @param mixed[] $valueByKey Map of (key => value) * @param int $exptime Either an interval in seconds or a unix timestamp for expiry * @param int $flags Bitfield of BagOStuff::WRITE_* constants (since 1.33) * @return bool Success * @since 1.24 */ - abstract public function setMulti( array $data, $exptime = 0, $flags = 0 ); + abstract public function setMulti( array $valueByKey, $exptime = 0, $flags = 0 ); /** * Batch deletion @@ -377,6 +406,7 @@ abstract class BagOStuff implements /** * 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] @@ -386,6 +416,7 @@ abstract class BagOStuff implements /** * 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] @@ -446,36 +477,51 @@ abstract class BagOStuff implements abstract public function addBusyCallback( callable $workCallback ); /** - * Construct a cache key. + * Make a cache key for the given keyspace and components + * + * Long components might be converted to respective hashes due to size constraints. + * In extreme cases, all of them might be combined into a single hash component. * * @internal This method should not be used outside of BagOStuff (since 1.36) + * + * @param string $keyspace Keyspace component + * @param string[]|int[] $components Key components (key collection name first) + * @return string Keyspace-prepended list of encoded components as a colon-separated value * @since 1.27 - * @param string $keyspace - * @param array $components - * @return string Colon-delimited list of $keyspace followed by escaped components of $args */ abstract public function makeKeyInternal( $keyspace, $components ); /** - * Make a global cache key. + * Make a cache key for the default keyspace and given components * + * @param string $class Key collection name component + * @param string|int ...$components Key components for entity IDs + * @return string Keyspace-prepended list of encoded components as a colon-separated value * @since 1.27 - * @param string $class Key class - * @param string|int ...$components Key components (starting with a key collection name) - * @return string Colon-delimited list of $keyspace followed by escaped components */ abstract public function makeGlobalKey( $class, ...$components ); /** - * Make a cache key, scoped to this instance's keyspace. + * Make a cache key for the global keyspace and given components * + * @param string $class Key collection name component + * @param string|int ...$components Key components for entity IDs + * @return string Keyspace-prepended list of encoded components as a colon-separated value * @since 1.27 - * @param string $class Key class - * @param string|int ...$components Key components (starting with a key collection name) - * @return string Colon-delimited list of $keyspace followed by escaped components */ abstract public function makeKey( $class, ...$components ); + /** + * Check whether a cache key is in the global keyspace + * + * @param string $key + * @return bool + * @since 1.35 + */ + public function isKeyGlobal( $key ) { + return ( strncmp( $key, self::GLOBAL_PREFIX, self::GLOBAL_PREFIX_LEN ) === 0 ); + } + /** * @param int $flag ATTR_* class constant * @return int QOS_* class constant @@ -533,7 +579,7 @@ abstract class BagOStuff implements } /** - * Prepare values for storage and get their serialized sizes, or, estimate those sizes + * Make a "generic" reversible cache key from the given components * * All previously prepared values will be cleared. Each of the new prepared values will be * individually cleared as they get used by write operations for that key. This is done to @@ -559,6 +605,102 @@ abstract class BagOStuff implements */ abstract public function setNewPreparedValues( array $valueByKey ); + /** + * At a minimum, there must be a keyspace and collection name component + * + * @param string|int ...$components Key components for keyspace, collection name, and IDs + * @return string Keyspace-prepended list of encoded components as a colon-separated value + * @since 1.35 + */ + final protected function genericKeyFromComponents( ...$components ) { + if ( count( $components ) < 2 ) { + throw new InvalidArgumentException( "Missing keyspace or collection name" ); + } + + $key = ''; + foreach ( $components as $component ) { + if ( $key !== '' ) { + $key .= ':'; + } + // Escape delimiter (":") and escape ("%") characters + $key .= strtr( $component, [ '%' => '%25', ':' => '%3A' ] ); + } + + return $key; + } + + /** + * Extract the components from a "generic" reversible cache key + * + * @see BagOStuff::genericKeyFromComponents() + * + * @param string $key Keyspace-prepended list of encoded components as a colon-separated value + * @return string[] Key components for keyspace, collection name, and IDs + * @since 1.35 + */ + final protected function componentsFromGenericKey( $key ) { + // Note that the order of each corresponding search/replace pair matters + return str_replace( [ '%3A', '%25' ], [ ':', '%' ], explode( ':', $key ) ); + } + + /** + * Convert a "generic" reversible cache key into one for this cache + * + * @see BagOStuff::genericKeyFromComponents() + * + * @param string $key Keyspace-prepended list of encoded components as a colon-separated value + * @return string Keyspace-prepended list of encoded components as a colon-separated value + */ + abstract protected function convertGenericKey( $key ); + + /** + * Call a method on behalf of wrapper BagOStuff instance that uses "generic" keys + * + * @param string $method Name of a non-final public method that reads/changes keys + * @param int $arg0Sig BagOStuff::ARG0_* constant describing argument 0 + * @param int $resSig BagOStuff::RES_* constant describing the return value + * @param array $genericArgs Method arguments passed to the wrapper instance + * @return mixed Method result with any resulting cache keys remapped to "generic" keys + */ + protected function proxyCall( $method, $arg0Sig, $resSig, array $genericArgs ) { + // Get the corresponding store-specific cache keys... + $storeArgs = $genericArgs; + switch ( $arg0Sig ) { + case self::ARG0_KEY: + $storeArgs[0] = $this->convertGenericKey( $genericArgs[0] ); + break; + case self::ARG0_KEYARR: + foreach ( $genericArgs[0] as $i => $genericKey ) { + $storeArgs[0][$i] = $this->convertGenericKey( $genericKey ); + } + break; + case self::ARG0_KEYMAP: + $storeArgs[0] = []; + foreach ( $genericArgs[0] as $genericKey => $v ) { + $storeArgs[0][$this->convertGenericKey( $genericKey )] = $v; + } + break; + } + + // Result of invoking the method with the corresponding store-specific cache keys + $storeRes = $this->$method( ...$storeArgs ); + + // Convert any store-specific cache keys in the result back to generic cache keys + if ( $resSig === self::RES_KEYMAP ) { + // Map of (store-specific cache key => generic cache key) + $genericKeyByStoreKey = array_combine( $storeArgs[0], $genericArgs[0] ); + + $genericRes = []; + foreach ( $storeRes as $storeKey => $value ) { + $genericRes[$genericKeyByStoreKey[$storeKey]] = $value; + } + } else { + $genericRes = $storeRes; + } + + return $genericRes; + } + /** * @internal For testing only * @return float UNIX timestamp diff --git a/includes/libs/objectcache/CachedBagOStuff.php b/includes/libs/objectcache/CachedBagOStuff.php index 1b1760d836b..e53945b61b4 100644 --- a/includes/libs/objectcache/CachedBagOStuff.php +++ b/includes/libs/objectcache/CachedBagOStuff.php @@ -36,7 +36,7 @@ */ class CachedBagOStuff extends BagOStuff { /** @var BagOStuff */ - protected $backend; + protected $store; /** @var HashBagOStuff */ protected $procCache; @@ -46,65 +46,78 @@ class CachedBagOStuff extends BagOStuff { * @param array $params Parameters for HashBagOStuff */ public function __construct( BagOStuff $backend, $params = [] ) { + $params['keyspace'] = $backend->keyspace; parent::__construct( $params ); - $this->backend = $backend; + $this->store = $backend; $this->procCache = new HashBagOStuff( $params ); $this->attrMap = $backend->attrMap; } public function setDebug( $enabled ) { parent::setDebug( $enabled ); - $this->backend->setDebug( $enabled ); + $this->store->setDebug( $enabled ); } public function get( $key, $flags = 0 ) { $value = $this->procCache->get( $key, $flags ); - if ( $value === false && !$this->procCache->hasKey( $key ) ) { - $value = $this->backend->get( $key, $flags ); - $this->set( $key, $value, self::TTL_INDEFINITE, self::WRITE_CACHE_ONLY ); + if ( $value !== false || $this->procCache->hasKey( $key ) ) { + return $value; } + $value = $this->store->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); + $this->set( $key, $value, self::TTL_INDEFINITE, self::WRITE_CACHE_ONLY ); + return $value; } public function getMulti( array $keys, $flags = 0 ) { - $valuesByKeyCached = []; + $valueByKeyCached = []; - $keysMissing = []; + $keysFetch = []; foreach ( $keys as $key ) { $value = $this->procCache->get( $key, $flags ); if ( $value === false && !$this->procCache->hasKey( $key ) ) { - $keysMissing[] = $key; + $keysFetch[] = $key; } else { - $valuesByKeyCached[$key] = $value; + $valueByKeyCached[$key] = $value; } } - $valuesByKeyFetched = $this->backend->getMulti( $keysMissing, $flags ); - $this->setMulti( $valuesByKeyFetched, self::TTL_INDEFINITE, self::WRITE_CACHE_ONLY ); + $valueByKeyFetched = $this->store->proxyCall( + __FUNCTION__, + self::ARG0_KEYARR, + self::RES_KEYMAP, + [ $keysFetch, $flags ] + ); + $this->setMulti( $valueByKeyFetched, self::TTL_INDEFINITE, self::WRITE_CACHE_ONLY ); - return $valuesByKeyCached + $valuesByKeyFetched; + return $valueByKeyCached + $valueByKeyFetched; } 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 ); + if ( $this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { + return true; } - return true; + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function delete( $key, $flags = 0 ) { $this->procCache->delete( $key, $flags ); - if ( !$this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { - $this->backend->delete( $key, $flags ); + if ( $this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { + return true; } - return true; + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function add( $key, $value, $exptime = 0, $flags = 0 ) { @@ -121,21 +134,21 @@ class CachedBagOStuff extends BagOStuff { public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { $this->procCache->delete( $key ); - return $this->backend->merge( $key, $callback, $exptime, $attempts, $flags ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function changeTTL( $key, $exptime = 0, $flags = 0 ) { $this->procCache->delete( $key ); - return $this->backend->changeTTL( $key, $exptime, $flags ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) { - return $this->backend->lock( $key, $timeout, $expiry, $rclass ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function unlock( $key ) { - return $this->backend->unlock( $key ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function deleteObjectsExpiringBefore( @@ -145,89 +158,93 @@ class CachedBagOStuff extends BagOStuff { ) { $this->procCache->deleteObjectsExpiringBefore( $timestamp, $progress, $limit ); - return $this->backend->deleteObjectsExpiringBefore( $timestamp, $progress, $limit ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_NONKEY, self::RES_NONKEY, func_get_args() ); } public function makeKeyInternal( $keyspace, $components ) { - return $this->backend->makeKeyInternal( $keyspace, $components ); + return $this->genericKeyFromComponents( $keyspace, ...$components ); } public function makeKey( $class, ...$components ) { - return $this->backend->makeKey( $class, ...$components ); + return $this->genericKeyFromComponents( $this->keyspace, $class, ...$components ); } public function makeGlobalKey( $class, ...$components ) { - return $this->backend->makeGlobalKey( $class, ...$components ); + return $this->genericKeyFromComponents( self::GLOBAL_KEYSPACE, $class, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys } public function getLastError() { - return $this->backend->getLastError(); + return $this->store->getLastError(); } public function clearLastError() { - return $this->backend->clearLastError(); + return $this->store->clearLastError(); } - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { - $this->procCache->setMulti( $data, $exptime, $flags ); + public function setMulti( array $valueByKey, $exptime = 0, $flags = 0 ) { + $this->procCache->setMulti( $valueByKey, $exptime, $flags ); - if ( !$this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { - return $this->backend->setMulti( $data, $exptime, $flags ); + if ( $this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { + return true; } - return true; + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEYMAP, self::RES_NONKEY, func_get_args() ); } public function deleteMulti( array $keys, $flags = 0 ) { $this->procCache->deleteMulti( $keys, $flags ); - if ( !$this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { - return $this->backend->deleteMulti( $keys, $flags ); + if ( $this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { + return true; } - return true; + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEYARR, self::RES_NONKEY, func_get_args() ); } public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) { $this->procCache->changeTTLMulti( $keys, $exptime, $flags ); - if ( !$this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { - return $this->backend->changeTTLMulti( $keys, $exptime, $flags ); + if ( $this->fieldHasFlags( $flags, self::WRITE_CACHE_ONLY ) ) { + return true; } - return true; + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEYARR, self::RES_NONKEY, func_get_args() ); } public function incr( $key, $value = 1, $flags = 0 ) { $this->procCache->delete( $key ); - return $this->backend->incr( $key, $value, $flags ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function decr( $key, $value = 1, $flags = 0 ) { $this->procCache->delete( $key ); - return $this->backend->decr( $key, $value, $flags ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) { $this->procCache->delete( $key ); - return $this->backend->incrWithInit( $key, $exptime, $value, $init, $flags ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function addBusyCallback( callable $workCallback ) { - $this->backend->addBusyCallback( $workCallback ); + $this->store->addBusyCallback( $workCallback ); } public function setNewPreparedValues( array $valueByKey ) { - return $this->backend->setNewPreparedValues( $valueByKey ); + return $this->store->proxyCall( __FUNCTION__, self::ARG0_KEYMAP, self::RES_NONKEY, func_get_args() ); } public function setMockTime( &$time ) { parent::setMockTime( $time ); $this->procCache->setMockTime( $time ); - $this->backend->setMockTime( $time ); + $this->store->setMockTime( $time ); } // @codeCoverageIgnoreEnd diff --git a/includes/libs/objectcache/EmptyBagOStuff.php b/includes/libs/objectcache/EmptyBagOStuff.php index 6ad258991f8..8731b953435 100644 --- a/includes/libs/objectcache/EmptyBagOStuff.php +++ b/includes/libs/objectcache/EmptyBagOStuff.php @@ -66,4 +66,12 @@ class EmptyBagOStuff extends MediumSpecificBagOStuff { public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { return true; // faster } + + public function makeKeyInternal( $keyspace, $components ) { + return $this->genericKeyFromComponents( $keyspace, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys + } } diff --git a/includes/libs/objectcache/HashBagOStuff.php b/includes/libs/objectcache/HashBagOStuff.php index e9043c1fd0d..64d12d7486e 100644 --- a/includes/libs/objectcache/HashBagOStuff.php +++ b/includes/libs/objectcache/HashBagOStuff.php @@ -177,4 +177,12 @@ class HashBagOStuff extends MediumSpecificBagOStuff { public function hasKey( $key ) { return isset( $this->bag[$key] ); } + + public function makeKeyInternal( $keyspace, $components ) { + return $this->genericKeyFromComponents( $keyspace, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys + } } diff --git a/includes/libs/objectcache/IStoreKeyEncoder.php b/includes/libs/objectcache/IStoreKeyEncoder.php index 47732e50878..1e9a5aa8210 100644 --- a/includes/libs/objectcache/IStoreKeyEncoder.php +++ b/includes/libs/objectcache/IStoreKeyEncoder.php @@ -8,20 +8,20 @@ */ interface IStoreKeyEncoder { /** - * Make a global cache key. + * Make a cache key using the "global" keyspace for the given components * - * @param string $class Key class - * @param string|int ...$components Key components (starting with a key collection name) - * @return string Colon-delimited list of $keyspace followed by escaped components + * @param string $class Key collection name component + * @param string|int ...$components Key components for entity IDs + * @return string Keyspace-prepended list of encoded components as a colon-separated value */ public function makeGlobalKey( $class, ...$components ); /** - * Make a cache key, scoped to this instance's keyspace. + * Make a cache key using the default keyspace for the given components * - * @param string $class Key class - * @param string|int ...$components Key components (starting with a key collection name) - * @return string Colon-delimited list of $keyspace followed by escaped components + * @param string $class Key collection name component + * @param string|int ...$components Key components for entity IDs + * @return string Keyspace-prepended list of encoded components as a colon-separated value */ public function makeKey( $class, ...$components ); } diff --git a/includes/libs/objectcache/MediumSpecificBagOStuff.php b/includes/libs/objectcache/MediumSpecificBagOStuff.php index 64d40ced03b..84681c6b3df 100644 --- a/includes/libs/objectcache/MediumSpecificBagOStuff.php +++ b/includes/libs/objectcache/MediumSpecificBagOStuff.php @@ -36,8 +36,6 @@ abstract class MediumSpecificBagOStuff extends BagOStuff { protected $locks = []; /** @var int ERR_* class constant */ protected $lastError = self::ERR_NONE; - /** @var string */ - protected $keyspace = 'local'; /** @var int Seconds */ protected $syncTimeout; /** @var int Bytes; chunk size of segmented cache values */ @@ -68,7 +66,6 @@ abstract class MediumSpecificBagOStuff extends BagOStuff { * @see BagOStuff::__construct() * Additional $params options include: * - logger: Psr\Log\LoggerInterface instance - * - keyspace: Default keyspace for $this->makeKey() * - reportDupes: Whether to emit warning log messages for all keys that were * requested more than once (requires an asyncHandler). * - syncTimeout: How long to wait with WRITE_SYNC in seconds. @@ -80,16 +77,12 @@ abstract class MediumSpecificBagOStuff extends BagOStuff { * amount of I/O between application and cache servers that the network can handle. * @param array $params * @codingStandardsIgnoreStart - * @phan-param array{logger?:Psr\Log\LoggerInterface,asyncHandler?:callable,keyspace?:string,reportDupes?:bool,syncTimeout?:int,segmentationSize?:int,segmentedValueMaxSize?:int} $params + * @phan-param array{logger?:Psr\Log\LoggerInterface,asyncHandler?:callable,reportDupes?:bool,syncTimeout?:int,segmentationSize?:int,segmentedValueMaxSize?:int} $params * @codingStandardsIgnoreEnd */ public function __construct( array $params = [] ) { parent::__construct( $params ); - if ( isset( $params['keyspace'] ) ) { - $this->keyspace = $params['keyspace']; - } - if ( !empty( $params['reportDupes'] ) && is_callable( $this->asyncHandler ) ) { $this->reportDupes = true; } @@ -600,18 +593,18 @@ abstract class MediumSpecificBagOStuff extends BagOStuff { * * This does not support WRITE_ALLOW_SEGMENTS to avoid excessive read I/O * - * @param mixed[] $data Map of (key => value) + * @param mixed[] $valueByKey Map of (key => value) * @param int $exptime Either an interval in seconds or a unix timestamp for expiry * @param int $flags Bitfield of BagOStuff::WRITE_* constants (since 1.33) * @return bool Success * @since 1.24 */ - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { + public function setMulti( array $valueByKey, $exptime = 0, $flags = 0 ) { if ( $this->fieldHasFlags( $flags, self::WRITE_ALLOW_SEGMENTS ) ) { throw new InvalidArgumentException( __METHOD__ . ' got WRITE_ALLOW_SEGMENTS' ); } - return $this->doSetMulti( $data, $exptime, $flags ); + return $this->doSetMulti( $valueByKey, $exptime, $flags ); } /** @@ -890,43 +883,26 @@ abstract class MediumSpecificBagOStuff extends BagOStuff { return ( $value === (string)$integer ); } - public function makeKeyInternal( $keyspace, $components ) { - $key = $keyspace; - foreach ( $components as $component ) { - $key .= ':' . str_replace( ':', '%3A', $component ); - } - return strtr( $key, ' ', '_' ); - } - - /** - * Make a global cache key. - * - * @param string $class Key class - * @param string|int ...$components Key components (starting with a key collection name) - * @return string Colon-delimited list of $keyspace followed by escaped components - * @since 1.27 - */ public function makeGlobalKey( $class, ...$components ) { - return $this->makeKeyInternal( 'global', func_get_args() ); + return $this->makeKeyInternal( self::GLOBAL_KEYSPACE, func_get_args() ); } - /** - * Make a cache key, scoped to this instance's keyspace. - * - * @param string $class Key class - * @param string|int ...$components Key components (starting with a key collection name) - * @return string Colon-delimited list of $keyspace followed by escaped components - * @since 1.27 - */ public function makeKey( $class, ...$components ) { return $this->makeKeyInternal( $this->keyspace, func_get_args() ); } - /** - * @param int $flag ATTR_* class constant - * @return int QOS_* class constant - * @since 1.28 - */ + protected function convertGenericKey( $key ) { + $components = $this->componentsFromGenericKey( $key ); + if ( count( $components ) < 2 ) { + // Legacy key not from makeKey()/makeGlobalKey(); keep it as-is + return $key; + } + + $keyspace = array_shift( $components ); + + return $this->makeKeyInternal( $keyspace, $components ); + } + public function getQoS( $flag ) { return $this->attrMap[$flag] ?? self::QOS_UNKNOWN; } diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php index e0c535df9a5..c3ed54d0d4c 100644 --- a/includes/libs/objectcache/MultiWriteBagOStuff.php +++ b/includes/libs/objectcache/MultiWriteBagOStuff.php @@ -34,8 +34,9 @@ use Wikimedia\ObjectFactory; * @ingroup Cache */ class MultiWriteBagOStuff extends BagOStuff { - /** @var BagOStuff[] */ + /** @var BagOStuff[] Backing cache stores in order of highest to lowest tier */ protected $caches; + /** @var bool Use async secondary writes */ protected $asyncWrites = false; /** @var int[] List of all backing cache indexes */ @@ -111,17 +112,31 @@ class MultiWriteBagOStuff extends BagOStuff { } public function get( $key, $flags = 0 ) { + $args = func_get_args(); + if ( $this->fieldHasFlags( $flags, 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 // is used by merge(), which only needs to hit the primary. - return $this->caches[0]->get( $key, $flags ); + return $this->callKeyMethodOnTierCache( + 0, + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + $args + ); } $value = false; $missIndexes = []; // backends checked - foreach ( $this->caches as $i => $cache ) { - $value = $cache->get( $key, $flags ); + foreach ( $this->cacheIndexes as $i ) { + $value = $this->callKeyMethodOnTierCache( + $i, + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + $args + ); if ( $value !== false ) { break; } @@ -134,11 +149,12 @@ class MultiWriteBagOStuff extends BagOStuff { $missIndexes ) { // Backfill the value to the higher (and often faster/smaller) cache tiers - $this->doWrite( + $this->callKeyWriteMethodOnTierCaches( $missIndexes, $this->asyncWrites, 'set', - // @TODO: consider using self::WRITE_ALLOW_SEGMENTS here? + self::ARG0_KEY, + self::RES_NONKEY, [ $key, $value, self::$UPGRADE_TTL ] ); } @@ -147,73 +163,99 @@ class MultiWriteBagOStuff extends BagOStuff { } public function set( $key, $value, $exptime = 0, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function delete( $key, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function add( $key, $value, $exptime = 0, $flags = 0 ) { // Try the write to the top-tier cache - $ok = $this->doWrite( - [ 0 ], - $this->usesAsyncWritesGivenFlags( $flags ), + $ok = $this->callKeyMethodOnTierCache( + 0, __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); if ( $ok ) { // Relay the add() using set() if it succeeded. This is meant to handle certain // migration scenarios where the same store might get written to twice for certain - // keys. In that case, it does not make sense to return false due to "self-conflicts". - return $this->doWrite( + // keys. In that case, it makes no sense to return false due to "self-conflicts". + $okSecondaries = $this->callKeyWriteMethodOnTierCaches( array_slice( $this->cacheIndexes, 1 ), - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), 'set', + self::ARG0_KEY, + self::RES_NONKEY, [ $key, $value, $exptime, $flags ] ); + if ( $okSecondaries === false ) { + $ok = false; + } } - return false; + return $ok; } public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function changeTTL( $key, $exptime = 0, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) { // Only need to lock the first cache; also avoids deadlocks - return $this->caches[0]->lock( $key, $timeout, $expiry, $rclass ); + return $this->callKeyMethodOnTierCache( + 0, + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function unlock( $key ) { // Only the first cache is locked - return $this->caches[0]->unlock( $key ); + return $this->callKeyMethodOnTierCache( + 0, + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function deleteObjectsExpiringBefore( @@ -244,127 +286,103 @@ class MultiWriteBagOStuff extends BagOStuff { return $res; } - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { - return $this->doWrite( + public function setMulti( array $valueByKey, $exptime = 0, $flags = 0 ) { + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEYMAP, + self::RES_NONKEY, func_get_args() ); } - public function deleteMulti( array $data, $flags = 0 ) { - return $this->doWrite( + public function deleteMulti( array $keys, $flags = 0 ) { + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEYARR, + self::RES_NONKEY, func_get_args() ); } public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->usesAsyncWritesGivenFlags( $flags ), + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEYARR, + self::RES_NONKEY, func_get_args() ); } public function incr( $key, $value = 1, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->asyncWrites, + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function decr( $key, $value = 1, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->asyncWrites, + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) { - return $this->doWrite( + return $this->callKeyWriteMethodOnTierCaches( $this->cacheIndexes, - $this->asyncWrites, + $this->useAsyncSecondaryWrites( $flags ), __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, func_get_args() ); } public function getLastError() { - return $this->caches[0]->getLastError(); - } - - public function clearLastError() { - $this->caches[0]->clearLastError(); - } - - /** - * Apply a write method to the backing caches specified by $indexes (in order) - * - * @param int[] $indexes List of backing cache indexes - * @param bool $asyncWrites - * @param string $method Method name of backing caches - * @param array $args Arguments to the method of backing caches - * @return bool - */ - protected function doWrite( $indexes, $asyncWrites, $method, array $args ) { - $ret = true; - - if ( array_diff( $indexes, [ 0 ] ) && $asyncWrites && $method !== 'merge' ) { - // Deep-clone $args to prevent misbehavior when something writes an - // object to the BagOStuff then modifies it afterwards, e.g. T168040. - $args = unserialize( serialize( $args ) ); - } - - foreach ( $indexes as $i ) { - $cache = $this->caches[$i]; - if ( $i == 0 || !$asyncWrites ) { - // First store or in sync mode: write now and get result - if ( !$cache->$method( ...$args ) ) { - $ret = false; - } - } else { - // Secondary write in async mode: do not block this HTTP request - $logger = $this->logger; - ( $this->asyncHandler )( - function () use ( $cache, $method, $args, $logger ) { - if ( !$cache->$method( ...$args ) ) { - $logger->warning( "Async $method op failed" ); - } - } - ); + foreach ( $this->caches as $cache ) { + $error = $cache->getLastError(); + if ( $error !== self::ERR_NONE ) { + return $error; } } - return $ret; + return self::ERR_NONE; } - /** - * @param int $flags - * @return bool - */ - protected function usesAsyncWritesGivenFlags( $flags ) { - return $this->fieldHasFlags( $flags, self::WRITE_SYNC ) ? false : $this->asyncWrites; + public function clearLastError() { + foreach ( $this->caches as $cache ) { + $cache->clearLastError(); + } } public function makeKeyInternal( $keyspace, $components ) { - return $this->caches[0]->makeKeyInternal( $keyspace, $components ); + return $this->genericKeyFromComponents( $keyspace, ...$components ); } public function makeKey( $class, ...$components ) { - return $this->caches[0]->makeKey( ...func_get_args() ); + return $this->genericKeyFromComponents( $this->keyspace, $class, ...$components ); } public function makeGlobalKey( $class, ...$components ) { - return $this->caches[0]->makeGlobalKey( ...func_get_args() ); + return $this->genericKeyFromComponents( self::GLOBAL_KEYSPACE, $class, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys } public function addBusyCallback( callable $workCallback ) { @@ -372,7 +390,13 @@ class MultiWriteBagOStuff extends BagOStuff { } public function setNewPreparedValues( array $valueByKey ) { - return $this->caches[0]->setNewPreparedValues( $valueByKey ); + return $this->callKeyMethodOnTierCache( + 0, + __FUNCTION__, + self::ARG0_KEYMAP, + self::RES_NONKEY, + func_get_args() + ); } public function setMockTime( &$time ) { @@ -381,4 +405,77 @@ class MultiWriteBagOStuff extends BagOStuff { $cache->setMockTime( $time ); } } + + /** + * Call a method on the cache instance for the given cache tier (index) + * + * @param int $index Cache tier + * @param string $method Method name + * @param int $arg0Sig BagOStuff::A0_* constant describing argument 0 + * @param int $rvSig BagOStuff::RV_* constant describing the return value + * @param array $args Method arguments + * @return mixed The result of calling the given method + */ + private function callKeyMethodOnTierCache( $index, $method, $arg0Sig, $rvSig, array $args ) { + return $this->caches[$index]->proxyCall( $method, $arg0Sig, $rvSig, $args ); + } + + /** + * Call a write method on the cache instances, in order, for the given tiers (indexes) + * + * @param int[] $indexes List of cache tiers + * @param bool $asyncSecondary Whether to use asynchronous writes for secondary tiers + * @param string $method Method name + * @param int $arg0Sig BagOStuff::ARG0_* constant describing argument 0 + * @param int $resSig BagOStuff::RES_* constant describing the return value + * @param array $args Method arguments + * @return mixed First synchronous result or false if any failed; null if all asynchronous + */ + private function callKeyWriteMethodOnTierCaches( + array $indexes, + $asyncSecondary, + $method, + $arg0Sig, + $resSig, + array $args + ) { + $res = null; + + if ( $asyncSecondary && array_diff( $indexes, [ 0 ] ) && $method !== 'merge' ) { + // Deep-clone $args to prevent misbehavior when something writes an + // object to the BagOStuff then modifies it afterwards, e.g. T168040. + $args = unserialize( serialize( $args ) ); + } + + foreach ( $indexes as $i ) { + $cache = $this->caches[$i]; + + if ( $i == 0 || !$asyncSecondary ) { + // Tier 0 store or in sync mode: write synchronously and get result + $storeRes = $cache->proxyCall( $method, $arg0Sig, $resSig, $args ); + if ( $storeRes === false ) { + $res = false; + } elseif ( $res === null ) { + $res = $storeRes; // first synchronous result + } + } else { + // Secondary write in async mode: do not block this HTTP request + ( $this->asyncHandler )( + function () use ( $cache, $method, $arg0Sig, $resSig, $args ) { + $cache->proxyCall( $method, $arg0Sig, $resSig, $args ); + } + ); + } + } + + return $res; + } + + /** + * @param int $flags + * @return bool + */ + private function useAsyncSecondaryWrites( $flags ) { + return $this->fieldHasFlags( $flags, self::WRITE_SYNC ) ? false : $this->asyncWrites; + } } diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php index 266fa06aaba..c951d07e5e0 100644 --- a/includes/libs/objectcache/RESTBagOStuff.php +++ b/includes/libs/objectcache/RESTBagOStuff.php @@ -222,6 +222,14 @@ class RESTBagOStuff extends MediumSpecificBagOStuff { return $this->incr( $key, -$value, $flags ); } + public function makeKeyInternal( $keyspace, $components ) { + return $this->genericKeyFromComponents( $keyspace, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys + } + /** * Processes the response body. * diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php index 328f5cfa782..e8cef0b562b 100644 --- a/includes/libs/objectcache/RedisBagOStuff.php +++ b/includes/libs/objectcache/RedisBagOStuff.php @@ -521,4 +521,12 @@ class RedisBagOStuff extends MediumSpecificBagOStuff { public function logRequest( $op, $keys, $server, $e = null ) { $this->debug( "$op($keys) on $server: " . ( $e ? "failure" : "success" ) ); } + + public function makeKeyInternal( $keyspace, $components ) { + return $this->genericKeyFromComponents( $keyspace, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys + } } diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php index 7dc4fe33385..8cceb310334 100644 --- a/includes/libs/objectcache/ReplicatedBagOStuff.php +++ b/includes/libs/objectcache/ReplicatedBagOStuff.php @@ -85,50 +85,89 @@ class ReplicatedBagOStuff extends BagOStuff { } public function get( $key, $flags = 0 ) { - return ( + $store = ( $this->hadRecentSessionWrite( [ $key ] ) || $this->fieldHasFlags( $flags, self::READ_LATEST ) ) - ? $this->writeStore->get( $key, $flags ) - : $this->readStore->get( $key, $flags ); + // Try to maintain session consistency and respect READ_LATEST + ? $this->writeStore + // Otherwise, just use the default "read" store + : $this->readStore; + + return $store->proxyCall( __FUNCTION__, self::ARG0_KEY, self::RES_NONKEY, func_get_args() ); } public function set( $key, $value, $exptime = 0, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->set( $key, $value, $exptime, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function delete( $key, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->delete( $key, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function add( $key, $value, $exptime = 0, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->add( $key, $value, $exptime, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->merge( $key, $callback, $exptime, $attempts, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function changeTTL( $key, $exptime = 0, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->changeTTL( $key, $exptime, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function lock( $key, $timeout = 6, $expiry = 6, $rclass = '' ) { - return $this->writeStore->lock( $key, $timeout, $expiry, $rclass ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function unlock( $key ) { - return $this->writeStore->unlock( $key ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function deleteObjectsExpiringBefore( @@ -136,52 +175,91 @@ class ReplicatedBagOStuff extends BagOStuff { callable $progress = null, $limit = INF ) { - return $this->writeStore->deleteObjectsExpiringBefore( $timestamp, $progress, $limit ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_NONKEY, + self::RES_NONKEY, + func_get_args() + ); } public function getMulti( array $keys, $flags = 0 ) { - return ( + $store = ( $this->hadRecentSessionWrite( $keys ) || $this->fieldHasFlags( $flags, self::READ_LATEST ) ) - ? $this->writeStore->getMulti( $keys, $flags ) - : $this->readStore->getMulti( $keys, $flags ); + // Try to maintain session consistency and respect READ_LATEST + ? $this->writeStore + // Otherwise, just use the default "read" store + : $this->readStore; + + return $store->proxyCall( __FUNCTION__, self::ARG0_KEYARR, self::RES_KEYMAP, func_get_args() ); } - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { - $this->remarkRecentSessionWrite( array_keys( $data ) ); + public function setMulti( array $valueByKey, $exptime = 0, $flags = 0 ) { + $this->remarkRecentSessionWrite( array_keys( $valueByKey ) ); - return $this->writeStore->setMulti( $data, $exptime, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEYMAP, + self::RES_KEYMAP, + func_get_args() + ); } public function deleteMulti( array $keys, $flags = 0 ) { $this->remarkRecentSessionWrite( $keys ); - return $this->writeStore->deleteMulti( $keys, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEYARR, + self::RES_NONKEY, + func_get_args() + ); } public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) { $this->remarkRecentSessionWrite( $keys ); - return $this->writeStore->changeTTLMulti( $keys, $exptime, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEYARR, + self::RES_NONKEY, + func_get_args() + ); } public function incr( $key, $value = 1, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->incr( $key, $value, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function decr( $key, $value = 1, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->decr( $key, $value, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function incrWithInit( $key, $exptime, $value = 1, $init = null, $flags = 0 ) { $this->remarkRecentSessionWrite( [ $key ] ); - return $this->writeStore->incrWithInit( $key, $exptime, $value, $init, $flags ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEY, + self::RES_NONKEY, + func_get_args() + ); } public function getLastError() { @@ -196,23 +274,32 @@ class ReplicatedBagOStuff extends BagOStuff { } public function makeKeyInternal( $keyspace, $components ) { - return $this->writeStore->makeKeyInternal( ...func_get_args() ); + return $this->genericKeyFromComponents( $keyspace, ...$components ); } public function makeKey( $class, ...$components ) { - return $this->writeStore->makeKey( ...func_get_args() ); + return $this->genericKeyFromComponents( $this->keyspace, $class, ...$components ); } public function makeGlobalKey( $class, ...$components ) { - return $this->writeStore->makeGlobalKey( ...func_get_args() ); + return $this->genericKeyFromComponents( self::GLOBAL_KEYSPACE, $class, ...$components ); + } + + protected function convertGenericKey( $key ) { + return $key; // short-circuit; already uses "generic" keys } public function addBusyCallback( callable $workCallback ) { - $this->writeStore->addBusyCallback( $workCallback ); + return $this->writeStore->addBusyCallback( $workCallback ); } public function setNewPreparedValues( array $valueByKey ) { - return $this->writeStore->setNewPreparedValues( $valueByKey ); + return $this->writeStore->proxyCall( + __FUNCTION__, + self::ARG0_KEYMAP, + self::RES_NONKEY, + func_get_args() + ); } public function setMockTime( &$time ) { diff --git a/includes/libs/objectcache/wancache/WANObjectCache.php b/includes/libs/objectcache/wancache/WANObjectCache.php index dd661257ecd..d01a47493c7 100644 --- a/includes/libs/objectcache/wancache/WANObjectCache.php +++ b/includes/libs/objectcache/wancache/WANObjectCache.php @@ -2161,7 +2161,7 @@ class WANObjectCache implements /** * @see BagOStuff::makeKey() - * @param string $class Key class + * @param string $class Key collection name * @param string|int ...$components Key components (starting with a key collection name) * @return string Colon-delimited list of $keyspace followed by escaped components * @since 1.27 @@ -2172,7 +2172,7 @@ class WANObjectCache implements /** * @see BagOStuff::makeGlobalKey() - * @param string $class Key class + * @param string $class Key collection name * @param string|int ...$components Key components (starting with a key collection name) * @return string Colon-delimited list of $keyspace followed by escaped components * @since 1.27 diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTestBase.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTestBase.php index f8f6d355e6b..181f8a743d0 100644 --- a/tests/phpunit/includes/libs/objectcache/BagOStuffTestBase.php +++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTestBase.php @@ -62,6 +62,19 @@ abstract class BagOStuffTestBase extends MediaWikiIntegrationTestCase { ); } + /** + * @covers MediumSpecificBagOStuff::isKeyGlobal + */ + public function testKeyIsGlobal() { + $cache = new HashBagOStuff(); + + $localKey = $cache->makeKey( 'first', 'second', 'third' ); + $globalKey = $cache->makeGlobalKey( 'first', 'second', 'third' ); + + $this->assertFalse( $cache->isKeyGlobal( $localKey ) ); + $this->assertTrue( $cache->isKeyGlobal( $globalKey ) ); + } + /** * @covers MediumSpecificBagOStuff::merge * @covers MediumSpecificBagOStuff::mergeViaCas diff --git a/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php index 5445e25ec9d..62ab00812be 100644 --- a/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/CachedBagOStuffTest.php @@ -126,19 +126,20 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase { */ public function testMakeKey() { $backend = $this->getMockBuilder( HashBagOStuff::class ) + ->setConstructorArgs( [ [ 'keyspace' => 'magic' ] ] ) ->setMethods( [ 'makeKey' ] ) ->getMock(); $backend->method( 'makeKey' ) ->willReturn( 'special/logic' ); - // CachedBagOStuff wraps any backend with a process cache - // using HashBagOStuff. Hash has no special key limitations, - // but backends often do. Make sure it uses the backend's - // makeKey() logic, not the one inherited from HashBagOStuff $cache = new CachedBagOStuff( $backend ); - $this->assertEquals( 'special/logic', $backend->makeKey( 'special', 'logic' ) ); - $this->assertEquals( 'special/logic', $cache->makeKey( 'special', 'logic' ) ); + $this->assertSame( 'special/logic', $backend->makeKey( 'special', 'logic' ) ); + $this->assertSame( + 'magic:special:logic', + $cache->makeKey( 'special', 'logic' ), + "Backend keyspace used" + ); } /** @@ -146,6 +147,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase { */ public function testMakeGlobalKey() { $backend = $this->getMockBuilder( HashBagOStuff::class ) + ->setConstructorArgs( [ [ 'keyspace' => 'magic' ] ] ) ->setMethods( [ 'makeGlobalKey' ] ) ->getMock(); $backend->method( 'makeGlobalKey' ) @@ -153,7 +155,7 @@ class CachedBagOStuffTest extends PHPUnit\Framework\TestCase { $cache = new CachedBagOStuff( $backend ); - $this->assertEquals( 'special/logic', $backend->makeGlobalKey( 'special', 'logic' ) ); - $this->assertEquals( 'special/logic', $cache->makeGlobalKey( 'special', 'logic' ) ); + $this->assertSame( 'special/logic', $backend->makeGlobalKey( 'special', 'logic' ) ); + $this->assertSame( 'global:special:logic', $cache->makeGlobalKey( 'special', 'logic' ) ); } } diff --git a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php index 9af4dd54f6c..8eec8bf51cd 100644 --- a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php @@ -25,17 +25,31 @@ class MultiWriteBagOStuffTest extends MediaWikiIntegrationTestCase { /** * @covers MultiWriteBagOStuff::set - * @covers MultiWriteBagOStuff::doWrite */ - public function testSetImmediate() { + public function testSet() { $key = 'key'; $value = 'value'; $this->cache->set( $key, $value ); // Set in tier 1 - $this->assertEquals( $value, $this->cache1->get( $key ), 'Written to tier 1' ); + $this->assertSame( $value, $this->cache1->get( $key ), 'Written to tier 1' ); // Set in tier 2 - $this->assertEquals( $value, $this->cache2->get( $key ), 'Written to tier 2' ); + $this->assertSame( $value, $this->cache2->get( $key ), 'Written to tier 2' ); + } + + /** + * @covers MultiWriteBagOStuff::add + */ + public function testAdd() { + $key = 'key'; + $value = 'value'; + $ok = $this->cache->add( $key, $value ); + + $this->assertTrue( $ok ); + // Set in tier 1 + $this->assertSame( $value, $this->cache1->get( $key ), 'Written to tier 1' ); + // Set in tier 2 + $this->assertSame( $value, $this->cache2->get( $key ), 'Written to tier 2' ); } /** @@ -109,15 +123,18 @@ class MultiWriteBagOStuffTest extends MediaWikiIntegrationTestCase { public function testMakeKey() { $cache1 = $this->getMockBuilder( HashBagOStuff::class ) ->setMethods( [ 'makeKey' ] )->getMock(); - $cache1->expects( $this->once() )->method( 'makeKey' ) - ->willReturn( 'special' ); + $cache1->expects( $this->never() )->method( 'makeKey' ); $cache2 = $this->getMockBuilder( HashBagOStuff::class ) ->setMethods( [ 'makeKey' ] )->getMock(); $cache2->expects( $this->never() )->method( 'makeKey' ); - $cache = new MultiWriteBagOStuff( [ 'caches' => [ $cache1, $cache2 ] ] ); - $this->assertSame( 'special', $cache->makeKey( 'a', 'b' ) ); + $cache = new MultiWriteBagOStuff( [ + 'keyspace' => 'generic', + 'caches' => [ $cache1, $cache2 ] + ] ); + + $this->assertSame( 'generic:a:b', $cache->makeKey( 'a', 'b' ) ); } /** @@ -126,8 +143,7 @@ class MultiWriteBagOStuffTest extends MediaWikiIntegrationTestCase { public function testMakeGlobalKey() { $cache1 = $this->getMockBuilder( HashBagOStuff::class ) ->setMethods( [ 'makeGlobalKey' ] )->getMock(); - $cache1->expects( $this->once() )->method( 'makeGlobalKey' ) - ->willReturn( 'special' ); + $cache1->expects( $this->never() )->method( 'makeGlobalKey' ); $cache2 = $this->getMockBuilder( HashBagOStuff::class ) ->setMethods( [ 'makeGlobalKey' ] )->getMock(); @@ -135,7 +151,7 @@ class MultiWriteBagOStuffTest extends MediaWikiIntegrationTestCase { $cache = new MultiWriteBagOStuff( [ 'caches' => [ $cache1, $cache2 ] ] ); - $this->assertSame( 'special', $cache->makeGlobalKey( 'a', 'b' ) ); + $this->assertSame( 'global:a:b', $cache->makeGlobalKey( 'a', 'b' ) ); } /** @@ -149,4 +165,50 @@ class MultiWriteBagOStuffTest extends MediaWikiIntegrationTestCase { $this->assertTrue( $cache->add( 'key', 1, 30 ) ); } + + /** + * @covers MultiWriteBagOStuff::incr + */ + public function testIncr() { + $key = $this->cache->makeKey( 'key' ); + + $this->cache->add( $key, 7, 30 ); + + $value = $this->cache->incr( $key ); + $this->assertSame( 8, $value, 'Value after incrementing' ); + + $value = $this->cache->get( $key ); + $this->assertSame( 8, $value, 'Value after incrementing' ); + } + + /** + * @covers MultiWriteBagOStuff::decr + */ + public function testDecr() { + $key = $this->cache->makeKey( 'key' ); + + $this->cache->add( $key, 10, 30 ); + + $value = $this->cache->decr( $key ); + $this->assertSame( 9, $value, 'Value after decrementing' ); + + $value = $this->cache->get( $key ); + $this->assertSame( 9, $value, 'Value after decrementing' ); + } + + /** + * @covers MultiWriteBagOStuff::incrWithInit + */ + public function testIncrWithInit() { + $key = $this->cache->makeKey( 'key' ); + $val = $this->cache->incrWithInit( $key, 0, 1, 3 ); + $this->assertSame( 3, $val, "Correct init value" ); + + $val = $this->cache->incrWithInit( $key, 0, 1, 3 ); + $this->assertSame( 4, $val, "Correct init value" ); + $this->cache->delete( $key ); + + $val = $this->cache->incrWithInit( $key, 0, 5 ); + $this->assertSame( 5, $val, "Correct init value" ); + } } diff --git a/tests/phpunit/includes/session/TestBagOStuff.php b/tests/phpunit/includes/session/TestBagOStuff.php index 64148b02e46..2f6f025be5f 100644 --- a/tests/phpunit/includes/session/TestBagOStuff.php +++ b/tests/phpunit/includes/session/TestBagOStuff.php @@ -72,7 +72,7 @@ class TestBagOStuff extends CachedBagOStuff { * @return mixed */ public function getSessionFromBackend( $id ) { - return $this->backend->get( $this->makeKey( 'MWSession', $id ) ); + return $this->store->get( $this->makeKey( 'MWSession', $id ) ); } /** diff --git a/tests/phpunit/unit/includes/libs/objectcache/ReplicatedBagOStuffTest.php b/tests/phpunit/unit/includes/libs/objectcache/ReplicatedBagOStuffTest.php index 446eef35161..bcea31bcf9f 100644 --- a/tests/phpunit/unit/includes/libs/objectcache/ReplicatedBagOStuffTest.php +++ b/tests/phpunit/unit/includes/libs/objectcache/ReplicatedBagOStuffTest.php @@ -14,6 +14,7 @@ class ReplicatedBagOStuffTest extends \MediaWikiUnitTestCase { $this->writeCache = new HashBagOStuff(); $this->readCache = new HashBagOStuff(); $this->cache = new ReplicatedBagOStuff( [ + 'keyspace' => 'repl_local', 'writeFactory' => $this->writeCache, 'readFactory' => $this->readCache, ] ); @@ -23,7 +24,68 @@ class ReplicatedBagOStuffTest extends \MediaWikiUnitTestCase { * @covers ReplicatedBagOStuff::set */ public function testSet() { - $key = 'a key'; + $key = $this->cache->makeKey( 'a', 'key' ); + $value = 'a value'; + + $this->cache->set( $key, $value ); + + $this->assertSame( $value, $this->writeCache->get( $key ), 'Written' ); + $this->assertFalse( $this->readCache->get( $key ), 'Async replication' ); + } + + /** + * @covers ReplicatedBagOStuff::get + */ + public function testGet() { + $key = $this->cache->makeKey( 'a', 'key' ); + + $write = 'new value'; + $this->writeCache->set( $key, $write ); + $read = 'old value'; + $this->readCache->set( $key, $read ); + + $this->assertSame( $read, $this->cache->get( $key ), 'Async replication' ); + } + + /** + * @covers ReplicatedBagOStuff::get + */ + public function testGetAbsent() { + $key = $this->cache->makeKey( 'a', 'key' ); + $value = 'a value'; + $this->writeCache->set( $key, $value ); + + $this->assertFalse( $this->cache->get( $key ), 'Async replication' ); + } + + /** + * @covers ReplicatedBagOStuff::setMulti + * @covers ReplicatedBagOStuff::getMulti + */ + public function testGetSetMulti() { + $keyA = $this->cache->makeKey( 'key', 'a' ); + $keyB = $this->cache->makeKey( 'key', 'b' ); + $valueAOld = 'one old value'; + $valueBOld = 'another old value'; + $valueANew = 'one new value'; + $valueBNew = 'another new value'; + + $this->writeCache->setMulti( [ $keyA => $valueANew, $keyB => $valueBNew ] ); + $this->readCache->setMulti( [ $keyA => $valueAOld, $keyB => $valueBOld ] ); + + $this->assertEquals( + [ $keyA => $valueAOld, $keyB => $valueBOld ], + $this->cache->getMulti( [ $keyA, $keyB ] ), + 'Async replication' + ); + } + + /** + * @covers ReplicatedBagOStuff::get + * @covers ReplicatedBagOStuff::set + */ + public function testGetSetRaw() { + $key = 'a:key'; $value = 'a value'; $this->cache->set( $key, $value ); @@ -32,31 +94,4 @@ class ReplicatedBagOStuffTest extends \MediaWikiUnitTestCase { // Don't write to replica. Replication is deferred to backend. $this->assertFalse( $this->readCache->get( $key ) ); } - - /** - * @covers ReplicatedBagOStuff::get - */ - public function testGet() { - $key = 'a key'; - - $write = 'one value'; - $this->writeCache->set( $key, $write ); - $read = 'another value'; - $this->readCache->set( $key, $read ); - - // Read from replica. - $this->assertEquals( $read, $this->cache->get( $key ) ); - } - - /** - * @covers ReplicatedBagOStuff::get - */ - public function testGetAbsent() { - $key = 'a key'; - $value = 'a value'; - $this->writeCache->set( $key, $value ); - - // Don't read from master. No failover if value is absent. - $this->assertFalse( $this->cache->get( $key ) ); - } }