diff --git a/cache/stores/redis/addinstanceform.php b/cache/stores/redis/addinstanceform.php index 6fa64c68a5f..e775bbcd0db 100644 --- a/cache/stores/redis/addinstanceform.php +++ b/cache/stores/redis/addinstanceform.php @@ -65,5 +65,10 @@ class cachestore_redis_addinstance_form extends cachestore_addinstance_form { $form->addHelpButton('compressor', 'usecompressor', 'cachestore_redis'); $form->setDefault('compressor', cachestore_redis::COMPRESSOR_NONE); $form->setType('compressor', PARAM_INT); + + $form->addElement('text', 'connectiontimeout', get_string('connectiontimeout', 'cachestore_redis')); + $form->addHelpButton('connectiontimeout', 'connectiontimeout', 'cachestore_redis'); + $form->setDefault('connectiontimeout', cachestore_redis::CONNECTION_TIMEOUT); + $form->setType('connectiontimeout', PARAM_INT); } } diff --git a/cache/stores/redis/lang/en/cachestore_redis.php b/cache/stores/redis/lang/en/cachestore_redis.php index f7674800d74..c3332562c8f 100644 --- a/cache/stores/redis/lang/en/cachestore_redis.php +++ b/cache/stores/redis/lang/en/cachestore_redis.php @@ -32,6 +32,8 @@ $string['clustermodeunavailable'] = 'Redis Cluster is currently unavailable. Ple $string['compressor_none'] = 'No compression.'; $string['compressor_php_gzip'] = 'Use gzip compression.'; $string['compressor_php_zstd'] = 'Use Zstandard compression.'; +$string['connectiontimeout'] = 'Connection timeout'; +$string['connectiontimeout_help'] = 'This sets the timeout when attempting to connect to the Redis server.'; $string['encrypt_connection'] = 'Use TLS encryption.'; $string['encrypt_connection_help'] = 'Use TLS to connect to Redis. Do not use \'tls://\' in the hostname for Redis, use this option instead.'; $string['password'] = 'Password'; diff --git a/cache/stores/redis/lib.php b/cache/stores/redis/lib.php index 4d3d1a622f9..170f04fc6b9 100644 --- a/cache/stores/redis/lib.php +++ b/cache/stores/redis/lib.php @@ -66,7 +66,7 @@ class cachestore_redis extends store implements const TTL_EXPIRE_BATCH = 10000; /** @var int The number of seconds to wait for a connection or response from the Redis server. */ - const CONNECTION_TIMEOUT = 10; + const CONNECTION_TIMEOUT = 3; /** * Name of this store. @@ -117,6 +117,14 @@ class cachestore_redis extends store implements */ protected $compressor = self::COMPRESSOR_NONE; + + /** + * The number of seconds to wait for a connection or response from the Redis server. + * + * @var int + */ + protected $connectiontimeout = self::CONNECTION_TIMEOUT; + /** * Bytes read or written by last call to set()/get() or set_many()/get_many(). * @@ -197,6 +205,9 @@ class cachestore_redis extends store implements if (array_key_exists('compressor', $configuration)) { $this->compressor = (int)$configuration['compressor']; } + if (array_key_exists('connectiontimeout', $configuration)) { + $this->connectiontimeout = (int)$configuration['connectiontimeout']; + } if (array_key_exists('lockwait', $configuration)) { $this->lockwait = (int)$configuration['lockwait']; } @@ -277,8 +288,8 @@ class cachestore_redis extends store implements $redis = new RedisCluster( name: null, seeds: $trimmedservers, - timeout: self::CONNECTION_TIMEOUT, // Timeout. - read_timeout: self::CONNECTION_TIMEOUT, // Read timeout. + timeout: $this->connectiontimeout, // Timeout. + read_timeout: $this->connectiontimeout, // Read timeout. persistent: true, auth: $password, context: !empty($opts) ? $opts : null, @@ -287,8 +298,8 @@ class cachestore_redis extends store implements $redis = new RedisCluster( null, $trimmedservers, - self::CONNECTION_TIMEOUT, - self::CONNECTION_TIMEOUT, + $this->connectiontimeout, + $this->connectiontimeout, true, $password, !empty($opts) ? $opts : null, ); @@ -300,18 +311,18 @@ class cachestore_redis extends store implements $redis->connect( host: $server, port: $port, - timeout: self::CONNECTION_TIMEOUT, // Timeout. + timeout: $this->connectiontimeout, // Timeout. retry_interval: 100, // Retry interval. - read_timeout: self::CONNECTION_TIMEOUT, // Read timeout. + read_timeout: $this->connectiontimeout, // Read timeout. context: $opts, ); } else { $redis->connect( $server, $port, - self::CONNECTION_TIMEOUT, + $this->connectiontimeout, null, 100, - self::CONNECTION_TIMEOUT, + $this->connectiontimeout, $opts, ); } @@ -863,6 +874,7 @@ class cachestore_redis extends store implements 'password' => $data->password, 'serializer' => $data->serializer, 'compressor' => $data->compressor, + 'connectiontimeout' => $data->connectiontimeout, 'encryption' => $data->encryption, 'cafile' => $data->cafile, 'clustermode' => $data->clustermode, @@ -887,6 +899,9 @@ class cachestore_redis extends store implements if (!empty($config['compressor'])) { $data['compressor'] = $config['compressor']; } + if (!empty($config['connectiontimeout'])) { + $data['connectiontimeout'] = $config['connectiontimeout']; + } if (!empty($config['encryption'])) { $data['encryption'] = $config['encryption']; } diff --git a/config-dist.php b/config-dist.php index b08a8fffe2a..36fc1e86d2b 100644 --- a/config-dist.php +++ b/config-dist.php @@ -370,6 +370,8 @@ $CFG->admin = 'admin'; // $CFG->session_redis_lock_expire = 7200; // Optional, defaults to session timeout. // $CFG->session_redis_lock_retry = 100; // Optional wait between lock attempts in ms, default is 100. // // After 5 seconds it will throttle down to once per second. +// $CFG->session_redis_connection_timeout = 3; // Optional, default is 3. +// $CFG->session_redis_maxretries = 3; // Optional, default is 3. // // Use the igbinary serializer instead of the php default one. Note that phpredis must be compiled with // igbinary support to make the setting to work. Also, if you change the serializer you have to flush the database! diff --git a/lib/classes/session/redis.php b/lib/classes/session/redis.php index 7f11fdb9f1c..a70341b2786 100644 --- a/lib/classes/session/redis.php +++ b/lib/classes/session/redis.php @@ -106,7 +106,7 @@ class redis extends handler implements SessionHandlerInterface { protected bool $clustermode = false; /** @var int Maximum number of retries for cache store operations. */ - const MAX_RETRIES = 5; + protected int $maxretries = 3; /** @var int $firstaccesstimeout The initial timeout (seconds) for the first browser access without login. */ protected int $firstaccesstimeout = 180; @@ -114,8 +114,8 @@ class redis extends handler implements SessionHandlerInterface { /** @var clock A clock instance */ protected clock $clock; - /** @var int The number of seconds to wait for a connection or response from the Redis server. */ - const CONNECTION_TIMEOUT = 10; + /** @var int $connectiontimeout The number of seconds to wait for a connection or response from the Redis server. */ + protected int $connectiontimeout = 3; /** * Create new instance of handler. @@ -204,6 +204,14 @@ class redis extends handler implements SessionHandlerInterface { $this->compressor = $CFG->session_redis_compressor; } + if (isset($CFG->session_redis_connection_timeout)) { + $this->connectiontimeout = (int)$CFG->session_redis_connection_timeout; + } + + if (isset($CFG->session_redis_max_retries)) { + $this->maxretries = (int)$CFG->session_redis_max_retries; + } + $this->clock = di::get(clock::class); } @@ -278,10 +286,10 @@ class redis extends handler implements SessionHandlerInterface { } } - // MDL-59866: Add retries for connections (up to 5 times) to make sure it goes through. + // Add retries for connections to make sure it goes through. $counter = 1; $exceptionclass = $this->clustermode ? 'RedisClusterException' : 'RedisException'; - while ($counter <= self::MAX_RETRIES) { + while ($counter <= $this->maxretries) { $this->connection = null; // Make a connection to Redis server(s). try { @@ -293,8 +301,8 @@ class redis extends handler implements SessionHandlerInterface { $this->connection = new \RedisCluster( name: null, seeds: $trimmedservers, - timeout: self::CONNECTION_TIMEOUT, // Timeout. - read_timeout: self::CONNECTION_TIMEOUT, // Read timeout. + timeout: $this->connectiontimeout, // Timeout. + read_timeout: $this->connectiontimeout, // Read timeout. persistent: true, auth: $this->auth, context: !empty($opts) ? $opts : null, @@ -303,8 +311,8 @@ class redis extends handler implements SessionHandlerInterface { $this->connection = new \RedisCluster( null, $trimmedservers, - self::CONNECTION_TIMEOUT, - self::CONNECTION_TIMEOUT, + $this->connectiontimeout, + $this->connectiontimeout, true, $this->auth, !empty($opts) ? $opts : null @@ -318,19 +326,19 @@ class redis extends handler implements SessionHandlerInterface { $this->connection->connect( host: $server, port: $port, - timeout: self::CONNECTION_TIMEOUT, // Timeout. + timeout: $this->connectiontimeout, // Timeout. retry_interval: $delay, - read_timeout: self::CONNECTION_TIMEOUT, // Read timeout. + read_timeout: $this->connectiontimeout, // Read timeout. context: $opts, ); } else { $this->connection->connect( $server, $port, - self::CONNECTION_TIMEOUT, + $this->connectiontimeout, null, $delay, - self::CONNECTION_TIMEOUT, + $this->connectiontimeout, $opts ); } @@ -383,7 +391,7 @@ class redis extends handler implements SessionHandlerInterface { return true; } catch (RedisException | RedisClusterException $e) { $redishost = $this->clustermode ? implode(',', $this->host) : $server . ':' . $port; - $logstring = "Failed to connect (try {$counter} out of " . self::MAX_RETRIES . ") to Redis "; + $logstring = "Failed to connect (try {$counter} out of " . $this->maxretries . ") to Redis "; $logstring .= "at ". $redishost .", the error returned was: {$e->getMessage()}"; debugging($logstring); } diff --git a/lib/tests/session/redis_test.php b/lib/tests/session/redis_test.php index a2346ba8586..52f8cba2095 100644 --- a/lib/tests/session/redis_test.php +++ b/lib/tests/session/redis_test.php @@ -360,8 +360,8 @@ final class redis_test extends \advanced_testcase { // Therefore, to get the host, we need to explode it. list($host, ) = explode(':', TEST_SESSION_REDIS_HOST); - $expected = "Failed to connect (try 5 out of 5) to Redis at $host:111111"; - $this->assertDebuggingCalledCount(5); + $expected = "Failed to connect (try 3 out of 3) to Redis at $host:111111"; + $this->assertDebuggingCalledCount(3); $this->assertStringContainsString($expected, $actual); }