Upgrade node-redis to 4.6.1 #129

eatyourgreens · 2023-01-25T12:17:59Z

Upgrade connection options to v4.
remove promisified async methods.
upgrade Redis command options to the v4 API.

eatyourgreens · 2023-01-25T14:39:11Z

Tests are passing, and this runs locally without crashing, but it's a big change that needs careful testing.

eatyourgreens · 2023-01-25T16:42:59Z

@camallen @mcbouslog would it be worth merging this and testing it on notifications-staging.zooniverse.org? It's easy enough to revert if staging breaks.

eatyourgreens · 2023-01-25T17:19:04Z

lib/pub_sub.js

+    let subscriber = this.subscribers[pattern];
+    if (!subscriber) {
+      subscriber = this.redis.sub.duplicate();
+      await subscriber.connect();
+      this.subscribers[pattern] = subscriber;
+    }


Tests fail unless I duplicate the subscription client here and create a dedicated connection for each channel.

that's not good as we'd end up with high connection counts from a single server.

what's the reasoning behind switching the p?subuscribe redis cmd to await?

We don't really need to know this function correctly resolved before we setup the emitter. I think this can move back to returning a promise and remove the async setup.

All the methods are asynchronous now. In v3, we wrap them in promisify and call punsubscribeAsync.

I’m confused though: Redis has always used promises in Sugar. We’ve never used the synchronous versions of the Redis commands, and they’re completely removed in v4.

The change to dedicated connections is straight from the v4 Readme.
https://github.com/redis/node-redis/blob/master/docs/pub-sub.md

hmm - that appears to use async to await the connection then proceed to use that connection. However we now call client.connect(); and then proceed to use that connection in tests,

sugar/lib/redis_client.js

Line 26 in dc62ddf

client.connect();

I may be completely wrong but the connection did seem to have issues for those failing tests and your change to create new client and await the connection seems to fix them. So perhaps we're not setting it up right?

Does adding the following client event event info logs to the redis client constructor help?

client.on('connect', () => console.log('connect')); client.on('ready', () => console.log('ready')); client.on('reconnecting', () => console.log('reconnecting')); client.on('error', (error) => console.log(error)); client.on('end', () => console.log('end'));

That will be the same regardless of the Promise syntax that you use: .then() or await.

Just to be clear, these two functions both return the same Promise, so reverting back to .then() alone won't fix the issue. The async/await syntax is just there to make the original nested code easier to read. So I wouldn't spend a lot of time trying to fix this with .then(). I think that's more about which JS syntax you find easier to follow.

function subscribe() { const client = redis.createClient(); return client.connect().then(function () { return client.subscribe(channel, listener).then(function () { emitter.on('message', fn); }); }); }

async function subscribe() { const client = redis.createClient(); await client.connect(); await client.subscribe(channel, listener); return emitter.on('message', fn); }

Both those functions return a pending Promise, which will resolve if the client successfully connects and subscribes to a channel.

fine - when i was testing the promise never fulfilled - instead it just hung till it hit a timeout.

