[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

@@ -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,
+}

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
+  const standardOpts = Object.assign({}, opts)
-  if (opts == null) {
+  delete standardOpts.key_schema
-    opts = { port: 6379, host: 'localhost' }
+
-  }
+  if (standardOpts.retry_max_delay == null) {
-  if (opts.retry_max_delay == null) {
+    standardOpts.retry_max_delay = 5000 // ms
-    opts.retry_max_delay = 5000 // ms
   }
 
-  if (opts.cluster != null) {
+  let client
-    const Redis = require('ioredis')
+  if (opts.cluster) {
-    standardOpts = _.clone(opts)
     delete standardOpts.cluster
-    delete standardOpts.key_schema
     client = new Redis.Cluster(opts.cluster, standardOpts)
-    client.healthCheck = clusterHealthCheckBuilder(client)
-    _monkeyPatchIoredisExec(client)
   } else {
-    standardOpts = _.clone(opts)
+    client = new Redis(standardOpts)
-    const ioredis = require('ioredis')
+  }
-    client = new ioredis(standardOpts)
+  monkeyPatchIoRedisExec(client)
-    _monkeyPatchIoredisExec(client)
+  client.healthCheck = (callback) => {
-    client.healthCheck = singleInstanceHealthCheckBuilder(client)
+    if (callback) {
+      // callback based invocation
+      healthCheck(client).then(callback).catch(callback)
+    } else {
+      // Promise based invocation
+      return healthCheck(client)
+    }
   }
   return client
 }
 
-const HEARTBEAT_TIMEOUT = 2000
+async function healthCheck(client) {
-function singleInstanceHealthCheckBuilder(client) {
-  const healthCheck = (callback) => _checkClient(client, callback)
-  return healthCheck
-}
-
-function clusterHealthCheckBuilder(client) {
-  return singleInstanceHealthCheckBuilder(client)
-}
-
-function _checkClient(client, callback) {
-  callback = _.once(callback)
   // 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(
+  // o-error context
-      `redis client health check timed out ${__guard__(
+  const context = {
-        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,
-      },
+    stage: 'add context for a timeout',
-      'client timed out'
-    )
-    return callback(error)
-  }, HEARTBEAT_TIMEOUT)
-  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()
+  await runWithTimeout({
-    multi.get(healthCheckKey)
+    runner: runCheck(client, uniqueToken, context),
-    multi.del(healthCheckKey)
+    timeout: HEARTBEAT_TIMEOUT,
-    return multi.exec(function (err, reply) {
+    context,
-      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()
-    })
   })
 }
 
-function _monkeyPatchIoredisExec(client) {
+async function runCheck(client, uniqueToken, context) {
-  const _multi = client.multi
+  const healthCheckKey = `_redis-wrapper:healthCheckKey:{${uniqueToken}}`
-  return (client.multi = function (...args) {
+  const healthCheckValue = `_redis-wrapper:healthCheckValue:{${uniqueToken}}`
-    const multi = _multi.call(client, ...Array.from(args))
+
-    const _exec = multi.exec
+  // set the unique key/value pair
-    multi.exec = function (callback) {
+  context.stage = 'write'
-      if (callback == null) {
+  const writeAck = await client
-        callback = function () {}
+    .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)
   }
-      return _exec.call(multi, function (error, result) {
+
-        // ioredis exec returns an results like:
+  // 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 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" ]
-        const filtered_result = []
+  //
-        for (const entry of Array.from(result || [])) {
+  // Basically reverse:
-          if (entry[0] != null) {
+  // https://github.com/luin/ioredis/blob/v4.17.3/lib/utils/index.ts#L75-L92
-            return callback(entry[0])
+  const filteredResult = []
+  for (const [err, value] of result || []) {
+    if (err) {
+      return callback(err)
     } else {
-            filtered_result.push(entry[1])
+      filteredResult.push(value)
     }
   }
-        return callback(error, filtered_result)
+  callback(null, filteredResult)
+}
+const unwrapMultiResultPromisified = promisify(unwrapMultiResult)
+
+function monkeyPatchIoRedisExec(client) {
+  const _multi = client.multi
+  client.multi = function () {
+    const multi = _multi.apply(client, arguments)
+    const _exec = multi.exec
+    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 multi
-  })
+  }
 }
 
-function __guard__(value, transform) {
+async function runWithTimeout({ runner, timeout, context }) {
-  return typeof value !== 'undefined' && value !== null
+  let healthCheckDeadline
-    ? transform(value)
+  await Promise.race([
-    : undefined
+    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 = {

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",

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"
-    "ioredis": "~4.17.3",
-    "underscore": "1.7.0"
   },
   "devDependencies": {
+    "@overleaf/o-error": "^3.1.0",
     "chai": "^4.2.0",
     "eslint": "^6.8.0",
     "eslint-config-prettier": "^6.10.1",