INT-16370: Shared redis connection for redis lock.

David Castro · David Castro · commit 1394b4184d98 · 2020-10-12T15:14:53.000-05:00
diff --git a/README.md b/README.md
@@ -1,14 +1,20 @@
-#About
+# Redis lock
+
+## About
 Provides a Moodle lock factory class for locking with Redis. This plugin was contributed by the Blackboard Open LMS Product Development team.  Blackboard is an education technology company dedicated to bringing excellent online teaching to institutions across the globe.  We serve colleges and universities, schools and organizations by supporting the software that educators use to manage and deliver instructional content to learners in virtual classrooms.
 
-#Requirements
+## Requirements
 * Moodle 2.9 or greater
 * Redis
 * PHP Redis extension
 
-#Installation
+## Installation
 Clone the repository or download and extract the code into the local directory of your Moodle install (e.g. $CFG->wwwroot/local/redislock) and run the site's upgrade script. Set $CFG->local_redislock_redis_server with your Redis server's connection string. Set $CFG->lock_factory to '\\\\local_redislock\\\\lock\\\\redis_lock_factory' in your config file.
 
-Use the boolean flag `$CFG->local_redislock_logging` to control whether verbose
+## Flags
+
+* Use the boolean flag `$CFG->local_redislock_logging` to control whether verbose
 logging should be emitted. If not set, logging is automatically-enabled when running
 in the CLI environment with debugging enabled on `DEBUG_NORMAL` level at least.
+* Use the boolean flag `$CFG->local_redislock_disable_shared_connection` to force creation
+of the redis connection for each factory instance.
diff --git a/classes/api/shared_redis_connection.php b/classes/api/shared_redis_connection.php
@@ -0,0 +1,151 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Shared redis connection handling class.
+ *
+ * @package   local_redislock
+ * @author    David Castro
+ * @copyright Copyright (c) 2020 Open LMS. (https://www.openlms.net)
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+namespace local_redislock\api;
+
+defined('MOODLE_INTERNAL') || die();
+
+use Redis;
+
+/**
+ * Shared redis connection handling class.
+ *
+ * @package   local_redislock
+ * @author    David Castro
+ * @copyright Copyright (c) 2020 Open LMS. (https://www.openlms.net)
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+class shared_redis_connection {
+
+    /**
+     * @var shared_redis_connection
+     */
+    private static $instance;
+
+    /**
+     * @var Redis
+     */
+    private $redis;
+
+    /**
+     * @var int
+     */
+    private $factorycount;
+
+    /**
+     * @var boolean
+     */
+    private $logging;
+
+    /**
+     * Singleton constructor.
+     */
+    private function __construct() {
+        $this->factorycount = 0;
+        if (isset($CFG->local_redislock_logging)) {
+            $this->logging = (bool) $CFG->local_redislock_logging;
+        } else {
+            $this->logging = (CLI_SCRIPT && debugging() && !PHPUNIT_TEST);
+        }
+    }
+
+    /**
+     * @return shared_redis_connection
+     */
+    public static function get_instance() {
+        if (self::$instance == null) {
+            self::$instance = new shared_redis_connection();
+        }
+        return self::$instance;
+    }
+
+    /**
+     * @param Redis $redis
+     */
+    public function set_redis(Redis $redis) {
+        $this->redis = $redis;
+    }
+
+    /**
+     * @return Redis
+     */
+    public function get_redis(): ?Redis {
+        return $this->redis;
+    }
+
+    /**
+     * Closes the shared connection.
+     */
+    public function close() {
+        if (!is_null($this->redis) && $this->redis->isConnected()) {
+            $this->redis->close();
+            $this->redis = null;
+            $this->log("Shared Redis connection is closed.");
+        }
+    }
+
+    /**
+     * Clears the redis attribute. Use only when the connection has become unresponsive.
+     */
+    public function clear() {
+        $this->redis = null;
+    }
+
+    /**
+     * Adds a factory to the count.
+     */
+    public function add_factory() {
+        $this->factorycount++;
+    }
+
+    /**
+     * @throws \coding_exception
+     */
+    public function remove_factory() {
+        if (empty($this->factorycount)) {
+            throw new \coding_exception('Can\'t remove a factory, count is 0.');
+        }
+        $this->factorycount--;
+    }
+
+    /**
+     * @return int
+     */
+    public function get_factory_count() {
+        return $this->factorycount;
+    }
+
+    /**
+     * Log message
+     *
+     * @param $message
+     */
+    private function log($message) {
+        if ($this->logging) {
+            mtrace(sprintf('Redis lock; pid=%d; %s', getmypid(), $message));
+        }
+    }
+}
diff --git a/classes/lock/redis_lock_factory.php b/classes/lock/redis_lock_factory.php
@@ -29,6 +29,7 @@
 
 use core\lock\lock_factory;
 use core\lock\lock;
