@uppy/companion: support redis sentinel

[dschmidt]

Initial checklist

• I understand this is a feature request and questions should be posted in the Community Forum
• I searched issues and couldn’t find anything (or linked relevant results below)

Problem

When I was talking to dev ops about companion deployment, they would have liked it to support redis sentinel natively, which apparently currently is not the case.

Solution

I'm no expert on this topic, but afaict ioredis has sophisticated support for this, while node-redis does not.
When I saw connect-redis abstracts them away, I was thinking it would be super straightforward to switch from node-redis to ioredis - it was until it wasn't

Porting createClient was easy, redis-emitter not sooo much.

I'm not sending a PR because switching libraries is usually rather controversial and I would like to discuss options first.

Have you considered switching to ioredis or are there reasons not to do it?
Possibly a breaking change if someone is passing advanced options to the library...

Alternatives

• switch to ioredis completely
• support ioredis and node-redis simultaneously
• don't change anything (just updateredis-connect which is slightly outdated)

Excerpt from my patch

diff --git a/package.json b/package.json
index d62b40b93..81ad5bb10 100644
--- a/package.json
+++ b/package.json
@@ -168,7 +168,6 @@
     ]
   },
   "resolutions": {
-    "@types/[email protected]": "patch:@types/connect-redis@npm:0.0.18#.yarn/patches/@types-connect-redis-npm-0.0.18-4fd2b614d3",
     "@types/eslint@^7.2.13": "^8.2.0",
     "@types/react": "^17",
     "@types/webpack-dev-server": "^4",
diff --git a/packages/@uppy/companion/package.json b/packages/@uppy/companion/package.json
index e6f6362f2..5b29ac696 100644
--- a/packages/@uppy/companion/package.json
+++ b/packages/@uppy/companion/package.json
@@ -36,7 +36,7 @@
     "body-parser": "1.20.0",
     "chalk": "4.1.2",
     "common-tags": "1.8.2",
-    "connect-redis": "6.1.3",
+    "connect-redis": "7.1.0",
     "content-disposition": "^0.5.4",
     "cookie-parser": "1.4.6",
     "cors": "^2.8.5",
@@ -52,6 +52,7 @@
     "got": "11",
     "grant": "5.4.21",
     "helmet": "^4.6.0",
+    "ioredis": "5.3.2",
     "ipaddr.js": "^2.0.1",
     "jsonwebtoken": "8.5.1",
     "lodash": "^4.17.21",
@@ -62,7 +63,6 @@
     "ms": "2.1.3",
     "node-schedule": "2.1.0",
     "prom-client": "14.0.1",
-    "redis": "4.2.0",
     "semver": "7.5.3",
     "serialize-error": "^2.1.0",
     "serialize-javascript": "^6.0.0",
@@ -73,7 +73,6 @@
   },
   "devDependencies": {
     "@types/compression": "1.7.0",
-    "@types/connect-redis": "0.0.18",
     "@types/cookie-parser": "1.4.2",
     "@types/cors": "2.8.6",
     "@types/eslint": "^8.2.0",
diff --git a/packages/@uppy/companion/src/server/emitter/redis-emitter.js b/packages/@uppy/companion/src/server/emitter/redis-emitter.js
index df1badfef..8f7d6615b 100644
--- a/packages/@uppy/companion/src/server/emitter/redis-emitter.js
+++ b/packages/@uppy/companion/src/server/emitter/redis-emitter.js
@@ -1,4 +1,4 @@
-const redis = require('redis')
+const Redis = require('ioredis').default
 const { EventEmitter } = require('node:events')
 
 const logger = require('../logger')
@@ -11,13 +11,13 @@ const logger = require('../logger')
 module.exports = (redisUrl, redisPubSubScope) => {
   const prefix = redisPubSubScope ? `${redisPubSubScope}:` : ''
   const getPrefixedEventName = (eventName) => `${prefix}${eventName}`
-  const publisher = redis.createClient({ url: redisUrl })
-  publisher.on('error', err => logger.error('publisher redis error', err))
+  const publisher = new Redis(redisUrl, { lazyConnect: true })
+  publisher.on('error', err => logger.error('publisher redis error', err.toString()))
   let subscriber
 
   const connectedPromise = publisher.connect().then(() => {
     subscriber = publisher.duplicate()
-    subscriber.on('error', err => logger.error('subscriber redis error', err))
+    subscriber.on('error', err => logger.error('subscriber redis error', err.toString()))
     return subscriber.connect()
   })
 
@@ -56,12 +56,17 @@ module.exports = (redisUrl, redisPubSubScope) => {
       handlersByThisEventName.delete(handler)
       if (handlersByThisEventName.size === 0) handlersByEvent.delete(eventName)
 
-      return subscriber.pUnsubscribe(getPrefixedEventName(eventName), actualHandler)
+      subscriber.off('message', actualHandler)
+      return subscriber.punsubscribe(getPrefixedEventName(eventName))
     })
   }
 
   function addListener (eventName, handler, _once = false) {
-    function actualHandler (message) {
+    function actualHandler (pattern, channel, message) {
+      if (pattern !== getPrefixedEventName(eventName)) {
+        return
+      }
+
       if (_once) removeListener(eventName, handler)
       let args
       try {
@@ -79,7 +84,10 @@ module.exports = (redisUrl, redisPubSubScope) => {
     }
     handlersByThisEventName.set(handler, actualHandler)
 
-    runWhenConnected(() => subscriber.pSubscribe(getPrefixedEventName(eventName), actualHandler))
+    runWhenConnected(() => {
+      subscriber.on('pmessage', actualHandler)
+      return subscriber.psubscribe(getPrefixedEventName(eventName))
+    })
   }
 
   /**
@@ -112,7 +120,9 @@ module.exports = (redisUrl, redisPubSubScope) => {
    * @param {string} eventName name of the event
    */
   function emit (eventName, ...args) {
-    runWhenConnected(() => publisher.publish(getPrefixedEventName(eventName), JSON.stringify(args)))
+    runWhenConnected(() => {
+      return publisher.publish(getPrefixedEventName(eventName), JSON.stringify(args))
+    })
   }
 
   /**
@@ -125,7 +135,7 @@ module.exports = (redisUrl, redisPubSubScope) => {
 
     return runWhenConnected(() => {
       handlersByEvent.delete(eventName)
-      return subscriber.pUnsubscribe(getPrefixedEventName(eventName))
+      return subscriber.punsubscribe(getPrefixedEventName(eventName))
     })
   }
 
diff --git a/packages/@uppy/companion/src/server/redis.js b/packages/@uppy/companion/src/server/redis.js
index 10298c6f0..87e2209bc 100644
--- a/packages/@uppy/companion/src/server/redis.js
+++ b/packages/@uppy/companion/src/server/redis.js
@@ -1,4 +1,4 @@
-const redis = require('redis')
+const Redis = require('ioredis').default
 
 const logger = require('./logger')
 
@@ -8,24 +8,13 @@ let redisClient
  * A Singleton module that provides a single redis client through out
  * the lifetime of the server
  *
- * @param {Record<string, unknown>} [opts] node-redis client options
+ * @param {Record<string, any>} [opts] node-redis client options
  */
 function createClient (opts) {
   if (!redisClient) {
-    // todo remove legacyMode when fixed: https://github.com/tj/connect-redis/issues/361
-    redisClient = redis.createClient({ ...opts, legacyMode: true })
+    redisClient = new Redis(opts.url, opts)
 
-    redisClient.on('error', err => logger.error('redis error', err))
-
-    ;(async () => {
-      try {
-        // fire and forget.
-        // any requests made on the client before connection is established will be auto-queued by node-redis
-        await redisClient.connect()
-      } catch (err) {
-        logger.error(err.message, 'redis.error')
-      }
-    })()
+    redisClient.on('error', err => logger.error('redis error', err.toString()))
   }
 
   return redisClient
diff --git a/packages/@uppy/companion/src/standalone/index.js b/packages/@uppy/companion/src/standalone/index.js
index fa395782d..9a8f94610 100644
--- a/packages/@uppy/companion/src/standalone/index.js
+++ b/packages/@uppy/companion/src/standalone/index.js
@@ -5,7 +5,7 @@ const morgan = require('morgan')
 const { URL } = require('node:url')
 const session = require('express-session')
 const addRequestId = require('express-request-id')()
-const connectRedis = require('connect-redis')
+const RedisStore = require('connect-redis').default
 
 const logger = require('../server/logger')
 const redis = require('../server/redis')
@@ -110,7 +110,6 @@ module.exports = function server (inputCompanionOptions) {
   }
 
   if (companionOptions.redisUrl) {
-    const RedisStore = connectRedis(session)
     const redisClient = redis.client(companionOptions)
     // todo next major: change default prefix to something like "companion-session:" and possibly remove this option
     sessionOptions.store = new RedisStore({ client: redisClient, prefix: process.env.COMPANION_REDIS_EXPRESS_SESSION_PREFIX || 'sess:' })

[mifi]

Personally I've used ioredis in other projects and have been very happy with it, but yea porting redis-emitter might be non trivial, so I'm not sure if this is something that will be prioritised any time soon.

[dschmidt]

Hehe ... well, see the patch - that's what I needed to make it work for me.

If we/you agree on an approach, I will send a PR... thinking about it again, I could have sent one from the beginning on 😁

[Murderlon]

I'm not against this change but I would be curious what the cons are of migrating. Is it breaking for custom providers? Is there functionality node-redis has which ioredis doesn't have?

[dschmidt]

Fixed via #4623