diff --git a/cache/classes/definition.php b/cache/classes/definition.php index e1e673c489394..52ce88a56414a 100644 --- a/cache/classes/definition.php +++ b/cache/classes/definition.php @@ -208,6 +208,12 @@ class cache_definition { */ protected $requirelockingwrite = false; + /** + * Gets set to true if this definition requires a lock to be acquired before a write is attempted. + * @var bool + */ + protected $requirelockingbeforewrite = false; + /** * Gets set to true if this definition requires searchable stores. * @since Moodle 2.4.4 @@ -357,6 +363,7 @@ public static function load($id, array $definition, $unused = null) { $requiremultipleidentifiers = false; $requirelockingread = false; $requirelockingwrite = false; + $requirelockingbeforewrite = false; $requiresearchable = ($mode === cache_store::MODE_SESSION) ? true : false; $maxsize = null; $overrideclass = null; @@ -395,7 +402,14 @@ public static function load($id, array $definition, $unused = null) { if (array_key_exists('requirelockingwrite', $definition)) { $requirelockingwrite = (bool)$definition['requirelockingwrite']; } - $requirelocking = $requirelockingwrite || $requirelockingread; + 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; if (array_key_exists('requiresearchable', $definition)) { $requiresearchable = (bool)$definition['requiresearchable']; @@ -523,6 +537,7 @@ public static function load($id, array $definition, $unused = null) { $cachedefinition->requirelocking = $requirelocking; $cachedefinition->requirelockingread = $requirelockingread; $cachedefinition->requirelockingwrite = $requirelockingwrite; + $cachedefinition->requirelockingbeforewrite = $requirelockingbeforewrite; $cachedefinition->requiresearchable = $requiresearchable; $cachedefinition->maxsize = $maxsize; $cachedefinition->overrideclass = $overrideclass; @@ -741,6 +756,14 @@ 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 + */ + public function require_locking_before_write() { + return $this->requirelockingbeforewrite; + } + /** * Returns true if this definition allows local storage to be used for caching. * @since Moodle 3.1.0 diff --git a/cache/classes/helper.php b/cache/classes/helper.php index db78844626c2a..cf9364e30ea95 100644 --- a/cache/classes/helper.php +++ b/cache/classes/helper.php @@ -382,6 +382,7 @@ protected static function ensure_ready_for_stats($store, $storeclass, $definitio 'misses' => 0, 'sets' => 0, 'iobytes' => cache_store::IO_BYTES_NOT_SUPPORTED, + 'locks' => 0, ) ) ); @@ -392,6 +393,7 @@ protected static function ensure_ready_for_stats($store, $storeclass, $definitio 'misses' => 0, 'sets' => 0, 'iobytes' => cache_store::IO_BYTES_NOT_SUPPORTED, + 'locks' => 0, ); } } diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index c1f6d8ee1e0c8..d60122be400ef 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -1581,12 +1581,24 @@ class cache_application extends cache implements cache_loader_with_locking { */ protected $requirelockingwrite = false; + /** + * Gets set to true if the cache writes (set|delete) must have a manual lock created first + * @var bool + */ + protected $requirelockingbeforewrite = false; + /** * Gets set to a cache_store to use for locking if the caches primary store doesn't support locking natively. * @var cache_lock_interface */ protected $cachelockinstance; + /** + * Store a list of locks acquired by this process. + * @var array + */ + protected $locks; + /** * Overrides the cache construct method. * @@ -1603,6 +1615,7 @@ public function __construct(cache_definition $definition, cache_store $store, $l $this->requirelocking = true; $this->requirelockingread = $definition->require_locking_read(); $this->requirelockingwrite = $definition->require_locking_write(); + $this->requirelockingbeforewrite = $definition->require_locking_before_write(); } $this->handle_invalidation_events(); @@ -1648,13 +1661,24 @@ public function __clone() { * @return bool Returns true if the lock could be acquired, false otherwise. */ public function acquire_lock($key) { + global $CFG; $key = $this->parse_key($key); + $before = microtime(true); if ($this->nativelocking) { - return $this->get_store()->acquire_lock($key, $this->get_identifier()); + $lock = $this->get_store()->acquire_lock($key, $this->get_identifier()); } else { $this->ensure_cachelock_available(); - return $this->cachelockinstance->lock($key, $this->get_identifier()); + $lock = $this->cachelockinstance->lock($key, $this->get_identifier()); + } + $after = microtime(true); + if ($lock) { + $this->locks[$key] = $lock; + if (defined('MDL_PERF') || !empty($CFG->perfdebug)) { + \core\lock\timing_wrapper_lock_factory::record_lock_data($after, $before, + $this->get_definition()->get_id(), $key, $lock, $this->get_identifier() . $key); + } } + return $lock; } /** @@ -1666,6 +1690,9 @@ public function acquire_lock($key) { */ public function check_lock_state($key) { $key = $this->parse_key($key); + if (!empty($this->locks[$key])) { + return true; // Shortcut to save having to make a call to the cache store if the lock is held by this process. + } if ($this->nativelocking) { return $this->get_store()->check_lock_state($key, $this->get_identifier()); } else { @@ -1683,11 +1710,18 @@ public function check_lock_state($key) { public function release_lock($key) { $key = $this->parse_key($key); if ($this->nativelocking) { - return $this->get_store()->release_lock($key, $this->get_identifier()); + $released = $this->get_store()->release_lock($key, $this->get_identifier()); } else { $this->ensure_cachelock_available(); - return $this->cachelockinstance->unlock($key, $this->get_identifier()); + $released = $this->cachelockinstance->unlock($key, $this->get_identifier()); } + if ($released && array_key_exists($key, $this->locks)) { + unset($this->locks[$key]); + if (defined('MDL_PERF') || !empty($CFG->perfdebug)) { + \core\lock\timing_wrapper_lock_factory::record_lock_released_data($this->get_identifier() . $key); + } + } + return $released; } /** @@ -1719,6 +1753,10 @@ protected function ensure_cachelock_available() { * @return bool True on success, false otherwise. */ 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; } @@ -1753,6 +1791,15 @@ protected function set_implementation($key, int $version, $data, bool $setparent * ... if they care that is. */ public function set_many(array $keyvaluearray) { + if ($this->requirelockingbeforewrite) { + foreach ($keyvaluearray as $id => $pair) { + if (!$this->check_lock_state($pair['key'])) { + debugging('Attempted to set cache key "' . $pair['key'] . '" without a lock. ' + . 'Locking before writes is required for ' . $this->get_definition()->get_name(), DEBUG_DEVELOPER); + unset($keyvaluearray[$id]); + } + } + } if ($this->requirelockingwrite) { $locks = array(); foreach ($keyvaluearray as $id => $pair) { diff --git a/cache/disabledlib.php b/cache/disabledlib.php index e27318dead673..c7cde7607dbfa 100644 --- a/cache/disabledlib.php +++ b/cache/disabledlib.php @@ -200,6 +200,26 @@ public function has_any(array $keys) { public function purge() { return true; } + + /** + * Pretend that we got a lock to avoid errors. + * + * @param string $key + * @return bool + */ + public function acquire_lock(string $key) : bool { + return true; + } + + /** + * Pretend that we released a lock to avoid errors. + * + * @param string $key + * @return void + */ + public function release_lock(string $key) : bool { + return true; + } } /** diff --git a/cache/stores/file/addinstanceform.php b/cache/stores/file/addinstanceform.php index a2f3c3a9d46b6..4240d7f595202 100644 --- a/cache/stores/file/addinstanceform.php +++ b/cache/stores/file/addinstanceform.php @@ -62,5 +62,10 @@ protected function configuration_definition() { $form->addElement('checkbox', 'asyncpurge', get_string('asyncpurge', 'cachestore_file')); $form->setType('asyncpurge', PARAM_BOOL); $form->addHelpButton('asyncpurge', 'asyncpurge', 'cachestore_file'); + + $form->addElement('text', 'lockwait', get_string('lockwait', 'cachestore_file')); + $form->setDefault('lockwait', 60); + $form->setType('lockwait', PARAM_INT); + $form->addHelpButton('lockwait', 'lockwait', 'cachestore_file'); } } \ No newline at end of file diff --git a/cache/stores/file/lang/en/cachestore_file.php b/cache/stores/file/lang/en/cachestore_file.php index ceb1bfe9ae827..1562d75269818 100644 --- a/cache/stores/file/lang/en/cachestore_file.php +++ b/cache/stores/file/lang/en/cachestore_file.php @@ -32,6 +32,8 @@ $string['asyncpurge_help'] = 'If enabled, the new directory is created with cache revision and the old directory will be deleted asynchronously via a scheduled task.'; $string['autocreate'] = 'Auto create directory'; $string['autocreate_help'] = 'If enabled the directory specified in path will be automatically created if it does not already exist.'; +$string['lockwait'] = 'Maximum lock wait time'; +$string['lockwait_help'] = 'The maximum amount of time in seconds to wait for an exclusive lock before reading or writing a cache key. This is only used for cache definitions that have read or write locking required.'; $string['path'] = 'Cache path'; $string['path_help'] = 'The directory that should be used to store files for this cache store. If left blank (default) a directory will be automatically created in the moodledata directory. This can be used to point a file store towards a directory on a better performing drive (such as one in memory).'; $string['pluginname'] = 'File cache'; diff --git a/cache/stores/file/lib.php b/cache/stores/file/lib.php index a6e026b02a0bb..12bb8b09fea9b 100644 --- a/cache/stores/file/lib.php +++ b/cache/stores/file/lib.php @@ -37,7 +37,8 @@ * @copyright 2012 Sam Hemelryk * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class cachestore_file extends cache_store implements cache_is_key_aware, cache_is_configurable, cache_is_searchable { +class cachestore_file extends cache_store implements cache_is_key_aware, cache_is_configurable, cache_is_searchable, + cache_is_lockable { /** * The name of the store. @@ -128,6 +129,23 @@ class cachestore_file extends cache_store implements cache_is_key_aware, cache_i */ private $cfg = null; + /** @var int Maximum number of seconds to wait for a lock before giving up. */ + protected $lockwait = 60; + + /** + * Instance of file_lock_factory configured to create locks in the cache directory. + * + * @var \core\lock\file_lock_factory $lockfactory + */ + protected $lockfactory = null; + + /** + * List of current locks. + * + * @var array $locks + */ + protected $locks = []; + /** * Constructs the store instance. * @@ -193,6 +211,21 @@ public function __construct($name, array $configuration = array()) { } else { $this->asyncpurge = false; } + + // Leverage cachelock_file to provide native locking, to avoid duplicating logic. + // This will store locks alongside the cache, so local cache uses local locks. + $lockdir = $path . '/filelocks'; + if (!file_exists($lockdir)) { + make_writable_directory($lockdir); + } + if (array_key_exists('lockwait', $configuration)) { + $this->lockwait = (int)$configuration['lockwait']; + } + $this->lockfactory = new \core\lock\file_lock_factory('cachestore_file', $lockdir); + if (!$this->lockfactory->is_available()) { + // File locking is disabled in config, fall back to default lock factory. + $this->lockfactory = \core\lock\lock_config::get_lock_factory('cachestore_file'); + } } /** @@ -675,6 +708,9 @@ public static function config_get_configuration_array($data) { if (isset($data->asyncpurge)) { $config['asyncpurge'] = $data->asyncpurge; } + if (isset($data->lockwait)) { + $config['lockwait'] = $data->lockwait; + } return $config; } @@ -702,6 +738,9 @@ public static function config_set_edit_form_data(moodleform $editform, array $co if (isset($config['asyncpurge'])) { $data['asyncpurge'] = (bool)$config['asyncpurge']; } + if (isset($config['lockwait'])) { + $data['lockwait'] = (int)$config['lockwait']; + } $editform->set_data($data); } @@ -924,4 +963,70 @@ public function cache_size_details(int $samplekeys = 50): stdClass { $result->sd = sqrt($squarediff); return $result; } + + /** + * Use lock factory to determine the lock state. + * + * @param string $key Lock identifier + * @param string $ownerid Cache identifier + * @return bool|null + */ + public function check_lock_state($key, $ownerid) : ?bool { + if (!array_key_exists($key, $this->locks)) { + return null; // Lock does not exist. + } + if (!array_key_exists($ownerid, $this->locks[$key])) { + return false; // Lock exists, but belongs to someone else. + } + if ($this->locks[$key][$ownerid] instanceof \core\lock\lock) { + return true; // Lock exists, and we own it. + } + // Try to get the lock with an immediate timeout. If this succeeds, the lock does not currently exist. + $lock = $this->lockfactory->get_lock($key, 0); + if ($lock) { + // Lock was not already held. + $lock->release(); + return null; + } else { + // Lock is held by someone else. + return false; + } + } + + /** + * Use lock factory to acquire a lock. + * + * @param string $key Lock identifier + * @param string $ownerid Cache identifier + * @return bool + * @throws cache_exception + */ + public function acquire_lock($key, $ownerid) : bool { + $lock = $this->lockfactory->get_lock($key, $this->lockwait); + if ($lock) { + $this->locks[$key][$ownerid] = $lock; + } + return (bool)$lock; + } + + /** + * Use lock factory to release a lock. + * + * @param string $key Lock identifier + * @param string $ownerid Cache identifier + * @return bool + */ + public function release_lock($key, $ownerid) : bool { + if (!array_key_exists($key, $this->locks)) { + return false; // No lock to release. + } + if (!array_key_exists($ownerid, $this->locks[$key])) { + return false; // Tried to release someone else's lock. + } + $unlocked = $this->locks[$key][$ownerid]->release(); + if ($unlocked) { + unset($this->locks[$key]); + } + return $unlocked; + } } diff --git a/cache/stores/file/tests/store_test.php b/cache/stores/file/tests/store_test.php index 364593bec99c5..d39725bd2c723 100644 --- a/cache/stores/file/tests/store_test.php +++ b/cache/stores/file/tests/store_test.php @@ -33,6 +33,7 @@ * @package cachestore_file * @copyright 2013 Sam Hemelryk * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \cachestore_file */ class store_test extends \cachestore_tests { /** @@ -99,4 +100,15 @@ public function test_get_last_io_bytes(): void { ]); $this->assertEquals(21, $store->get_last_io_bytes()); } + + public function test_lock() { + $store = new \cachestore_file('Test'); + + $this->assertTrue($store->acquire_lock('lock', '123')); + $this->assertTrue($store->check_lock_state('lock', '123')); + $this->assertFalse($store->check_lock_state('lock', '321')); + $this->assertNull($store->check_lock_state('notalock', '123')); + $this->assertFalse($store->release_lock('lock', '321')); + $this->assertTrue($store->release_lock('lock', '123')); + } } diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index d64e921e8833a..f5b874236b7e3 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -2111,6 +2111,55 @@ public function test_application_locking() { $this->assertFalse($cache->has('a')); } + /** + * Test requiring a lock before attempting to set a key. + * + * @covers ::set_implementation + */ + public function test_application_locking_before_write() { + $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, + 'requirelockingbeforewrite' => true + )); + $cache = cache::make('phpunit', 'test_application_locking'); + $this->assertInstanceOf(cache_application::class, $cache); + + $cache->acquire_lock('a'); + $this->assertTrue($cache->set('a', 'A')); + $cache->release_lock('a'); + + $this->expectExceptionMessage('Attempted to set cache key "b" without a lock. ' + . 'Locking before writes is required for phpunit/test_application_locking'); + $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 the static cache_helper method purge_stores_used_by_definition. */ diff --git a/cache/upgrade.txt b/cache/upgrade.txt index b42024bff769d..079de19a94a6b 100644 --- a/cache/upgrade.txt +++ b/cache/upgrade.txt @@ -1,6 +1,14 @@ This files describes API changes in /cache/stores/* - cache store plugins. Information provided here is intended especially for developers. +=== 4.1 === +* Added new `requirelockingbeforewrite` option for cache definitions. This will check that a lock for a given cache key already + exists before it will perform a `set()` on that key. A `coding_exception` is thrown if the lock has not been acquired. +* Added native locking to cachestore_file. This will use an instance of file_lock_factory pointing at a subdirectory in the same + location as the cache instance, meaning a local file cache will have its locks stored locally. If file locks are disabled + globally, it will fall back to use the default lock factory, which may not be in the same location as the cache. cachestore_file + includes an additional setting to control how long it will wait for a lock before giving up, default is 60 seconds. + === 4.0 === * Cache stores may implement new optional function cache_store::get_last_io_bytes() to provide information about the size of data transferred (shown in footer if performance info enabled). diff --git a/lib/classes/lock/file_lock_factory.php b/lib/classes/lock/file_lock_factory.php index 87b93803571ee..ff60aeb10a089 100644 --- a/lib/classes/lock/file_lock_factory.php +++ b/lib/classes/lock/file_lock_factory.php @@ -58,12 +58,15 @@ class file_lock_factory implements lock_factory { * Create this lock factory. * * @param string $type - The type, e.g. cron, cache, session + * @param string|null $lockdirectory - Optional path to the lock directory, to override defaults. */ - public function __construct($type) { + public function __construct($type, ?string $lockdirectory = null) { global $CFG; $this->type = $type; - if (!isset($CFG->file_lock_root)) { + if (!is_null($lockdirectory)) { + $this->lockdirectory = $lockdirectory; + } else if (!isset($CFG->file_lock_root)) { $this->lockdirectory = $CFG->dataroot . '/lock'; } else { $this->lockdirectory = $CFG->file_lock_root; @@ -100,7 +103,7 @@ public function is_available() { global $CFG; $preventfilelocking = !empty($CFG->preventfilelocking); $lockdirisdataroot = true; - if (!empty($CFG->file_lock_root) && strpos($CFG->file_lock_root, $CFG->dataroot) !== 0) { + if (strpos($this->lockdirectory, $CFG->dataroot) !== 0) { $lockdirisdataroot = false; } return !$preventfilelocking || !$lockdirisdataroot; diff --git a/lib/classes/lock/timing_wrapper_lock_factory.php b/lib/classes/lock/timing_wrapper_lock_factory.php index 8563664e1d47d..0a8a6c155138b 100644 --- a/lib/classes/lock/timing_wrapper_lock_factory.php +++ b/lib/classes/lock/timing_wrapper_lock_factory.php @@ -70,40 +70,67 @@ public function get_real_factory(): lock_factory { * @return \core\lock\lock|boolean - An instance of \core\lock\lock if the lock was obtained, or false. */ public function get_lock($resource, $timeout, $maxlifetime = 86400) { - global $PERF; - $before = microtime(true); $result = $this->factory->get_lock($resource, $timeout, $maxlifetime); $after = microtime(true); + self::record_lock_data($after, $before, $this->type, $resource, (bool)$result, $result); + if ($result) { + $result->init_factory($this); + } + + return $result; + } + + /** + * Records statistics about a lock to the performance data. + * + * @param float $after The time after the lock was achieved. + * @param float $before The time before the lock was requested. + * @param string $type The type of lock. + * @param string $resource The resource being locked. + * @param bool $result Whether the lock was successful. + * @param lock|string $lock A value uniquely identifying the lock. + * @return void + */ + public static function record_lock_data(float $after, float $before, string $type, string $resource, bool $result, $lock) { + global $PERF; $duration = $after - $before; if (empty($PERF->locks)) { $PERF->locks = []; } $lockdata = (object) [ - 'type' => $this->type, + 'type' => $type, 'resource' => $resource, 'wait' => $duration, - 'success' => (bool)$result + 'success' => $result ]; if ($result) { - $lockdata->lock = $result; + $lockdata->lock = $lock; $lockdata->timestart = $after; - $result->init_factory($this); } $PERF->locks[] = $lockdata; - - return $result; } /** - * Release a lock that was previously obtained with @lock. + * Release a lock that was previously obtained with {@see get_lock}. * * @param lock $lock - The lock to release. * @return boolean - True if the lock is no longer held (including if it was never held). */ public function release_lock(lock $lock) { + self::record_lock_released_data($lock); + return $this->factory->release_lock($lock); + } + + /** + * Find the lock in the performance info and update it with the time held. + * + * @param lock|string $lock A value uniquely identifying the lock. + * @return void + */ + public static function record_lock_released_data($lock) { global $PERF; // Find this lock in the list of locks we got, looking backwards since it is probably @@ -117,8 +144,6 @@ public function release_lock(lock $lock) { break; } } - - return $this->factory->release_lock($lock); } /**