From 8df2ac4e985a1b572e5eb7ad6eb5b614f15c7d0f Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 3 May 2023 10:47:08 +0100 Subject: [PATCH] MDL-78109 core_cache: Remove harmful requirelockingwrite/read options These cache options are not used in core and would cause a performance cost without any benefit if ever used. Also they don't work properly. --- cache/README.md | 6 +- cache/classes/definition.php | 60 +++----------- cache/classes/loaders.php | 152 +---------------------------------- cache/tests/cache_test.php | 48 ----------- cache/upgrade.txt | 4 + 5 files changed, 21 insertions(+), 249 deletions(-) diff --git a/cache/README.md b/cache/README.md index f4987d83fae..d225aa62572 100644 --- a/cache/README.md +++ b/cache/README.md @@ -16,8 +16,7 @@ A definition: ), 'requiredataguarantee' => false, // Optional 'requiremultipleidentifiers' => false, // Optional - 'requirelockingread' => false, // Optional - 'requirelockingwrite' => false, // Optional + 'requirelockingbeforewrite' => false, // Optional 'requiresearchable' => false, // Optional 'maxsize' => null, // Optional 'overrideclass' => null, // Optional @@ -137,8 +136,7 @@ The following optional settings can also be defined: * requireidentifiers - Any identifiers the definition requires. Must be provided when creating the loader. * requiredataguarantee - If set to true then only stores that support data guarantee will be used. * requiremultipleidentifiers - If set to true then only stores that support multiple identifiers will be used. -* requirelockingread - If set to true a lock will be acquired for reading. Don't use this setting unless you have a REALLY good reason to. -* requirelockingwrite - If set to true a lock will be acquired before writing to the cache. Avoid this unless necessary. +* requirelockingbeforewrite - If set to true the system will throw an error if you write to a cache without having a lock on the relevant key. * requiresearchable - If set to true only stores that support key searching will be used for this definition. Its not recommended to use this unless absolutely unavoidable. * maxsize - This gives a cache an indication about the maximum items it should store. Cache stores don't have to use this, it is up to them to decide if its required. * overrideclass - If provided this class will be used for the loader. It must extend one of the core loader classes (based upon mode). diff --git a/cache/classes/definition.php b/cache/classes/definition.php index 52ce88a5641..ca9ae17d5ed 100644 --- a/cache/classes/definition.php +++ b/cache/classes/definition.php @@ -52,14 +52,9 @@ defined('MOODLE_INTERNAL') || die(); * [bool] If set to true then only stores that can guarantee data will remain available once set will be used. * + requiremultipleidentifiers * [bool] If set to true then only stores that support multiple identifiers will be used. - * + requirelockingread - * [bool] If set to true then a lock will be gained before reading from the cache store. It is recommended not to use - * this setting unless 100% absolutely positively required. Remember 99.9% of caches will NOT need this setting. - * This setting will only be used for application caches presently. - * + requirelockingwrite - * [bool] If set to true then a lock will be gained before writing to the cache store. As above this is not recommended - * unless truly needed. Please think about the order of your code and deal with race conditions there first. - * This setting will only be used for application caches presently. + * + requirelockingbeforewrite + * [bool] If set to true then the system will throw an exception if you try to write to + * the cache without having a lock on the relevant keys. * + maxsize * [int] If set this will be used as the maximum number of entries within the cache store for this definition. * Its important to note that cache stores don't actually have to acknowledge this setting or maintain it as a hard limit. @@ -191,23 +186,11 @@ class cache_definition { /** * If set to true then we know that this definition requires the locking functionality. - * This gets set during construction based upon the settings requirelockingread and requirelockingwrite. + * This gets set during construction based upon the setting requirelockingbeforewrite. * @var bool */ protected $requirelocking = false; - /** - * Set to true if this definition requires read locking. - * @var bool - */ - protected $requirelockingread = false; - - /** - * Gets set to true if this definition requires write locking. - * @var bool - */ - protected $requirelockingwrite = false; - /** * Gets set to true if this definition requires a lock to be acquired before a write is attempted. * @var bool @@ -361,8 +344,6 @@ class cache_definition { $requireidentifiers = array(); $requiredataguarantee = false; $requiremultipleidentifiers = false; - $requirelockingread = false; - $requirelockingwrite = false; $requirelockingbeforewrite = false; $requiresearchable = ($mode === cache_store::MODE_SESSION) ? true : false; $maxsize = null; @@ -397,19 +378,20 @@ class cache_definition { } if (array_key_exists('requirelockingread', $definition)) { - $requirelockingread = (bool)$definition['requirelockingread']; + debugging('The cache option requirelockingread is deprecated and now has no effect.', + DEBUG_DEVELOPER); } if (array_key_exists('requirelockingwrite', $definition)) { - $requirelockingwrite = (bool)$definition['requirelockingwrite']; + debugging('The cache option requirelockingwrite is deprecated and now has no effect. ' . + 'Consider removing the option, or using requirelockingbeforewrite.', + DEBUG_DEVELOPER); } if (array_key_exists('requirelockingbeforewrite', $definition)) { $requirelockingbeforewrite = (bool)$definition['requirelockingbeforewrite']; } - if ($requirelockingbeforewrite && ($requirelockingwrite || $requirelockingread)) { - throw new coding_exception('requirelockingbeforewrite cannot be set with requirelockingread or requirelockingwrite - in a cache definition, as this will result in conflicting locks.'); - } - $requirelocking = $requirelockingwrite || $requirelockingbeforewrite || $requirelockingread; + // This generic $requirelocking variable is kept in code in case we ever add + // another locking option, most obviously requirelockingbeforeread. + $requirelocking = $requirelockingbeforewrite; if (array_key_exists('requiresearchable', $definition)) { $requiresearchable = (bool)$definition['requiresearchable']; @@ -535,8 +517,6 @@ class cache_definition { $cachedefinition->requiredataguarantee = $requiredataguarantee; $cachedefinition->requiremultipleidentifiers = $requiremultipleidentifiers; $cachedefinition->requirelocking = $requirelocking; - $cachedefinition->requirelockingread = $requirelockingread; - $cachedefinition->requirelockingwrite = $requirelockingwrite; $cachedefinition->requirelockingbeforewrite = $requirelockingbeforewrite; $cachedefinition->requiresearchable = $requiresearchable; $cachedefinition->maxsize = $maxsize; @@ -740,22 +720,6 @@ class cache_definition { return $this->requirelocking; } - /** - * Returns true if this definition requires read locking. - * @return bool - */ - public function require_locking_read() { - return $this->requirelockingread; - } - - /** - * Returns true if this definition requires write locking. - * @return bool - */ - public function require_locking_write() { - return $this->requirelockingwrite; - } - /** * Returns true if this definition requires a lock to be aquired before a write is attempted. * @return bool diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index f0a12df383b..5347ce8e21b 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -1596,18 +1596,6 @@ class cache_application extends cache implements cache_loader_with_locking { */ protected $requirelocking = false; - /** - * Gets set to true if the cache must use read locking (get|has). - * @var bool - */ - protected $requirelockingread = false; - - /** - * Gets set to true if the cache must use write locking (set|delete) - * @var bool - */ - protected $requirelockingwrite = false; - /** * Gets set to true if the cache writes (set|delete) must have a manual lock created first * @var bool @@ -1640,8 +1628,6 @@ class cache_application extends cache implements cache_loader_with_locking { $this->nativelocking = $this->store_supports_native_locking(); if ($definition->require_locking()) { $this->requirelocking = true; - $this->requirelockingread = $definition->require_locking_read(); - $this->requirelockingwrite = $definition->require_locking_write(); $this->requirelockingbeforewrite = $definition->require_locking_before_write(); } @@ -1785,20 +1771,14 @@ class cache_application extends cache implements cache_loader_with_locking { * @param mixed $data The data to set against the key. * @param bool $setparents If true, sets all parent loaders, otherwise only this one * @return bool True on success, false otherwise. + * @throws coding_exception If a required lock has not beeen acquired */ protected function set_implementation($key, int $version, $data, bool $setparents = true): bool { if ($this->requirelockingbeforewrite && !$this->check_lock_state($key)) { throw new coding_exception('Attempted to set cache key "' . $key . '" without a lock. ' . 'Locking before writes is required for ' . $this->get_definition()->get_id()); } - if ($this->requirelockingwrite && !$this->acquire_lock($key)) { - return false; - } - $result = parent::set_implementation($key, $version, $data, $setparents); - if ($this->requirelockingwrite && !$this->release_lock($key)) { - debugging('Failed to release cache lock on set operation... this should not happen.', DEBUG_DEVELOPER); - } - return $result; + return parent::set_implementation($key, $version, $data, $setparents); } /** @@ -1834,133 +1814,7 @@ class cache_application extends cache implements cache_loader_with_locking { } } } - if ($this->requirelockingwrite) { - $locks = array(); - foreach ($keyvaluearray as $id => $pair) { - $key = $pair['key']; - if ($this->acquire_lock($key)) { - $locks[] = $key; - } else { - unset($keyvaluearray[$id]); - } - } - } - $result = parent::set_many($keyvaluearray); - if ($this->requirelockingwrite) { - foreach ($locks as $key) { - if ($this->release_lock($key)) { - debugging('Failed to release cache lock on set_many operation... this should not happen.', DEBUG_DEVELOPER); - } - } - } - return $result; - } - - /** - * Retrieves the value for the given key from the cache. - * - * @param string|int $key The key for the data being requested. - * @param int $requiredversion Minimum required version of the data or cache::VERSION_NONE - * @param int $strictness One of IGNORE_MISSING | MUST_EXIST - * @param mixed &$actualversion If specified, will be set to the actual version number retrieved - * @return mixed|false The data from the cache or false if the key did not exist within the cache. - */ - protected function get_implementation($key, int $requiredversion, int $strictness, &$actualversion = null) { - if ($this->requirelockingread && $this->check_lock_state($key) === false) { - // Read locking required and someone else has the read lock. - return false; - } - return parent::get_implementation($key, $requiredversion, $strictness, $actualversion); - } - - /** - * Retrieves an array of values for an array of keys. - * - * Using this function comes with potential performance implications. - * Not all cache stores will support get_many/set_many operations and in order to replicate this functionality will call - * the equivalent singular method for each item provided. - * This should not deter you from using this function as there is a performance benefit in situations where the cache store - * does support it, but you should be aware of this fact. - * - * @param array $keys The keys of the data being requested. - * @param int $strictness One of IGNORE_MISSING or MUST_EXIST. - * @return array An array of key value pairs for the items that could be retrieved from the cache. - * If MUST_EXIST was used and not all keys existed within the cache then an exception will be thrown. - * Otherwise any key that did not exist will have a data value of false within the results. - * @throws coding_exception - */ - public function get_many(array $keys, $strictness = IGNORE_MISSING) { - $locks = []; - if ($this->requirelockingread) { - foreach ($keys as $id => $key) { - $locks[$key] = $this->acquire_lock($key); - if (!$locks[$key]) { - if ($strictness === MUST_EXIST) { - throw new coding_exception('Could not acquire read locks for all of the items being requested.'); - } else { - // Can't return this as we couldn't get a read lock. - unset($keys[$id]); - } - } - - } - } - $result = parent::get_many($keys, $strictness); - if ($this->requirelockingread) { - foreach ($locks as $key => $lock) { - $this->release_lock($key); - } - } - return $result; - } - - /** - * Delete the given key from the cache. - * - * @param string|int $key The key to delete. - * @param bool $recurse When set to true the key will also be deleted from all stacked cache loaders and their stores. - * This happens by default and ensure that all the caches are consistent. It is NOT recommended to change this. - * @return bool True of success, false otherwise. - */ - public function delete($key, $recurse = true) { - if ($this->requirelockingwrite && !$this->acquire_lock($key)) { - return false; - } - $result = parent::delete($key, $recurse); - if ($this->requirelockingwrite && !$this->release_lock($key)) { - debugging('Failed to release cache lock on delete operation... this should not happen.', DEBUG_DEVELOPER); - } - return $result; - } - - /** - * Delete all of the given keys from the cache. - * - * @param array $keys The key to delete. - * @param bool $recurse When set to true the key will also be deleted from all stacked cache loaders and their stores. - * This happens by default and ensure that all the caches are consistent. It is NOT recommended to change this. - * @return int The number of items successfully deleted. - */ - public function delete_many(array $keys, $recurse = true) { - if ($this->requirelockingwrite) { - $locks = array(); - foreach ($keys as $id => $key) { - if ($this->acquire_lock($key)) { - $locks[] = $key; - } else { - unset($keys[$id]); - } - } - } - $result = parent::delete_many($keys, $recurse); - if ($this->requirelockingwrite) { - foreach ($locks as $key) { - if ($this->release_lock($key)) { - debugging('Failed to release cache lock on delete_many operation... this should not happen.', DEBUG_DEVELOPER); - } - } - } - return $result; + return parent::set_many($keyvaluearray); } } diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index ae4f8d28e49..dd29c64f409 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -2095,32 +2095,6 @@ class cache_test extends \advanced_testcase { $this->assertEquals(false, $cache2->get('var')); } - /** - * Test application locking. - */ - public function test_application_locking() { - $instance = cache_config_testing::instance(true); - $instance->phpunit_add_definition('phpunit/test_application_locking', array( - 'mode' => cache_store::MODE_APPLICATION, - 'component' => 'phpunit', - 'area' => 'test_application_locking', - 'staticacceleration' => true, - 'staticaccelerationsize' => 1, - 'requirelockingread' => true, - 'requirelockingwrite' => true - )); - $cache = cache::make('phpunit', 'test_application_locking'); - $this->assertInstanceOf(cache_application::class, $cache); - - $this->assertTrue($cache->set('a', 'A')); - $this->assertTrue($cache->set('b', 'B')); - $this->assertTrue($cache->set('c', 'C')); - $this->assertEquals('A', $cache->get('a')); - $this->assertEquals(array('b' => 'B', 'c' => 'C'), $cache->get_many(array('b', 'c'))); - $this->assertTrue($cache->delete('a')); - $this->assertFalse($cache->has('a')); - } - /** * The application locking feature should work with caches that support multiple identifiers * (static cache and MongoDB with a specific setting). @@ -2172,28 +2146,6 @@ class cache_test extends \advanced_testcase { $this->assertFalse($cache->set('b', 'B')); } - - /** - * Test that invalid lock setting combinations are caught. - * - * @covers ::make - */ - public function test_application_conflicting_locks() { - $instance = cache_config_testing::instance(true); - $instance->phpunit_add_definition('phpunit/test_application_locking', array( - 'mode' => cache_store::MODE_APPLICATION, - 'component' => 'phpunit', - 'area' => 'test_application_locking', - 'staticacceleration' => true, - 'staticaccelerationsize' => 1, - 'requirelockingwrite' => true, - 'requirelockingbeforewrite' => true, - )); - - $this->expectException('coding_exception'); - cache::make('phpunit', 'test_application_locking'); - } - /** * Test that locking before write works when writing across multiple layers. * diff --git a/cache/upgrade.txt b/cache/upgrade.txt index 6db6c2f1ca1..32f06aa7057 100644 --- a/cache/upgrade.txt +++ b/cache/upgrade.txt @@ -7,6 +7,10 @@ Information provided here is intended especially for developers. - `\core\lock\lock::extend` - `\core\lock\lock_factory::extend_lock` - `\core\lock\lock_factory::supports_recursion` +* The automatic locking options 'requirelockingread' and 'requirelockingwrite' have been removed. + These features are not used in core, were recommended against in the documentation, had significant + bugs, and had no useful purpose. 'requirelockingbeforewrite' is still available to check that manual + locking has been applied. === 4.2 === * The memcached cachestore has been removed.