+use local_redislock\api\shared_redis_connection;
 
 /**
  * Redis-backed lock factory class.
@@ -60,6 +61,21 @@ class redis_lock_factory implements lock_factory {
      */
     protected $logging;
 
+    /**
+     * @var string Redis server connection string.
+     */
+    private $redisserver;
+
+    /**
+     * @var boolean Shared connection enabled.
+     */
+    private $shareconnection;
+
+    /**
+     * @var int Connection count.
+     */
+    private static $conncount = 0;
+
     /**
      * @param string $type The type this lock is used for (e.g. cron, cache).
      * @param \Redis|null $redis An instance of the PHPRedis extension class.
@@ -70,9 +86,14 @@ public function __construct($type, \Redis $redis = null, $logging = null) {
         global $CFG;
 
         $this->type = $type;
-
+        $this->redisserver = $CFG->local_redislock_redis_server ?? null;
+        $this->shareconnection = empty($CFG->local_redislock_disable_shared_connection);
         if (is_null($redis)) {
+            shared_redis_connection::get_instance()->add_factory();
             $redis = $this->bootstrap_redis();
+        } else {
+            // If a Redis instance is set, we shouldn't share it as we don't know who else is using it.
+            $this->shareconnection = false;
         }
         if (is_null($logging)) {
             if (isset($CFG->local_redislock_logging)) {
@@ -148,6 +169,11 @@ public function get_lock($resource, $timeout, $maxlifetime = 86400) {
             $resource = $CFG->dbname . '_' . $resource;
         }
 
+        if ($this->shareconnection) {
+            // Re-get the Redis shared connection in case it's be cleared or recreated elsewhere.
+            $this->redis = $this->bootstrap_redis();
+        }
+
         $this->log('Waiting to get '.$resource.' lock');
 
         $exception = false;
@@ -160,6 +186,10 @@ public function get_lock($resource, $timeout, $maxlifetime = 86400) {
             } catch (\RedisException $e) {
                 // If there has been a redis exception, we will try to reconnect.
                 $exception = $e;
+                if (!$this->shareconnection) {
+                    self::$conncount--;
+                }
+                shared_redis_connection::get_instance()->clear(); // Delete shared connection.
                 $this->log("Got exception while trying to get lock: {$e->getMessage()}");
                 $this->log("Attempting to reconnect to Redis");
                 $this->redis = $this->bootstrap_redis();
@@ -197,6 +227,11 @@ public function get_lock($resource, $timeout, $maxlifetime = 86400) {
     public function release_lock(lock $lock) {
         $resource = $lock->get_key();
 
+        if ($this->shareconnection) {
+            // Re-get the Redis shared connection in case it's be cleared or recreated elsewhere.
+            $this->redis = $this->bootstrap_redis();
+        }
+
         // We will retry connecting and releasing up to 5 times.
         $failcount = 0;
         $value = false;
@@ -212,6 +247,10 @@ public function release_lock(lock $lock) {
                 }
 
                 // If there has been a redis exception, we will try to reconnect.
+                if (!$this->shareconnection) {
+                    self::$conncount--;
+                }
+                shared_redis_connection::get_instance()->clear(); // Delete shared connection.
                 $this->log("Got exception while trying to release lock: {$e->getMessage()}");
                 $this->log('Attempting to reconnect to Redis');
                 $this->redis = $this->bootstrap_redis();
@@ -272,7 +311,18 @@ public function auto_release() {
             $lock->release();
         }
 
-        $this->redis->close();
+        if (!$this->shareconnection) {
+            // Connection is not shared. Closing now!
+            $this->redis->close();
+            self::$conncount--;
+            $conncount = self::$conncount;
+            $this->log("Connection to Redis from factory type {$this->type} is closed, {$conncount} remaining.");
+        } else {
+            shared_redis_connection::get_instance()->remove_factory();
+            if (empty(shared_redis_connection::get_instance()->get_factory_count())) {
+                shared_redis_connection::get_instance()->close();
+            }
+        }
     }
 
     /**
@@ -293,19 +343,27 @@ public function get_ttl(lock $lock) {
      * @throws \coding_exception
      */
     protected function bootstrap_redis() {
-        global $CFG;
+        if (!is_null($redis = shared_redis_connection::get_instance()->get_redis())) {
+            // Reuse the connection if available.
+            return $redis;
+        }
 
         if (!class_exists('Redis')) {
             throw new \coding_exception('Redis class not found, Redis PHP Extension is probably not installed on host: '
                     . $this->get_hostname());
         }
-        if (empty($CFG->local_redislock_redis_server)) {
+        if (empty($this->redisserver)) {
             throw new \coding_exception('Redis connection string is not configured in $CFG->local_redislock_redis_server');
         }
 
         try {
             $redis = new \Redis();
-            $redis->connect($CFG->local_redislock_redis_server);
+            $redis->connect($this->redisserver);
+            if ($this->shareconnection) {
+                shared_redis_connection::get_instance()->set_redis($redis); // Reusing the connection.
+            } else {
+                self::$conncount++;
+            }
         } catch (\RedisException $e) {
             throw new \coding_exception("RedisException caught on host {$this->get_hostname()} with message: {$e->getMessage()}");
         }
diff --git a/tests/redis_lock_factory_test.php b/tests/redis_lock_factory_test.php
@@ -26,6 +26,7 @@
 defined('MOODLE_INTERNAL') || die();
 
 use core\lock\lock_config;
+use local_redislock\api\shared_redis_connection;
 
 /**
  * PHPUnit testcase class for \local_redislock\lock\redis_lock_factory.
@@ -48,6 +49,16 @@ public function setUp() {
         $CFG->lock_factory = '\\local_redislock\\lock\\redis_lock_factory';
     }
 
+    /**
+     * @throws coding_exception
+     */
+    protected function tearDown() {
+        shared_redis_connection::get_instance()->close();
+        while (!empty(shared_redis_connection::get_instance()->get_factory_count())) {
+            shared_redis_connection::get_instance()->remove_factory();
+        }
+    }
+
     /**
      * Tests acquiring locks using the Redis lock factory.
      *
@@ -189,6 +200,44 @@ public function test_lock_zero_timeout() {
         $this->assertLessThan(.5, microtime(true) - $start);
     }
 
+    /**
+     * Tests shared connection.
+     *
+     * @throws coding_exception
+     */
+    public function test_shared_connection() {
+        if (!$this->is_redis_available()) {
+            $this->markTestSkipped('Redis server not available');
+        }
+
+        /** @var local_redislock\lock\redis_lock_factory $redislockfactory1 */
+        $redislockfactory1 = lock_config::get_lock_factory('conduit_cron');
+        $lock1 = $redislockfactory1->get_lock('shared_conn_test1', 10, 200);
+        $this->assertNotEmpty($lock1);
+        $redis1 = shared_redis_connection::get_instance()->get_redis();
+        $this->assertNotNull($redis1);
+        $lock1->release(); // All locks should be released.
+
+        /** @var local_redislock\lock\redis_lock_factory $redislockfactory2 */
+        $redislockfactory2 = lock_config::get_lock_factory('cron');
+        $lock2 = $redislockfactory2->get_lock('shared_conn_test2', 10, 200);
+        $this->assertNotEmpty($lock2);
+
+        // Simulating auto releases.
+        $redislockfactory1->auto_release(); // This should not close redis.
+
+        $redis2 = shared_redis_connection::get_instance()->get_redis();
+        $this->assertSame($redis1, $redis2);
+        $this->assertTrue($redis2->isConnected());
+
+        // Last auto-release.
+        $redislockfactory2->auto_release(); // This SHOULD close redis.
+
+        // Connection should be auto closed when Moodle shuts down (All auto-releases have run).
+        $redis3 = shared_redis_connection::get_instance()->get_redis();
+        $this->assertNull($redis3);
+    }
+
     /**
      * Helper method to determine whether a Redis server is available to run these tests.
      * If LOCAL_REDISLOCK_REDIS_LOCK_TEST is not true most of these tests will be skipped.
