From 79d0e63b180a801784f2bb826803fc8fe56dfd5d Mon Sep 17 00:00:00 2001
From: sam marshall <s.marshall@open.ac.uk>
Date: Wed, 3 May 2023 10:47:01 +0100
Subject: [PATCH] MDL-78092 Cache: Modinfo locking with Redis store does not
 work

The feature added in 4.1 to lock the modinfo cache does not work when
using Redis, because:

* The API to acquire a cache lock is confusing, and the code did not
  check that it successfully acquired a lock before going on to build
  the cache anyway.

* Unlike the other types of cache lock, the Redis store did not retry
  the lock for a timeout period before giving up and failing.

This change fixes both points.
---
 cache/stores/redis/lib.php              | 51 +++++++++++++++++++++++-
 cache/stores/redis/tests/store_test.php | 52 +++++++++++++++++++++++++
 lib/modinfolib.php                      |  5 ++-
 3 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/cache/stores/redis/lib.php b/cache/stores/redis/lib.php
index 84b07cf02ae..a226d56c56e 100644
--- a/cache/stores/redis/lib.php
+++ b/cache/stores/redis/lib.php
@@ -119,6 +119,15 @@ class cachestore_redis extends cache_store implements cache_is_key_aware, cache_
      */
     protected $lastiobytes = 0;
 
