From 2e8bd84403acaa3d26c18680652a54f863d72397 Mon Sep 17 00:00:00 2001
From: Frederik Milling Pytlick <fmp@praxis.dk>
Date: Mon, 5 Feb 2024 10:18:15 +0100
Subject: [PATCH] MDL-80815 cachestore_redis: Simplified acquire_lock method

Simplified the acquire_lock method in the redis cache store, so that it
sets the key with an expiry in one go. Before, it would set it, then
update it afterwards with an expiry.

This also makes test_lock_timeouts() more robust.

Co-authored-by: Daniel Ziegenberg <daniel@ziegenberg.at>
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
---
 cache/stores/redis/lib.php              | 51 +++++++++++++++++--------
 cache/stores/redis/tests/store_test.php |  6 +--
 2 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/cache/stores/redis/lib.php b/cache/stores/redis/lib.php
index 170f04fc6b9..769303ffd0a 100644
--- a/cache/stores/redis/lib.php
+++ b/cache/stores/redis/lib.php
@@ -20,6 +20,8 @@ use core_cache\key_aware_cache_interface;
 use core_cache\lockable_cache_interface;
 use core_cache\searchable_cache_interface;
 use core_cache\store;
+use core\clock;
+use core\di;
 
 /**
  * Redis Cache Store
@@ -141,6 +143,9 @@ class cachestore_redis extends store implements
     /** @var ?array Array of current locks, or null if we haven't registered shutdown function */
     protected $currentlocks = null;
 
+    /** @var clock */
+    private readonly clock $clock;
+
     /**
      * Determines if the requirements for this type of store are met.
      *
@@ -215,6 +220,7 @@ class cachestore_redis extends store implements
             $this->locktimeout = (int)$configuration['locktimeout'];
         }
         $this->redis = $this->new_redis($configuration);
+        $this->clock = di::get(clock::class);
     }
 
     /**
@@ -659,24 +665,39 @@ class cachestore_redis extends store implements
      * @return bool True if the lock was acquired, false if it was not.
      */
     public function acquire_lock($key, $ownerid) {
-        $clock = \core\di::get(\core\clock::class);
-        $timelimit = $clock->time() + $this->lockwait;
+        $timelimit = $this->clock->time() + $this->lockwait;
+        $startlocktime = $this->clock->time();
+
         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 = [];
+            // Lock already exists, wait 1 second then retry.
+            $haslock = $this->redis->set($key, $ownerid, ['nx', 'ex' => $this->locktimeout]);
+            if (!$haslock) {
+                if ($this->clock->time() < $startlocktime + 5) {
+                    // We want a random delay to stagger the polling load. Ideally, this delay should be a fraction
+                    // of the average response time. If it is too small we will poll too much and if it is too
+                    // large we will waste time waiting for no reason. 100ms is the default starting point.
+                    $delay = rand(100, 110);
+                } else {
+                    // If we don't get a lock within 5 seconds then there must be a very long-lived process holding the lock
+                    // so throttle back to just polling roughly once a second.
+                    $delay = rand(1000, 1100);
                 }
-                $this->currentlocks[$key] = $ownerid;
-                return true;
+
+                usleep($delay * 1000);
+                continue;
             }
-            // Wait 1 second then retry.
-            sleep(1);
-        } while ($clock->time() < $timelimit);
+
+            // 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;
+        } while ($this->clock->time() < $timelimit);
+
         return false;
     }
 
diff --git a/cache/stores/redis/tests/store_test.php b/cache/stores/redis/tests/store_test.php
index 8d7a6b40e08..0dc169d6c2e 100644
--- a/cache/stores/redis/tests/store_test.php
+++ b/cache/stores/redis/tests/store_test.php
@@ -155,10 +155,10 @@ final class store_test extends \cachestore_tests {
         $before = microtime(true);
         $this->assertFalse($store->acquire_lock('lock', '456'));
         $after = microtime(true);
-        $this->assertEqualsWithDelta(2, $after - $before, 0.5);
+        $this->assertEqualsWithDelta(2, $after - $before, 1);
 
-        // Wait another 2 seconds and then it should be able to get the lock because of timeout.
-        sleep(2);
+        // Wait another 3 seconds and then it should be able to get the lock because of timeout.
+        sleep(3);
         $this->assertTrue($store->acquire_lock('lock', '456'));
         $this->assertTrue($store->check_lock_state('lock', '456'));