mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-04-17 22:45:54 +02:00
Code Issues Projects Releases Wiki Activity

MDL-78467 core_cache: Improve cache locking API

Browse Source 
* 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...)
This commit is contained in:
sam marshall 2023-05-03 10:47:08 +01:00
parent 9b2c445143
commit 0f8577784d
9 changed files with 448 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
cache/classes/interfaces.php vendored
View File

@ -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);

148
cache/classes/loaders.php vendored
View File

@ -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);
}
}
/**

197
cache/tests/cache_test.php vendored
View File

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

12
cache/tests/fixtures/lib.php vendored
View File

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

4
cache/upgrade.txt vendored
View File

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

105
lib/modinfolib.php
View File

@ -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;
}
}
}

3
lib/moodlelib.php
View File

@ -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;

63
lib/tests/modinfolib_test.php
View File

@ -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;

4
lib/upgrade.txt
View File

@ -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 ===