From 0f8577784db3f73b3f496f0821e6871dfb4d3040 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 3 May 2023 10:47:08 +0100 Subject: [PATCH] MDL-78467 core_cache: Improve cache locking API * Makes it possible to safely call acquire_lock so that it throws an exception instead of returning false if it can't get a lock (which most existing uses assumed it already does). * Fix some omissions from the requirelockingbeforewrite option (it now checks on delete). * Modinfo uses a versioned cache, so it is not necessary to delete items, only increase the version. (Provided we keep track of cacherev carefully...) --- cache/classes/interfaces.php | 6 +- cache/classes/loaders.php | 148 +++++++++++++++++-------- cache/tests/cache_test.php | 197 ++++++++++++++++++++++++++++++++-- cache/tests/fixtures/lib.php | 12 +++ cache/upgrade.txt | 4 + lib/modinfolib.php | 105 +++++++++++------- lib/moodlelib.php | 3 +- lib/tests/modinfolib_test.php | 63 +++++++++++ lib/upgrade.txt | 4 +- 9 files changed, 448 insertions(+), 94 deletions(-) diff --git a/cache/classes/interfaces.php b/cache/classes/interfaces.php index 543041f3989..e0dff287f64 100644 --- a/cache/classes/interfaces.php +++ b/cache/classes/interfaces.php @@ -248,8 +248,12 @@ interface cache_loader_with_locking { * However this doesn't guarantee consistent access. It will become the responsibility of the calling code to ensure * locks are acquired, checked, and released. * + * Prior to Moodle 4,3 this function used to return false if the lock cannot be obtained. It + * now always returns true, and throws an exception if the lock cannot be obtained. + * * @param string|int $key - * @return bool True if the lock could be acquired, false otherwise. + * @return bool Always returns true (for backwards compatibility) + * @throws moodle_exception If the lock cannot be obtained after a timeout */ public function acquire_lock($key); diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 765ce0f3d91..36fdb4da40e 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -620,18 +620,22 @@ class cache implements cache_loader { // 6. Set it to the store if we got it from the loader/datasource. Only set to this direct // store; parent method will have set it to all stores if needed. if ($setaftervalidation) { - $lock = null; - // Only try to acquire a lock for this cache if we do not already have one. - if (!empty($this->requirelockingbeforewrite) && !$this->check_lock_state($key)) { - $lock = $this->acquire_lock($key); - } - if ($requiredversion === self::VERSION_NONE) { - $this->set_implementation($key, self::VERSION_NONE, $result, false); - } else { - $this->set_implementation($key, $actualversion, $result, false); - } - if ($lock) { - $this->release_lock($key); + $lock = false; + try { + // Only try to acquire a lock for this cache if we do not already have one. + if (!empty($this->requirelockingbeforewrite) && !$this->check_lock_state($key)) { + $this->acquire_lock($key); + $lock = true; + } + if ($requiredversion === self::VERSION_NONE) { + $this->set_implementation($key, self::VERSION_NONE, $result, false); + } else { + $this->set_implementation($key, $actualversion, $result, false); + } + } finally { + if ($lock) { + $this->release_lock($key); + } } } // 7. Make sure we don't pass back anything that could be a reference. @@ -740,15 +744,19 @@ class cache implements cache_loader { } foreach ($resultmissing as $key => $value) { $result[$keysparsed[$key]] = $value; - $lock = null; - if (!empty($this->requirelockingbeforewrite)) { - $lock = $this->acquire_lock($key); - } - if ($value !== false) { - $this->set($key, $value); - } - if ($lock) { - $this->release_lock($key); + $lock = false; + try { + if (!empty($this->requirelockingbeforewrite)) { + $this->acquire_lock($key); + $lock = true; + } + if ($value !== false) { + $this->set($key, $value); + } + } finally { + if ($lock) { + $this->release_lock($key); + } } } unset($resultmissing); @@ -1672,30 +1680,44 @@ class cache_application extends cache implements cache_loader_with_locking { * rely on the integrators review skills. * * @param string|int $key The key as given to get|set|delete - * @return bool Returns true if the lock could be acquired, false otherwise. + * @return bool Always returns true + * @throws moodle_exception If the lock cannot be obtained */ public function acquire_lock($key) { - global $CFG; - if ($this->get_loader() !== false) { - $this->get_loader()->acquire_lock($key); - } - $key = cache_helper::hash_key($key, $this->get_definition()); - $before = microtime(true); - if ($this->nativelocking) { - $lock = $this->get_store()->acquire_lock($key, $this->get_identifier()); - } else { - $this->ensure_cachelock_available(); - $lock = $this->cachelockinstance->lock($key, $this->get_identifier()); - } - $after = microtime(true); - if ($lock) { - $this->locks[$key] = $lock; - if (MDL_PERF || $this->perfdebug) { - \core\lock\timing_wrapper_lock_factory::record_lock_data($after, $before, - $this->get_definition()->get_id(), $key, $lock, $this->get_identifier() . $key); + $releaseparent = false; + try { + if ($this->get_loader() !== false) { + $this->get_loader()->acquire_lock($key); + // We need to release this lock later if the lock is not successful. + $releaseparent = true; + } + $hashedkey = cache_helper::hash_key($key, $this->get_definition()); + $before = microtime(true); + if ($this->nativelocking) { + $lock = $this->get_store()->acquire_lock($hashedkey, $this->get_identifier()); + } else { + $this->ensure_cachelock_available(); + $lock = $this->cachelockinstance->lock($hashedkey, $this->get_identifier()); + } + $after = microtime(true); + if ($lock) { + $this->locks[$hashedkey] = $lock; + if (MDL_PERF || $this->perfdebug) { + \core\lock\timing_wrapper_lock_factory::record_lock_data($after, $before, + $this->get_definition()->get_id(), $hashedkey, $lock, $this->get_identifier() . $hashedkey); + } + $releaseparent = false; + return true; + } else { + throw new moodle_exception('ex_unabletolock', 'cache', '', null, + 'store: ' . get_class($this->get_store()) . ', lock: ' . $hashedkey); + } + } finally { + // Release the parent lock if we acquired it, then threw an exception. + if ($releaseparent) { + $this->get_loader()->release_lock($key); } } - return $lock; } /** @@ -1804,19 +1826,57 @@ class cache_application extends cache implements cache_loader_with_locking { * @param array $keyvaluearray An array of key => value pairs to send to the cache. * @return int The number of items successfully set. It is up to the developer to check this matches the number of items. * ... if they care that is. + * @throws coding_exception If a required lock has not beeen acquired */ public function set_many(array $keyvaluearray) { if ($this->requirelockingbeforewrite) { foreach ($keyvaluearray as $key => $value) { if (!$this->check_lock_state($key)) { - debugging('Attempted to set cache key "' . $key . '" without a lock. ' - . 'Locking before writes is required for ' . $this->get_definition()->get_name(), DEBUG_DEVELOPER); - unset($keyvaluearray[$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()); } } } return parent::set_many($keyvaluearray); } + + /** + * 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. + * @throws coding_exception If a required lock has not beeen acquired + */ + public function delete($key, $recurse = true) { + if ($this->requirelockingbeforewrite && !$this->check_lock_state($key)) { + throw new coding_exception('Attempted to delete cache key "' . $key . '" without a lock. ' + . 'Locking before writes is required for ' . $this->get_definition()->get_id()); + } + return parent::delete($key, $recurse); + } + + /** + * 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. + * @throws coding_exception If a required lock has not beeen acquired + */ + public function delete_many(array $keys, $recurse = true) { + if ($this->requirelockingbeforewrite) { + foreach ($keys as $key) { + if (!$this->check_lock_state($key)) { + throw new coding_exception('Attempted to delete cache key "' . $key . '" without a lock. ' + . 'Locking before writes is required for ' . $this->get_definition()->get_id()); + } + } + } + return parent::delete_many($keys, $recurse); + } } /** diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index dd29c64f409..ac6df7fe274 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -883,14 +883,23 @@ class cache_test extends \advanced_testcase { 'component' => 'phpunit', 'area' => 'lockingtest' )); + // Configure the lock timeout so the test doesn't take too long to run. + $instance->phpunit_edit_store_config('default_application', ['lockwait' => 2]); $cache1 = cache::make('phpunit', 'lockingtest'); $cache2 = clone($cache1); $this->assertTrue($cache1->set('testkey', 'test data')); $this->assertTrue($cache2->set('testkey', 'test data')); - $this->assertTrue($cache1->acquire_lock('testkey')); - $this->assertFalse($cache2->acquire_lock('testkey')); + $cache1->acquire_lock('testkey'); + try { + $cache2->acquire_lock('testkey'); + $this->fail(); + } catch (\moodle_exception $e) { + // Check the right exception message, and debug info mentions the store type. + $this->assertMatchesRegularExpression('~Unable to acquire a lock.*cachestore_file.*~', + $e->getMessage()); + } $this->assertTrue($cache1->check_lock_state('testkey')); $this->assertFalse($cache2->check_lock_state('testkey')); @@ -2138,12 +2147,83 @@ class cache_test extends \advanced_testcase { $this->assertInstanceOf(cache_application::class, $cache); $cache->acquire_lock('a'); - $this->assertTrue($cache->set('a', 'A')); - $cache->release_lock('a'); + try { + // Set with lock. + $this->assertTrue($cache->set('a', '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')); + // Set without lock. + try { + $cache->set('b', 'B'); + $this->fail(); + } catch (\coding_exception $e) { + $this->assertStringContainsString( + 'Attempted to set cache key "b" without a lock. ' . + 'Locking before writes is required for phpunit/test_application_locking', + $e->getMessage()); + } + + // Set many without full lock. + try { + $cache->set_many(['a' => 'AA', 'b' => 'BB']); + $this->fail(); + } catch (\coding_exception $e) { + $this->assertStringContainsString( + 'Attempted to set cache key "b" without a lock.', + $e->getMessage()); + } + + // Check it didn't set key a either. + $this->assertEquals('A', $cache->get('a')); + + // Set many with full lock. + $cache->acquire_lock('b'); + try { + $this->assertEquals(2, $cache->set_many(['a' => 'AA', 'b' => 'BB'])); + $this->assertEquals('AA', $cache->get('a')); + $this->assertEquals('BB', $cache->get('b')); + } finally { + $cache->release_lock('b'); + } + + // Delete key with lock. + $this->assertTrue($cache->delete('a')); + $this->assertFalse($cache->get('a')); + + // Delete key without lock. + try { + $cache->delete('b'); + $this->fail(); + } catch (\coding_exception $e) { + $this->assertStringContainsString( + 'Attempted to delete cache key "b" without a lock.', + $e->getMessage()); + } + + // Delete many without full lock. + $cache->set('a', 'AAA'); + try { + $cache->delete_many(['a', 'b']); + $this->fail(); + } catch (\coding_exception $e) { + $this->assertStringContainsString( + 'Attempted to delete cache key "b" without a lock.', + $e->getMessage()); + } + // Nothing was deleted. + $this->assertEquals('AAA', $cache->get('a')); + + // Delete many with full lock. + $cache->acquire_lock('b'); + try { + $this->assertEquals(2, $cache->delete_many(['a', 'b'])); + } finally { + $cache->release_lock('b'); + } + $this->assertFalse($cache->get('a')); + $this->assertFalse($cache->get('b')); + } finally { + $cache->release_lock('a'); + } } /** @@ -2174,10 +2254,10 @@ class cache_test extends \advanced_testcase { // Check that we can set a key across multiple layers. $cache->acquire_lock('a'); $this->assertTrue($cache->set('a', 'A')); - $cache->release_lock('a'); // Delete from the current layer. $cache->delete('a', false); + $cache->release_lock('a'); // Check that we can get the value from the deeper layer, which will also re-set it in the current one. $this->assertEquals('A', $cache->get('a')); @@ -2187,11 +2267,11 @@ class cache_test extends \advanced_testcase { $cache->acquire_lock('y'); $cache->acquire_lock('z'); $this->assertEquals(3, $cache->set_many(['x' => 'X', 'y' => 'Y', 'z' => 'Z'])); + $cache->delete_many(['x', 'y', 'z'], false); $cache->release_lock('x'); $cache->release_lock('y'); $cache->release_lock('z'); - $cache->delete_many(['x', 'y', 'z'], false); $this->assertEquals(['x' => 'X', 'y' => 'Y', 'z' => 'Z'], $cache->get_many(['x', 'y', 'z'])); $cache->purge(); @@ -2205,10 +2285,10 @@ class cache_test extends \advanced_testcase { // Check that we can set a key across multiple layers. $cache->acquire_lock('a'); $this->assertTrue($cache->set('a', 'A')); - $cache->release_lock('a'); // Delete from the current layer. $cache->delete('a', false); + $cache->release_lock('a'); // Check that we can get the value from the deeper layer, which will also re-set it in the current one. $this->assertEquals('A', $cache->get('a')); @@ -2218,14 +2298,109 @@ class cache_test extends \advanced_testcase { $cache->acquire_lock('y'); $cache->acquire_lock('z'); $this->assertEquals(3, $cache->set_many(['x' => 'X', 'y' => 'Y', 'z' => 'Z'])); + $cache->delete_many(['x', 'y', 'z'], false); $cache->release_lock('x'); $cache->release_lock('y'); $cache->release_lock('z'); - $cache->delete_many(['x', 'y', 'z'], false); $this->assertEquals(['x' => 'X', 'y' => 'Y', 'z' => 'Z'], $cache->get_many(['x', 'y', 'z'])); } + /** + * Tests that locking fails correctly when either layer of a 2-layer cache has a lock already. + * + * @covers \cache_loader + */ + public function test_application_locking_multiple_layers_failures(): void { + + $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 + ), false); + $instance->phpunit_add_file_store('phpunittest1'); + $instance->phpunit_add_file_store('phpunittest2'); + $instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest1', 1); + $instance->phpunit_add_definition_mapping('phpunit/test_application_locking', 'phpunittest2', 2); + + $cache = cache::make('phpunit', 'test_application_locking'); + + // We need to get the individual stores so as to set up the right behaviour here. + $ref = new \ReflectionClass('\cache'); + $definitionprop = $ref->getProperty('definition'); + $definitionprop->setAccessible(true); + $storeprop = $ref->getProperty('store'); + $storeprop->setAccessible(true); + $loaderprop = $ref->getProperty('loader'); + $loaderprop->setAccessible(true); + + $definition = $definitionprop->getValue($cache); + $localstore = $storeprop->getValue($cache); + $sharedcache = $loaderprop->getValue($cache); + $sharedstore = $storeprop->getValue($sharedcache); + + // Set the lock waiting time to 1 second so it doesn't take forever to run the test. + $ref = new \ReflectionClass('\cachestore_file'); + $lockwaitprop = $ref->getProperty('lockwait'); + $lockwaitprop->setAccessible(true); + + $lockwaitprop->setValue($localstore, 1); + $lockwaitprop->setValue($sharedstore, 1); + + // Get key details and the cache identifier. + $hashedkey = cache_helper::hash_key('apple', $definition); + $localidentifier = $cache->get_identifier(); + $sharedidentifier = $sharedcache->get_identifier(); + + // 1. Local cache is not locked but parent cache is locked. + $sharedstore->acquire_lock($hashedkey, 'somebodyelse'); + try { + try { + $cache->acquire_lock('apple'); + $this->fail(); + } catch (\moodle_exception $e) { + } + + // Neither store is locked by us, shared store still locked. + $this->assertFalse((bool)$localstore->check_lock_state($hashedkey, $localidentifier)); + $this->assertFalse((bool)$sharedstore->check_lock_state($hashedkey, $sharedidentifier)); + $this->assertTrue((bool)$sharedstore->check_lock_state($hashedkey, 'somebodyelse')); + } finally { + $sharedstore->release_lock($hashedkey, 'somebodyelse'); + } + + // 2. Local cache is locked, parent cache is not locked. + $localstore->acquire_lock($hashedkey, 'somebodyelse'); + try { + try { + $cache->acquire_lock('apple'); + $this->fail(); + } catch (\moodle_exception $e) { + + } + + // Neither store is locked by us, local store still locked. + $this->assertFalse((bool)$localstore->check_lock_state($hashedkey, $localidentifier)); + $this->assertFalse((bool)$sharedstore->check_lock_state($hashedkey, $sharedidentifier)); + $this->assertTrue((bool)$localstore->check_lock_state($hashedkey, 'somebodyelse')); + } finally { + $localstore->release_lock($hashedkey, 'somebodyelse'); + } + + // 3. Just for completion, test what happens if we do lock it. + $this->assertTrue($cache->acquire_lock('apple')); + try { + $this->assertTrue((bool)$localstore->check_lock_state($hashedkey, $localidentifier)); + $this->assertTrue((bool)$sharedstore->check_lock_state($hashedkey, $sharedidentifier)); + } finally { + $cache->release_lock('apple'); + } + } + /** * Test the static cache_helper method purge_stores_used_by_definition. */ diff --git a/cache/tests/fixtures/lib.php b/cache/tests/fixtures/lib.php index 37ae3731b99..92d3f2e4a0d 100644 --- a/cache/tests/fixtures/lib.php +++ b/cache/tests/fixtures/lib.php @@ -249,6 +249,18 @@ class cache_config_testing extends cache_config_writer { ); } + /** + * Hacks the in-memory configuration for a store. + * + * @param string $store Name of store to edit e.g. 'default_application' + * @param array $configchanges List of config changes + */ + public function phpunit_edit_store_config(string $store, array $configchanges): void { + foreach ($configchanges as $name => $value) { + $this->configstores[$store]['configuration'][$name] = $value; + } + } + /** * Forcefully adds a session store. * diff --git a/cache/upgrade.txt b/cache/upgrade.txt index 32f06aa7057..489e1eb4ef8 100644 --- a/cache/upgrade.txt +++ b/cache/upgrade.txt @@ -11,6 +11,10 @@ Information provided here is intended especially for developers. 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. +* The acquire_lock function within cache loaders now throws an exception if it fails to obtain the + lock after a timeout, instead of returning false. +* The functions set_many, delete, and delete_many now all throw exceptions if you are using + 'requirelockingbeforewrite' and do not have a lock, same as the set function. === 4.2 === * The memcached cachestore has been removed. diff --git a/lib/modinfolib.php b/lib/modinfolib.php index 834c381668c..00ff61aeab1 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -368,18 +368,34 @@ class course_modinfo { */ protected static $cacheaccessed = array(); + /** + * Store a list of known course cacherev values. This is in case people reuse a course object + * (with an old cacherev value) within the same request when calling things like + * get_fast_modinfo, after rebuild_course_cache. + * + * @var int[] + */ + protected static $mincacherevs = []; + /** * Clears the cache used in course_modinfo::instance() * * Used in {@link get_fast_modinfo()} when called with argument $reset = true * and in {@link rebuild_course_cache()} * + * If the cacherev for the course is known to have updated (i.e. when doing + * rebuild_course_cache), it should be specified here. + * * @param null|int|stdClass $courseorid if specified removes only cached value for this course + * @param int $newcacherev If specified, the known cache rev for this course id will be updated */ - public static function clear_instance_cache($courseorid = null) { + public static function clear_instance_cache($courseorid = null, int $newcacherev = 0) { if (empty($courseorid)) { self::$instancecache = array(); self::$cacheaccessed = array(); + // This is called e.g. in phpunit when we just want to reset the caches, so also + // reset the mincacherevs static cache. + self::$mincacherevs = []; return; } if (is_object($courseorid)) { @@ -393,6 +409,11 @@ class course_modinfo { unset(self::$instancecache[$courseorid]); unset(self::$cacheaccessed[$courseorid]); } + // When clearing cache for a course, we record the new cacherev version, to make + // sure that any future requests for the cache use at least this version. + if ($newcacherev) { + self::$mincacherevs[(int)$courseorid] = $newcacherev; + } } /** @@ -468,6 +489,14 @@ class course_modinfo { $course = get_course($course->id, false); } + // If we have rebuilt the course cache in this request, ensure that requested cacherev is + // at least that value. This ensures that we're not reusing a course object with old + // cacherev, which could result in using old cached data. + if (array_key_exists($course->id, self::$mincacherevs) && + $course->cacherev < self::$mincacherevs[$course->id]) { + $course->cacherev = self::$mincacherevs[$course->id]; + } + $cachecoursemodinfo = cache::make('core', 'coursemodinfo'); // Retrieve modinfo from cache. If not present or cacherev mismatches, call rebuild and retrieve again. @@ -641,10 +670,7 @@ class course_modinfo { $cachecoursemodinfo = cache::make('core', 'coursemodinfo'); $cachekey = $course->id; - if (!$cachecoursemodinfo->acquire_lock($cachekey)) { - throw new moodle_exception('ex_unabletolock', 'cache', '', null, - 'Unable to lock modinfo cache for course ' . $cachekey); - } + $cachecoursemodinfo->acquire_lock($cachekey); try { // Only actually do the build if it's still needed after getting the lock (not if // somebody else, who might have been holding the lock, built it already). @@ -702,18 +728,21 @@ class course_modinfo { $cache = cache::make('core', 'coursemodinfo'); $cachekey = $course->id; $cache->acquire_lock($cachekey); - $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); - if ($coursemodinfo !== false) { - foreach ($coursemodinfo->sectioncache as $sectionno => $sectioncache) { - if ($sectioncache->id == $sectionid) { - $coursemodinfo->cacherev = -1; - unset($coursemodinfo->sectioncache[$sectionno]); - $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); - break; + try { + $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); + if ($coursemodinfo !== false) { + foreach ($coursemodinfo->sectioncache as $sectionno => $sectioncache) { + if ($sectioncache->id == $sectionid) { + $coursemodinfo->cacherev = -1; + unset($coursemodinfo->sectioncache[$sectionno]); + $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + break; + } } } + } finally { + $cache->release_lock($cachekey); } - $cache->release_lock($cachekey); } /** @@ -727,13 +756,16 @@ class course_modinfo { $cache = cache::make('core', 'coursemodinfo'); $cachekey = $course->id; $cache->acquire_lock($cachekey); - $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); - if ($coursemodinfo !== false && array_key_exists($sectionno, $coursemodinfo->sectioncache)) { - $coursemodinfo->cacherev = -1; - unset($coursemodinfo->sectioncache[$sectionno]); - $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + try { + $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); + if ($coursemodinfo !== false && array_key_exists($sectionno, $coursemodinfo->sectioncache)) { + $coursemodinfo->cacherev = -1; + unset($coursemodinfo->sectioncache[$sectionno]); + $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + } + } finally { + $cache->release_lock($cachekey); } - $cache->release_lock($cachekey); } /** @@ -747,15 +779,18 @@ class course_modinfo { $cache = cache::make('core', 'coursemodinfo'); $cachekey = $course->id; $cache->acquire_lock($cachekey); - $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); - $hascache = ($coursemodinfo !== false) && array_key_exists($cmid, $coursemodinfo->modinfo); - if ($hascache) { - $coursemodinfo->cacherev = -1; - unset($coursemodinfo->modinfo[$cmid]); - $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + try { $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); + $hascache = ($coursemodinfo !== false) && array_key_exists($cmid, $coursemodinfo->modinfo); + if ($hascache) { + $coursemodinfo->cacherev = -1; + unset($coursemodinfo->modinfo[$cmid]); + $cache->set_versioned($cachekey, $course->cacherev, $coursemodinfo); + $coursemodinfo = $cache->get_versioned($cachekey, $course->cacherev); + } + } finally { + $cache->release_lock($cachekey); } - $cache->release_lock($cachekey); } /** @@ -950,8 +985,8 @@ class course_modinfo { */ public static function purge_course_cache(int $courseid): void { increment_revision_number('course', 'cacherev', 'id = :id', array('id' => $courseid)); - $cachemodinfo = cache::make('core', 'coursemodinfo'); - $cachemodinfo->delete($courseid); + // Because this is a versioned cache, there is no need to actually delete the cache item, + // only increase the required version number. } } @@ -2801,21 +2836,19 @@ function rebuild_course_cache(int $courseid = 0, bool $clearonly = false, bool $ } } else { // Clearing cache for one course, make sure it is deleted from user request cache as well. + // Because this is a versioned cache, there is no need to actually delete the cache item, + // only increase the required version number. increment_revision_number('course', 'cacherev', 'id = :id', array('id' => $courseid)); - if (!$partialrebuild) { - // Purge all course modinfo. - $cachecoursemodinfo->delete($courseid); - } + $cacherev = $DB->get_field('course', 'cacherev', ['id' => $courseid]); // Clear memory static cache. - course_modinfo::clear_instance_cache($courseid); + course_modinfo::clear_instance_cache($courseid, $cacherev); // Update global values too. if ($courseid == $COURSE->id || $courseid == $SITE->id) { - $cacherev = $DB->get_field('course', 'cacherev', array('id' => $courseid)); if ($courseid == $COURSE->id) { $COURSE->cacherev = $cacherev; } if ($courseid == $SITE->id) { - $SITE->cachrev = $cacherev; + $SITE->cacherev = $cacherev; } } } diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 2de6b34aa09..fcf7a72d8e9 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -5444,10 +5444,11 @@ function remove_course_contents($courseid, $showfeedback = true, array $options $fs->delete_area_files($coursecontext->id, 'backup'); // Cleanup course record - remove links to deleted stuff. + // Do not wipe cacherev, as this course might be reused and we need to ensure that it keeps + // increasing. $oldcourse = new stdClass(); $oldcourse->id = $course->id; $oldcourse->summary = ''; - $oldcourse->cacherev = 0; $oldcourse->legacyfiles = 0; if (!empty($options['keep_groups_and_groupings'])) { $oldcourse->defaultgroupingid = 0; diff --git a/lib/tests/modinfolib_test.php b/lib/tests/modinfolib_test.php index a0c20cc5c56..4e87ebbb567 100644 --- a/lib/tests/modinfolib_test.php +++ b/lib/tests/modinfolib_test.php @@ -334,6 +334,69 @@ class modinfolib_test extends advanced_testcase { $this->assertEmpty($cache->get($course->id)); } + /** + * The cacherev is updated when we rebuild course cache, but there are scenarios where an + * existing course object with old cacherev might be reused within the same request after + * clearing the cache. In that case, we need to check that the new data is loaded and it + * does not reuse the old cached data with old cacherev. + * + * @covers ::rebuild_course_cache() + */ + public function test_cache_clear_wrong_cacherev(): void { + global $DB; + + $this->resetAfterTest(); + $originalcourse = $this->getDataGenerator()->create_course(); + $course = $DB->get_record('course', ['id' => $originalcourse->id]); + $page = $this->getDataGenerator()->create_module('page', + ['course' => $course->id, 'name' => 'frog']); + $oldmodinfo = get_fast_modinfo($course); + $this->assertEquals('frog', $oldmodinfo->get_cm($page->cmid)->name); + + // Change page name and rebuild cache. + $DB->set_field('page', 'name', 'Frog', ['id' => $page->id]); + rebuild_course_cache($course->id, true); + + // Get modinfo using original course object which has old cacherev. + $newmodinfo = get_fast_modinfo($course); + $this->assertEquals('Frog', $newmodinfo->get_cm($page->cmid)->name); + } + + /** + * When cacherev is updated for a course, it is supposed to update in the $COURSE and $SITE + * globals automatically. Check this is working. + * + * @covers ::rebuild_course_cache() + */ + public function test_cacherev_update_in_globals(): void { + global $DB, $COURSE, $SITE; + + $this->resetAfterTest(); + + // Create a course and get modinfo. + $originalcourse = $this->getDataGenerator()->create_course(); + $oldmodinfo = get_fast_modinfo($originalcourse->id); + + // Store (two clones of) the course in COURSE and SITE globals. + $COURSE = get_course($originalcourse->id); + $SITE = get_course($originalcourse->id); + + // Note original cacherev. + $originalcacherev = $oldmodinfo->get_course()->cacherev; + $this->assertEquals($COURSE->cacherev, $originalcacherev); + $this->assertEquals($SITE->cacherev, $originalcacherev); + + // Clear the cache and check cacherev updated. + rebuild_course_cache($originalcourse->id, true); + + $newcourse = $DB->get_record('course', ['id' => $originalcourse->id]); + $this->assertGreaterThan($originalcacherev, $newcourse->cacherev); + + // Check that the in-memory $COURSE and $SITE have updated. + $this->assertEquals($newcourse->cacherev, $COURSE->cacherev); + $this->assertEquals($newcourse->cacherev, $SITE->cacherev); + } + public function test_course_modinfo_properties() { global $USER, $DB; diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 96daf7fce74..f7b26e5a87b 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -167,8 +167,10 @@ being forced open in all behat tests. the grade item once the recalculations are completed. (This was fixed in 4.3, 4.2.2) * Added a new constant called MAX_PASSWORD_CHARACTERS in moodlelib.php to hold a length of accepted password. * Added a new method called exceeds_password_length in moodlelib.php to validate the password length. -* The core/modal_factory has been deprecated. From Moodle 4.3 onwards please instantiate new modals using the ModalType.create method instead. +* The core/modal_factory has been deprecated. From Moodle 4.3 onwards pleáse instantiate new modals using the ModalType.create method instead. Please note that this method does not support the `trigger` option. +* Function course_modinfo::clear_instance_cache now has an extra optional parameter $newcacherev, although this is really + intended only for use by rebuild_course_cache. === 4.2 ===