Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@uppy/companion: support redis sentinel #4571

Open
2 tasks done
dschmidt opened this issue Jul 12, 2023 · 3 comments · May be fixed by #4623
Open
2 tasks done

@uppy/companion: support redis sentinel #4571

dschmidt opened this issue Jul 12, 2023 · 3 comments · May be fixed by #4623
Labels

Comments

@dschmidt
Copy link
Contributor

dschmidt commented Jul 12, 2023

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 :trollface:

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/connect-redis@0.0.18": "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
Copy link
Contributor

mifi commented Jul 12, 2023

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
Copy link
Contributor Author

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
Copy link
Member

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 dschmidt linked a pull request Aug 14, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants