From 08ec1b4e47d73a1bd4cae3947e353477c3a8a9f2 Mon Sep 17 00:00:00 2001
From: sam marshall <s.marshall@open.ac.uk>
Date: Mon, 29 Jan 2018 17:16:06 +0000
Subject: [PATCH] MDL-61305 Performance: Modinfo cache can get built in
 parallel

In a busy site it is possible that one user will access the site
while another is building modinfo cache. This can result in the
cache being built several times at once, potentially causing a
performance issue.
---
 lib/modinfolib.php | 67 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/lib/modinfolib.php b/lib/modinfolib.php
index f7179b4f3a1..a0cedf2b680 100644
--- a/lib/modinfolib.php
+++ b/lib/modinfolib.php
@@ -54,6 +54,12 @@ if (!defined('MAX_MODINFO_CACHE_SIZE')) {
  *     Is an array of grouping id => array of group id => group id. Includes grouping id 0 for 'all groups'
  */
 class course_modinfo {
+    /** @var int Maximum time the course cache building lock can be held */
+    const COURSE_CACHE_LOCK_EXPIRY = 180;
+
+    /** @var int Time to wait for the course cache building lock before throwing an exception */
+    const COURSE_CACHE_LOCK_WAIT = 60;
+
     /**
      * List of fields from DB table 'course' that are cached in MUC and are always present in course_modinfo::$course
      * @var array
@@ -448,7 +454,17 @@ class course_modinfo {
         // Retrieve modinfo from cache. If not present or cacherev mismatches, call rebuild and retrieve again.
         $coursemodinfo = $cachecoursemodinfo->get($course->id);
         if ($coursemodinfo === false || ($course->cacherev != $coursemodinfo->cacherev)) {
-            $coursemodinfo = self::build_course_cache($course);
+            $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($course->id);
+                if ($coursemodinfo === false || ($course->cacherev != $coursemodinfo->cacherev)) {
+                    $coursemodinfo = self::inner_build_course_cache($course, $lock);
+                }
+            } finally {
+                $lock->release();
+            }
         }
 
         // Set initial values
@@ -577,6 +593,34 @@ class course_modinfo {
         return $compressedsections;
     }
 
+    /**
+     * Gets a lock for rebuilding the cache of a single course.
+     *
+     * Caller must release the returned lock.
+     *
+     * This is used to ensure that the cache rebuild doesn't happen multiple times in parallel.
+     * This function will wait up to 1 minute for the lock to be obtained. If the lock cannot
+     * be obtained, it throws an exception.
+     *
+     * @param int $courseid Course id
+     * @return \core\lock\lock Lock (must be released!)
+     * @throws moodle_exception If the lock cannot be obtained
+     */
+    protected static function get_course_cache_lock($courseid) {
+        // Get database lock to ensure this doesn't happen multiple times in parallel. Wait a
+        // reasonable time for the lock to be released, so we can give a suitable error message.
+        // In case the system crashes while building the course cache, the lock will automatically
+        // expire after a (slightly longer) period.
+        $lockfactory = \core\lock\lock_config::get_lock_factory('core_modinfo');
+        $lock = $lockfactory->get_lock('build_course_cache_' . $courseid,
+                self::COURSE_CACHE_LOCK_WAIT, self::COURSE_CACHE_LOCK_EXPIRY);
+        if (!$lock) {
+            throw new moodle_exception('locktimeout', '', '', null,
+                    'core_modinfo/build_course_cache_' . $courseid);
+        }
+        return $lock;
+    }
+
     /**
      * Builds and stores in MUC object containing information about course
      * modules and sections together with cached fields from table course.
@@ -589,11 +633,28 @@ class course_modinfo {
      *     necessary fields it is re-requested from database)
      */
     public static function build_course_cache($course) {
-        global $DB, $CFG;
-        require_once("$CFG->dirroot/course/lib.php");
         if (empty($course->id)) {
             throw new coding_exception('Object $course is missing required property \id\'');
         }
+
+        $lock = self::get_course_cache_lock($course->id);
+        try {
+            return self::inner_build_course_cache($course, $lock);
+        } finally {
+            $lock->release();
+        }
+    }
+
+    /**
+     * Called to build course cache when there is already a lock obtained.
+     *
+     * @param stdClass $course object from DB table course
+     * @param \core\lock\lock $lock Lock object - not actually used, just there to indicate you have a lock
+     * @return stdClass Course object that has been stored in MUC
+     */
+    protected static function inner_build_course_cache($course, \core\lock\lock $lock) {
+        global $DB;
+
         // Ensure object has all necessary fields.
         foreach (self::$cachedfields as $key) {
             if (!isset($course->$key)) {