From 8bc44141be5370d68df4ecd93365bed85eb2d71e Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 19 Jan 2024 08:33:27 +0000 Subject: [PATCH] Merge pull request #16590 from overleaf/jpa-redis-lock-60s [server-ce] increase the doc lock TTL from 30s to 60s GitOrigin-RevId: 468f7483cc6a80e8034e3cc8071b674123985deb --- libraries/redis-wrapper/RedisLocker.js | 28 +++++++--- libraries/redis-wrapper/test/unit/src/test.js | 52 ++++++++++++++++++- server-ce/config/settings.js | 5 ++ .../document-updater/app/js/LockManager.js | 1 + .../config/settings.defaults.js | 2 + 5 files changed, 81 insertions(+), 7 deletions(-) diff --git a/libraries/redis-wrapper/RedisLocker.js b/libraries/redis-wrapper/RedisLocker.js index 2cc53725a9..03f861e95b 100644 --- a/libraries/redis-wrapper/RedisLocker.js +++ b/libraries/redis-wrapper/RedisLocker.js @@ -15,10 +15,11 @@ const UNLOCK_SCRIPT = module.exports = class RedisLocker { /** - * @param rclient initialized ioredis client - * @param getKey compose the redis key based on the passed id - * @param wrapTimeoutError assign the id to a designated field on the error - * @param metricsPrefix prefix all the metrics with the given prefix + * @param {import('ioredis')} rclient initialized ioredis client + * @param {function(string): string} getKey compose the redis key based on the passed id + * @param {function(Error, string): Error} wrapTimeoutError assign the id to a designated field on the error + * @param {string} metricsPrefix prefix all the metrics with the given prefix + * @param {number} lockTTLSeconds * * @example ``` * const lock = new RedisLocker({ @@ -35,7 +36,22 @@ module.exports = class RedisLocker { * } * ``` */ - constructor({ rclient, getKey, wrapTimeoutError, metricsPrefix }) { + constructor({ + rclient, + getKey, + wrapTimeoutError, + metricsPrefix, + lockTTLSeconds = 30, + }) { + if ( + typeof lockTTLSeconds !== 'number' || + lockTTLSeconds < 30 || + lockTTLSeconds >= 1000 + ) { + // set upper limit to 1000s to detect wrong units + throw new Error('redis lock TTL must be at least 30s and below 1000s') + } + this.rclient = rclient this.getKey = getKey this.wrapTimeoutError = wrapTimeoutError @@ -44,7 +60,7 @@ module.exports = class RedisLocker { this.LOCK_TEST_INTERVAL = 50 // 50ms between each test of the lock this.MAX_TEST_INTERVAL = 1000 // back off to 1s between each test of the lock this.MAX_LOCK_WAIT_TIME = 10000 // 10s maximum time to spend trying to get the lock - this.LOCK_TTL = 30 // seconds. Time until lock auto expires in redis. + this.LOCK_TTL = lockTTLSeconds // seconds. Time until lock auto expires in redis. // read-only copy for unit tests this.unlockScript = UNLOCK_SCRIPT diff --git a/libraries/redis-wrapper/test/unit/src/test.js b/libraries/redis-wrapper/test/unit/src/test.js index 732cdf9f73..03d42217ae 100644 --- a/libraries/redis-wrapper/test/unit/src/test.js +++ b/libraries/redis-wrapper/test/unit/src/test.js @@ -10,6 +10,7 @@ const assert = require('assert') const path = require('path') const sinon = require('sinon') const modulePath = path.join(__dirname, './../../../index.js') +const redisLockerModulePath = path.join(__dirname, './../../../RedisLocker.js') const { expect } = require('chai') describe('index', function () { @@ -53,9 +54,58 @@ describe('index', function () { }, globals: { process, + Buffer, }, }) - return (this.auth_pass = '1234 pass') + this.auth_pass = '1234 pass' + + this.RedisLocker = SandboxedModule.require(redisLockerModulePath, { + requires: { + '@overleaf/metrics': { + inc() {}, + }, + }, + globals: { + process, + Math, + Buffer, + }, + }) + }) + + describe('lock TTL', function () { + it('should throw an error when creating a client with wrong type', function () { + const createNewRedisLock = () => { + return new this.RedisLocker({ + lockTTLSeconds: '60', + }) + } + expect(createNewRedisLock).to.throw( + 'redis lock TTL must be at least 30s and below 1000s' + ) + }) + + it('should throw an error when creating a client with small TTL', function () { + const createNewRedisLock = () => { + return new this.RedisLocker({ + lockTTLSeconds: 1, + }) + } + expect(createNewRedisLock).to.throw( + 'redis lock TTL must be at least 30s and below 1000s' + ) + }) + + it('should throw an error when creating a client with huge TTL', function () { + const createNewRedisLock = () => { + return new this.RedisLocker({ + lockTTLSeconds: 30_000, + }) + } + expect(createNewRedisLock).to.throw( + 'redis lock TTL must be at least 30s and below 1000s' + ) + }) }) describe('redis-sentinel', function () { diff --git a/server-ce/config/settings.js b/server-ce/config/settings.js index 3d4aea512a..7c37c2ab76 100644 --- a/server-ce/config/settings.js +++ b/server-ce/config/settings.js @@ -235,6 +235,11 @@ const settings = { 10 ), + redisLockTTLSeconds: parseInt( + process.env.SHARELATEX_REDIS_LOCK_TTL_SECONDS || '60', + 10 + ), + i18n: { subdomainLang: { www: { diff --git a/services/document-updater/app/js/LockManager.js b/services/document-updater/app/js/LockManager.js index ccba1d55b1..a65ab9f1e3 100644 --- a/services/document-updater/app/js/LockManager.js +++ b/services/document-updater/app/js/LockManager.js @@ -14,4 +14,5 @@ module.exports = new RedisLocker({ return err }, metricsPrefix: 'doc', + lockTTLSeconds: Settings.redisLockTTLSeconds, }) diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index 27857821f8..9f4ce7bc8b 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -150,6 +150,8 @@ module.exports = { dispatcherCount: parseInt(process.env.DISPATCHER_COUNT || 10, 10), + redisLockTTLSeconds: 30, + mongo: { url: process.env.MONGO_CONNECTION_STRING ||