+    /** @var int Maximum number of seconds to wait for a lock before giving up. */
+    protected $lockwait = 60;
+
+    /** @var int Timeout before lock is automatically released (in case of crashes) */
+    protected $locktimeout = 600;
+
+    /** @var ?array Array of current locks, or null if we haven't registered shutdown function */
+    protected $currentlocks = null;
+
     /**
      * Determines if the requirements for this type of store are met.
      *
@@ -182,6 +191,12 @@ class cachestore_redis extends cache_store implements cache_is_key_aware, cache_
         }
         $password = !empty($configuration['password']) ? $configuration['password'] : '';
         $prefix = !empty($configuration['prefix']) ? $configuration['prefix'] : '';
+        if (array_key_exists('lockwait', $configuration)) {
+            $this->lockwait = (int)$configuration['lockwait'];
+        }
+        if (array_key_exists('locktimeout', $configuration)) {
+            $this->locktimeout = (int)$configuration['locktimeout'];
+        }
         $this->redis = $this->new_redis($configuration['server'], $prefix, $password);
     }
 
@@ -527,7 +542,38 @@ class cachestore_redis extends cache_store implements cache_is_key_aware, cache_
      * @return bool True if the lock was acquired, false if it was not.
      */
     public function acquire_lock($key, $ownerid) {
-        return $this->redis->setnx($key, $ownerid);
+        $timelimit = time() + $this->lockwait;
+        do {
+            // If the key doesn't already exist, grab it and return true.
+            if ($this->redis->setnx($key, $ownerid)) {
+                // Ensure Redis deletes the key after a bit in case something goes wrong.
+                $this->redis->expire($key, $this->locktimeout);
+                // If we haven't got it already, better register a shutdown function.
+                if ($this->currentlocks === null) {
+                    core_shutdown_manager::register_function([$this, 'shutdown_release_locks']);
+                    $this->currentlocks = [];
+                }
+                $this->currentlocks[$key] = $ownerid;
+                return true;
+            }
+            // Wait 1 second then retry.
+            sleep(1);
+        } while (time() < $timelimit);
+        return false;
+    }
+
+    /**
+     * Releases any locks when the system shuts down, in case there is a crash or somebody forgets
+     * to use 'try-finally'.
+     *
+     * Do not call this function manually (except from unit test).
+     */
+    public function shutdown_release_locks() {
+        foreach ($this->currentlocks as $key => $ownerid) {
+            debugging('Automatically releasing Redis cache lock: ' . $key . ' (' . $ownerid .
+                    ') - did somebody forget to call release_lock()?', DEBUG_DEVELOPER);
+            $this->release_lock($key, $ownerid);
+        }
     }
 
     /**
@@ -541,7 +587,7 @@ class cachestore_redis extends cache_store implements cache_is_key_aware, cache_
      */
     public function check_lock_state($key, $ownerid) {
         $result = $this->redis->get($key);
-        if ($result === $ownerid) {
+        if ($result === (string)$ownerid) {
             return true;
         }
         if ($result === false) {
@@ -586,6 +632,7 @@ class cachestore_redis extends cache_store implements cache_is_key_aware, cache_
      */
     public function release_lock($key, $ownerid) {
         if ($this->check_lock_state($key, $ownerid)) {
+            unset($this->currentlocks[$key]);
             return ($this->redis->del($key) !== false);
         }
         return false;
diff --git a/cache/stores/redis/tests/store_test.php b/cache/stores/redis/tests/store_test.php
index dd44f6b796e..525848bf06b 100644
--- a/cache/stores/redis/tests/store_test.php
+++ b/cache/stores/redis/tests/store_test.php
@@ -141,6 +141,58 @@ class store_test extends \cachestore_tests {
         $this->assertTrue($store->release_lock('lock', '123'));
     }
 
+    /**
+     * Checks the timeout features of locking.
+     */
+    public function test_lock_timeouts(): void {
+        $store = $this->create_cachestore_redis(['lockwait' => 2, 'locktimeout' => 4]);
+
+        // User 123 acquires lock.
+        $this->assertTrue($store->acquire_lock('lock', '123'));
+        $this->assertTrue($store->check_lock_state('lock', '123'));
+
+        // User 456 tries to acquire lock - should fail after about 2 seconds.
+        $before = microtime(true);
+        $this->assertFalse($store->acquire_lock('lock', '456'));
+        $after = microtime(true);
+        $this->assertEqualsWithDelta(2, $after - $before, 0.5);
+
+        // Wait another 2 seconds and then it should be able to get the lock because of timeout.
+        sleep(2);
+        $this->assertTrue($store->acquire_lock('lock', '456'));
+        $this->assertTrue($store->check_lock_state('lock', '456'));
+
+        // The first user doesn't have the lock any more.
+        $this->assertFalse($store->check_lock_state('lock', '123'));
+
+        // Releasing the lock from the first user does nothing.
+        $this->assertFalse($store->release_lock('lock', '123'));
+        $this->assertTrue($store->check_lock_state('lock', '456'));
+
+        $this->assertTrue($store->release_lock('lock', '456'));
+    }
+
+    /**
+     * Tests the shutdown function that is supposed to free any remaining locks.
+     */
+    public function test_lock_shutdown(): void {
+        $store = $this->create_cachestore_redis();
+        try {
+            $this->assertTrue($store->acquire_lock('a', '123'));
+            $this->assertTrue($store->acquire_lock('b', '123'));
+            $this->assertTrue($store->acquire_lock('c', '123'));
+            $this->assertTrue($store->check_lock_state('a', '123'));
+            $this->assertTrue($store->check_lock_state('b', '123'));
+            $this->assertTrue($store->check_lock_state('c', '123'));
+        } finally {
+            $store->shutdown_release_locks();
+            $this->assertDebuggingCalledCount(3);
+        }
+        $this->assertNull($store->check_lock_state('a', '123'));
+        $this->assertNull($store->check_lock_state('b', '123'));
+        $this->assertNull($store->check_lock_state('c', '123'));
+    }
+
     /**
      * Tests the get_last_io_bytes function when not using compression (just returns unknown).
      */
diff --git a/lib/modinfolib.php b/lib/modinfolib.php
index 9d85fc872bb..d64bc4dfbce 100644
--- a/lib/modinfolib.php
+++ b/lib/modinfolib.php
@@ -640,7 +640,10 @@ class course_modinfo {
 
         $cachecoursemodinfo = cache::make('core', 'coursemodinfo');
         $cachekey = $course->id;
-        $cachecoursemodinfo->acquire_lock($cachekey);
+        if (!$cachecoursemodinfo->acquire_lock($cachekey)) {
+            throw new moodle_exception('ex_unabletolock', 'cache', '', null,
+                'Unable to lock modinfo cache for course ' . $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).