Yes! That's exactly the same problem I've been running into, both yesterday and today. If I add client.on('connect', () => console.log('connected!') to the RedisClient class, then I see the client connect but the client.subscribe() command never resolves, unless I call client.duplicate() before subscribing.

Here's the terminal output. All the PubSub tests time out after 2s and fail.

i think the new client changes are related to test harness failures as it doesn't appear to impact server / app behaviours.

On 4.5, the server crashes on the first ping for me unless I add .duplicate() to each new channel subscription. 4.6 fixes that.

redis/node-redis#2345

That was a pain to diagnose.

mcbouslog · 2023-01-26T04:24:01Z

I think merging and testing on staging is a good idea 👍 . Nothing is jumping out from a cursory code review, but I think I'm more likely to notice an issue testing on staging.

camallen

I've had a go at getting the tests working via 9f7db4e

though i'm not super happy with that ^ i ended up stubbing / spying the underlying redis calls like how nock is doing it. I think nock may be interfering in the docker-compose setup as the redis server runs on the redis network host - others may be testing a different way and seeing different errors.

I think the tests are failing due to the way the redis client now connects as it's not auto. Ideally we can get the RedisClient class to have an init style function that awaits the connection result to ensure the client is connected and then we can use that redis connection correctly - i.e. the tests should run and not block on the redis subscribe / pSubscribe cmds

Happy to elaborate on this if needed - i couldn't get it working locally.

camallen · 2023-01-26T05:53:27Z

lib/pub_sub.js

+    let subscriber = this.subscribers[pattern];
+    if (!subscriber) {
+      subscriber = this.redis.sub.duplicate();
+      await subscriber.connect();
+      this.subscribers[pattern] = subscriber;
+    }


that's not good as we'd end up with high connection counts from a single server.

what's the reasoning behind switching the p?subuscribe redis cmd to await?

We don't really need to know this function correctly resolved before we setup the emitter. I think this can move back to returning a promise and remove the async setup.

eatyourgreens · 2023-01-26T07:57:18Z

I’m confused. I got the tests working yesterday.

camallen · 2023-01-26T08:31:20Z

feel free to merge this to staging but note the issues with adding new redis connections for each subscription event may impede the server.

We should be able to run 1 node-redis connection pool for each client invocation, pubsub, presence etc.

eatyourgreens · 2023-01-26T09:07:47Z

4.6.0 came out yesterday, with updated docs and examples. I'm going to update this branch. I'm still worried about performance.

eatyourgreens · 2023-01-26T09:59:20Z

UPDATE: This is the actual client error that fails the tests if you try to re-use an existing client in PubSub mode. The #unsubscribe test calls .subscribe() twice and the second call generates this error, if you use this.redis.sub directly for each subscription.

1) PubSub
       #unsubscribe
         should only unsubscribe from redis when no subscribers remain:
     Error: Cannot send commands in PubSub mode
      at RedisCommandsQueue.addCommand (node_modules/@redis/client/dist/lib/client/commands-queue.js:72:35)
      at RedisSocket.socketInitiator (node_modules/@redis/client/dist/lib/client/index.js:334:81)
      at RedisSocket._RedisSocket_connect (node_modules/@redis/client/dist/lib/client/socket.js:136:81)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at async Commander.connect (node_modules/@redis/client/dist/lib/client/index.js:173:9)
      at async PubSub.subscribe (lib/pub_sub.js:36:9)
      at async Promise.all (index 0)

- Upgrade connection options to v4. - remove promisified async methods. - upgrade Redis command options to the v4 API.

- Remove `client.connect()` from the base client class. New clients must call `client.connect()` before they can be used. - Rename the PubSub channel clients.

eatyourgreens · 2023-01-26T11:01:03Z

I've upgraded to 4.6.1 and also refactored the RedisClient class. New clients must explicitly connect now.

const client = new RedisClient();
await client.connect();
client.publish(channel, message);

or

const client = new RedisClient();
client.connect().then(function doStuff() {
  client.publish(channel, message);
});

eatyourgreens · 2023-01-26T11:21:12Z

Tests pass in 4.6.1 with a single subscriber client connection so 🤷‍♂️ . I'll push those changes up.

EDIT: I think the docs mean that PubSub should have a dedicated connection, which can only run a limited command set. If you try to run any other command, PubSub errors with cannot send commands in PubSub mode.

eatyourgreens · 2023-01-26T11:24:59Z

It was this issue, fixed in 4.6.
redis/node-redis#2345

eatyourgreens · 2023-01-26T11:26:04Z

I'm feeling a lot better about this now that it doesn't use .duplicate() for PubSub.

Dynamic methods can be a security vulnerability, so avoid them where possible.

eatyourgreens · 2023-01-26T13:27:53Z

Note to self: wrap client connections in isOpen to make PubSub more robust. That might be a new feature in v4.

eatyourgreens · 2023-01-26T13:30:09Z

I’ve also removed dynamic method invocation, which allowed the caller to run any Redis command without restriction. That’s a security vulnerability in production, though not one that I think is exploitable. At least, I don’t think we pass untrusted input as arguments to those functions.

eatyourgreens · 2023-01-26T14:32:28Z

I'm running this now, with three different tabs open to http://local.zooniverse.org:2999/ in Firefox, all connected to the local Sugar server. Connections seem to be staying up without dropping.

I've added client.isOpen checks to PubSub, so the redis client should reconnect if the connection does drop.

EDIT: local clients have been connected for > 10 minutes now, without any errors or server problems.

eatyourgreens · 2023-01-26T14:45:46Z

I think I'm a lot happier with this now:

node-redis has been bumped from 4.5 to 4.6.
PubSub uses dedicated subscriber and publisher clients to subscribe/unsubscribe and to publish to channels. .duplicate() has been removed.
Responsibility for opening a connection now lies with individual instances of RedisClient.
Redis commands aren't passed as function arguments but instead explicitly declared in the code.

