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 <[email protected]>
Signed-off-by: Daniel Ziegenberg <[email protected]>

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

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