From aabe2d18b9770c99c6e8bf767c7bd78a5446ca86 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 10 Nov 2020 10:34:06 +0000 Subject: [PATCH] [misc] rewrite: squash history - promisify - merge health check for single node and cluster - replace the first multi with simple SET in health check - reworked health check with o-error context/stack-traces for failures - drop console.error on health check timeout, consumer logs the error - cleanup unwrapping of ioredis multi result - Promise support for multi.exec This has been squashed from das7pad s fork. REF: b3dd8c5cf4cc6482fd450e6bb67013508844f93f --- libraries/redis-wrapper/Errors.js | 15 ++ libraries/redis-wrapper/index.js | 239 ++++++++++++---------- libraries/redis-wrapper/package-lock.json | 18 -- libraries/redis-wrapper/package.json | 8 +- 4 files changed, 149 insertions(+), 131 deletions(-) create mode 100644 libraries/redis-wrapper/Errors.js diff --git a/libraries/redis-wrapper/Errors.js b/libraries/redis-wrapper/Errors.js new file mode 100644 index 0000000000..b3b3ec0b62 --- /dev/null +++ b/libraries/redis-wrapper/Errors.js @@ -0,0 +1,15 @@ +const OError = require('@overleaf/o-error') + +class RedisError extends OError {} +class RedisHealthCheckFailed extends RedisError {} +class RedisHealthCheckTimedOut extends RedisHealthCheckFailed {} +class RedisHealthCheckWriteError extends RedisHealthCheckFailed {} +class RedisHealthCheckVerifyError extends RedisHealthCheckFailed {} + +module.exports = { + RedisError, + RedisHealthCheckFailed, + RedisHealthCheckTimedOut, + RedisHealthCheckWriteError, + RedisHealthCheckVerifyError, +} diff --git a/libraries/redis-wrapper/index.js b/libraries/redis-wrapper/index.js index 80be830845..f6c600ae4a 100644 --- a/libraries/redis-wrapper/index.js +++ b/libraries/redis-wrapper/index.js @@ -1,14 +1,16 @@ -/* - * decaffeinate suggestions: - * DS101: Remove unnecessary use of Array.from - * DS102: Remove unnecessary code created because of implicit returns - * DS103: Rewrite code to no longer use __guard__, or convert again using --optional-chaining - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -const _ = require('underscore') -const os = require('os') const crypto = require('crypto') +const os = require('os') +const { promisify } = require('util') + +const Redis = require('ioredis') + +const { + RedisHealthCheckTimedOut, + RedisHealthCheckWriteError, + RedisHealthCheckVerifyError, +} = require('./Errors') + +const HEARTBEAT_TIMEOUT = 2000 // generate unique values for health check const HOST = os.hostname() @@ -17,130 +19,147 @@ const RND = crypto.randomBytes(4).toString('hex') let COUNT = 0 function createClient(opts) { - let client, standardOpts - if (opts == null) { - opts = { port: 6379, host: 'localhost' } - } - if (opts.retry_max_delay == null) { - opts.retry_max_delay = 5000 // ms + const standardOpts = Object.assign({}, opts) + delete standardOpts.key_schema + + if (standardOpts.retry_max_delay == null) { + standardOpts.retry_max_delay = 5000 // ms } - if (opts.cluster != null) { - const Redis = require('ioredis') - standardOpts = _.clone(opts) + let client + if (opts.cluster) { delete standardOpts.cluster - delete standardOpts.key_schema client = new Redis.Cluster(opts.cluster, standardOpts) - client.healthCheck = clusterHealthCheckBuilder(client) - _monkeyPatchIoredisExec(client) } else { - standardOpts = _.clone(opts) - const ioredis = require('ioredis') - client = new ioredis(standardOpts) - _monkeyPatchIoredisExec(client) - client.healthCheck = singleInstanceHealthCheckBuilder(client) + client = new Redis(standardOpts) + } + monkeyPatchIoRedisExec(client) + client.healthCheck = (callback) => { + if (callback) { + // callback based invocation + healthCheck(client).then(callback).catch(callback) + } else { + // Promise based invocation + return healthCheck(client) + } } return client } -const HEARTBEAT_TIMEOUT = 2000 -function singleInstanceHealthCheckBuilder(client) { - const healthCheck = (callback) => _checkClient(client, callback) - return healthCheck -} - -function clusterHealthCheckBuilder(client) { - return singleInstanceHealthCheckBuilder(client) -} - -function _checkClient(client, callback) { - callback = _.once(callback) +async function healthCheck(client) { // check the redis connection by storing and retrieving a unique key/value pair const uniqueToken = `host=${HOST}:pid=${PID}:random=${RND}:time=${Date.now()}:count=${COUNT++}` - const timer = setTimeout(function () { - const error = new Error( - `redis client health check timed out ${__guard__( - client != null ? client.options : undefined, - (x) => x.host - )}` - ) - console.error( - { - err: error, - key: client.options != null ? client.options.key : undefined, // only present for cluster - clientOptions: client.options, - uniqueToken, - }, - 'client timed out' - ) - return callback(error) - }, HEARTBEAT_TIMEOUT) + + // o-error context + const context = { + uniqueToken, + stage: 'add context for a timeout', + } + + await runWithTimeout({ + runner: runCheck(client, uniqueToken, context), + timeout: HEARTBEAT_TIMEOUT, + context, + }) +} + +async function runCheck(client, uniqueToken, context) { const healthCheckKey = `_redis-wrapper:healthCheckKey:{${uniqueToken}}` const healthCheckValue = `_redis-wrapper:healthCheckValue:{${uniqueToken}}` + // set the unique key/value pair - let multi = client.multi() - multi.set(healthCheckKey, healthCheckValue, 'EX', 60) - return multi.exec(function (err, reply) { - if (err != null) { - clearTimeout(timer) - return callback(err) - } - // check that we can retrieve the unique key/value pair - multi = client.multi() - multi.get(healthCheckKey) - multi.del(healthCheckKey) - return multi.exec(function (err, reply) { - clearTimeout(timer) - if (err != null) { - return callback(err) - } - if ( - (reply != null ? reply[0] : undefined) !== healthCheckValue || - (reply != null ? reply[1] : undefined) !== 1 - ) { - return callback(new Error('bad response from redis health check')) - } - return callback() + context.stage = 'write' + const writeAck = await client + .set(healthCheckKey, healthCheckValue, 'EX', 60) + .catch((err) => { + throw new RedisHealthCheckWriteError('write errored', context, err) }) - }) + if (writeAck !== 'OK') { + context.writeAck = writeAck + throw new RedisHealthCheckWriteError('write failed', context) + } + + // check that we can retrieve the unique key/value pair + context.stage = 'verify' + const [roundTrippedHealthCheckValue, deleteAck] = await client + .multi() + .get(healthCheckKey) + .del(healthCheckKey) + .exec() + .catch((err) => { + throw new RedisHealthCheckVerifyError( + 'read/delete errored', + context, + err + ) + }) + if (roundTrippedHealthCheckValue !== healthCheckValue) { + context.roundTrippedHealthCheckValue = roundTrippedHealthCheckValue + throw new RedisHealthCheckVerifyError('read failed', context) + } + if (deleteAck !== 1) { + context.deleteAck = deleteAck + throw new RedisHealthCheckVerifyError('delete failed', context) + } } -function _monkeyPatchIoredisExec(client) { +function unwrapMultiResult(result, callback) { + // ioredis exec returns a results like: + // [ [null, 42], [null, "foo"] ] + // where the first entries in each 2-tuple are + // presumably errors for each individual command, + // and the second entry is the result. We need to transform + // this into the same result as the old redis driver: + // [ 42, "foo" ] + // + // Basically reverse: + // https://github.com/luin/ioredis/blob/v4.17.3/lib/utils/index.ts#L75-L92 + const filteredResult = [] + for (const [err, value] of result || []) { + if (err) { + return callback(err) + } else { + filteredResult.push(value) + } + } + callback(null, filteredResult) +} +const unwrapMultiResultPromisified = promisify(unwrapMultiResult) + +function monkeyPatchIoRedisExec(client) { const _multi = client.multi - return (client.multi = function (...args) { - const multi = _multi.call(client, ...Array.from(args)) + client.multi = function () { + const multi = _multi.apply(client, arguments) const _exec = multi.exec - multi.exec = function (callback) { - if (callback == null) { - callback = function () {} + multi.exec = (callback) => { + if (callback) { + // callback based invocation + _exec.call(multi, (error, result) => { + // The command can fail all-together due to syntax errors + if (error) return callback(error) + unwrapMultiResult(result, callback) + }) + } else { + // Promise based invocation + return _exec.call(multi).then(unwrapMultiResultPromisified) } - return _exec.call(multi, function (error, result) { - // ioredis exec returns an results like: - // [ [null, 42], [null, "foo"] ] - // where the first entries in each 2-tuple are - // presumably errors for each individual command, - // and the second entry is the result. We need to transform - // this into the same result as the old redis driver: - // [ 42, "foo" ] - const filtered_result = [] - for (const entry of Array.from(result || [])) { - if (entry[0] != null) { - return callback(entry[0]) - } else { - filtered_result.push(entry[1]) - } - } - return callback(error, filtered_result) - }) } return multi - }) + } } -function __guard__(value, transform) { - return typeof value !== 'undefined' && value !== null - ? transform(value) - : undefined +async function runWithTimeout({ runner, timeout, context }) { + let healthCheckDeadline + await Promise.race([ + new Promise((resolve, reject) => { + healthCheckDeadline = setTimeout(() => { + // attach the timeout when hitting the timeout only + context.timeout = timeout + reject(new RedisHealthCheckTimedOut('timeout', context)) + }, timeout) + }), + runner.finally(() => clearTimeout(healthCheckDeadline)), + ]) } module.exports = { diff --git a/libraries/redis-wrapper/package-lock.json b/libraries/redis-wrapper/package-lock.json index f46c613bab..86fa19296b 100644 --- a/libraries/redis-wrapper/package-lock.json +++ b/libraries/redis-wrapper/package-lock.json @@ -919,14 +919,6 @@ "resolved": "https://registry.npmjs.org/cluster-key-slot/-/cluster-key-slot-1.1.0.tgz", "integrity": "sha512-2Nii8p3RwAPiFwsnZvukotvow2rIHM+yQ6ZcBXGHdniadkYGZYiGmkHJIbZPIV9nfv7m/U1IPMVVcAhoWFeklw==" }, - "coffee-script": { - "version": "1.8.0", - "resolved": "https://registry.npmjs.org/coffee-script/-/coffee-script-1.8.0.tgz", - "integrity": "sha1-nJ8dK0pSoADe0Vtll5FwNkgmPB0=", - "requires": { - "mkdirp": "~0.3.5" - } - }, "color-convert": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/color-convert/-/color-convert-2.0.1.tgz", @@ -2615,11 +2607,6 @@ "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", "dev": true }, - "mkdirp": { - "version": "0.3.5", - "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.3.5.tgz", - "integrity": "sha1-3j5fiWHIjHh+4TaN+EmsRBPsqNc=" - }, "mocha": { "version": "8.2.1", "resolved": "https://registry.npmjs.org/mocha/-/mocha-8.2.1.tgz", @@ -4508,11 +4495,6 @@ "integrity": "sha512-BLbiRkiBzAwsjut4x/dsibSTB6yWpwT5qWmC2OfuCg3GgVQCSgMs4vEctYPhsaGtd0AeuuHMkjZ2h2WG8MSzRw==", "dev": true }, - "underscore": { - "version": "1.7.0", - "resolved": "https://registry.npmjs.org/underscore/-/underscore-1.7.0.tgz", - "integrity": "sha1-a7rwh3UA02vjTsqlhODbn+8DUgk=" - }, "uri-js": { "version": "4.4.0", "resolved": "https://registry.npmjs.org/uri-js/-/uri-js-4.4.0.tgz", diff --git a/libraries/redis-wrapper/package.json b/libraries/redis-wrapper/package.json index 323f6a92ce..5d42b60815 100644 --- a/libraries/redis-wrapper/package.json +++ b/libraries/redis-wrapper/package.json @@ -11,12 +11,14 @@ "format:fix": "prettier-eslint $PWD'/**/*.js' --write", "test": "mocha --recursive test/unit/src/" }, + "peerDependencies": { + "@overleaf/o-error": "^3.1.0" + }, "dependencies": { - "coffee-script": "1.8.0", - "ioredis": "~4.17.3", - "underscore": "1.7.0" + "ioredis": "~4.17.3" }, "devDependencies": { + "@overleaf/o-error": "^3.1.0", "chai": "^4.2.0", "eslint": "^6.8.0", "eslint-config-prettier": "^6.10.1",