eatyourgreens · 2023-01-26T15:51:59Z

I'm going to deploy this so that we can try it out in staging.

eatyourgreens · 2023-01-26T16:26:30Z

Staging crashed when I deployed this. 😞

eatyourgreens · 2023-01-26T16:52:18Z

I guess I can't re-open a merged PR. I'll move the changes here to a new PR.

eatyourgreens · 2023-01-26T18:08:39Z

PR re-opened at #133. First thing to figure out, I guess, is why did this crash on staging? I think the only major difference between the staging deploy and local dev is that staging would use the TLS settings and password. I'm not 100% sure that I got the changes in the Redis client connection options right.

eatyourgreens mentioned this pull request Jan 25, 2023

handle fatal errors that cause pod restarts #128

Closed

eatyourgreens force-pushed the redis-4 branch 3 times, most recently from 0e2803e to 16e9d3b Compare January 25, 2023 14:29

eatyourgreens commented Jan 25, 2023

View reviewed changes

eatyourgreens force-pushed the redis-4 branch 3 times, most recently from 378814c to dc62ddf Compare January 25, 2023 17:39

camallen reviewed Jan 26, 2023

View reviewed changes

eatyourgreens added 4 commits January 26, 2023 10:36

Upgrade node-redis to 4.5.1

d93de73

- Upgrade connection options to v4. - remove promisified async methods. - upgrade Redis command options to the v4 API.

Cleanup PubSub with async/await

4f791d3

Refactor PubSub connections

ca1a383

- Remove `client.connect()` from the base client class. New clients must call `client.connect()` before they can be used. - Rename the PubSub channel clients.

Bump to redis 4.6.0

df05018

eatyourgreens force-pushed the redis-4 branch from cd62e98 to df05018 Compare January 26, 2023 10:56

eatyourgreens marked this pull request as ready for review January 26, 2023 11:25

eatyourgreens force-pushed the redis-4 branch from 53b4946 to c5b01e2 Compare January 26, 2023 11:42

eatyourgreens changed the title ~~Upgrade node-redis to 4.5.1~~ Upgrade node-redis to 4.6.1 Jan 26, 2023

eatyourgreens added 2 commits January 26, 2023 11:54

Use a single PubSub subscriber

608e749

Don't use dynamic methods in presence.js

2179494

Dynamic methods can be a security vulnerability, so avoid them where possible.

eatyourgreens force-pushed the redis-4 branch from c5b01e2 to 2179494 Compare January 26, 2023 11:54

Wrap PubSub connections in client.isOpen checks

20f0cb1

eatyourgreens merged commit f228528 into master Jan 26, 2023

eatyourgreens deleted the redis-4 branch January 26, 2023 15:52

eatyourgreens restored the redis-4 branch January 26, 2023 16:25

eatyourgreens deleted the redis-4 branch January 26, 2023 16:27

eatyourgreens restored the redis-4 branch January 26, 2023 16:47

eatyourgreens mentioned this pull request Jan 26, 2023

Upgrade node-redis to 4.6.1 #133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade node-redis to 4.6.1 #129

Upgrade node-redis to 4.6.1 #129

eatyourgreens commented Jan 25, 2023

eatyourgreens commented Jan 25, 2023

eatyourgreens commented Jan 25, 2023

eatyourgreens Jan 25, 2023

camallen Jan 26, 2023

eatyourgreens Jan 26, 2023

eatyourgreens Jan 26, 2023

camallen Jan 26, 2023

eatyourgreens Jan 26, 2023

eatyourgreens Jan 26, 2023

eatyourgreens Jan 26, 2023

eatyourgreens Jan 26, 2023

eatyourgreens Jan 26, 2023

mcbouslog commented Jan 26, 2023

camallen left a comment

camallen Jan 26, 2023

eatyourgreens commented Jan 26, 2023

camallen commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023 •

edited

Loading

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023 •

edited

Loading

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

Upgrade node-redis to 4.6.1 #129

Upgrade node-redis to 4.6.1 #129

Conversation

eatyourgreens commented Jan 25, 2023

eatyourgreens commented Jan 25, 2023

eatyourgreens commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcbouslog commented Jan 26, 2023

camallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eatyourgreens commented Jan 26, 2023

camallen commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023 • edited Loading

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023 • edited Loading

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023

eatyourgreens commented Jan 26, 2023 •

edited

Loading

eatyourgreens commented Jan 26, 2023 •

edited

Loading