L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-75736 lib: Course cache can be rebuilt unnecessarily

Browse Source 
This change puts back the behaviour that get_fast_modinfo will accept
a cached version of modinfo that is newer than expected (according to
the course cacherev).

Not accepting newer versions can lead to poor performance in cases
where multiple requests take place at once, or using a clustered
database.
This commit is contained in:
sam marshall
2022-10-11 16:56:21 +01:00
parent cc4fec275f
commit 18dc2a50ab
2 changed files with 50 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/modinfolib.php
+5 -2
View File
@@ -471,13 +471,16 @@ class course_modinfo {
// Retrieve modinfo from cache. If not present or cacherev mismatches, call rebuild and retrieve again.
$coursemodinfo = $cachecoursemodinfo->get_versioned($course->id, $course->cacherev);
if ($coursemodinfo === false || ($course->cacherev != $coursemodinfo->cacherev)) {
// Note the version comparison using the data in the cache should not be necessary, but the
// partial rebuild logic sometimes sets the $coursemodinfo->cacherev to -1 which is an
// indicator that it needs rebuilding.
if ($coursemodinfo === false || ($course->cacherev > $coursemodinfo->cacherev)) {
$lock = self::get_course_cache_lock($course->id);
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).
$coursemodinfo = $cachecoursemodinfo->get_versioned($course->id, $course->cacherev);
if ($coursemodinfo === false || ($course->cacherev != $coursemodinfo->cacherev)) {
if ($coursemodinfo === false || ($course->cacherev > $coursemodinfo->cacherev)) {
$coursemodinfo = self::inner_build_course_cache($course, $lock);
}
} finally {
lib/tests/modinfolib_test.php
+45
View File
@@ -1126,4 +1126,49 @@ class modinfolib_test extends advanced_testcase {
$this->expectExceptionMessage('Invalid course module id: ' . $forum1->cmid);
delete_course($course, false);
}
/**
* Tests that if the modinfo cache returns a newer-than-expected version, Moodle won't rebuild
* it.
*
* This is important to avoid wasted time/effort and poor performance, for example in cases
* where multiple requests are accessing the course.
*
* Certain cases could be particularly bad if this test fails. For example, if using clustered
* databases where there is a 100ms delay between updates to the course table being available
* to all users (but no such delay on the cache infrastructure), then during that 100ms, every
* request that calls get_fast_modinfo and uses the read-only database will rebuild the course
* cache. Since these will then create a still-newer version, future requests for the next
* 100ms will also rebuild it again... etc.
*
* @covers \course_modinfo
*/
public function test_get_modinfo_with_newer_version(): void {
global $DB;
$this->resetAfterTest();
// Get info about a course and build the initial cache, then drop it from memory.
$course = $this->getDataGenerator()->create_course();
get_fast_modinfo($course);
get_fast_modinfo(0, 0, true);
// User A starts a request, which takes some time...
$useracourse = $DB->get_record('course', ['id' => $course->id]);
// User B also starts a request and makes a change to the course.
$userbcourse = $DB->get_record('course', ['id' => $course->id]);
$this->getDataGenerator()->create_module('page', ['course' => $course->id]);
rebuild_course_cache($userbcourse->id, false);
// Finally, user A's request now gets modinfo. It should accept the version from B even
// though the course version (of cache) is newer than the one expected by A.
$before = $DB->perf_get_queries();
$modinfo = get_fast_modinfo($useracourse);
$after = $DB->perf_get_queries();
$this->assertEquals($after, $before, 'Should use cached version, making no DB queries');
// Obviously, modinfo should include the Page now.
$this->assertCount(1, $modinfo->get_instances_of('page'));
}
}