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

@@ -16,8 +16,7 @@ A definition:
             ),
             'requiredataguarantee' => false,          // Optional
             'requiremultipleidentifiers' => false,    // Optional
-            'requirelockingread' => false,            // Optional
+            'requirelockingbeforewrite' => false,     // Optional
-            'requirelockingwrite' => 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.
+* 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.
-* requirelockingwrite - If set to true a lock will be acquired before writing to the cache. Avoid this unless necessary.
 * 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).

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
+ *     + requirelockingbeforewrite
- *          [bool] If set to true then a lock will be gained before reading from the cache store. It is recommended not to use
+ *          [bool] If set to true then the system will throw an exception if you try to write to
- *          this setting unless 100% absolutely positively required. Remember 99.9% of caches will NOT need this setting.
+ *          the cache without having a lock on the relevant keys.
- *          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.
  *     + 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)) {
+        // This generic $requirelocking variable is kept in code in case we ever add
-            throw new coding_exception('requirelockingbeforewrite cannot be set with requirelockingread or requirelockingwrite
+        // another locking option, most obviously requirelockingbeforeread.
-                    in a cache definition, as this will result in conflicting locks.');
+        $requirelocking = $requirelockingbeforewrite;
-        }
-        $requirelocking = $requirelockingwrite || $requirelockingbeforewrite || $requirelockingread;
 
         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

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 parent::set_implementation($key, $version, $data, $setparents);
-            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;
     }
 
     /**
@@ -1834,133 +1814,7 @@ class cache_application extends cache implements cache_loader_with_locking {
                 }
             }
         }
-        if ($this->requirelockingwrite) {
+        return parent::set_many($keyvaluearray);
-            $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;
     }
 }
 

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.
      *

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.