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

node-redis: fetchSockets() call to sendCommand() needs update for cluster client #494

Closed
winchell opened this issue Apr 14, 2023 · 1 comment
Milestone

Comments

@winchell
Copy link
Contributor

winchell commented Apr 14, 2023

Versions:

Node: 18.15.0
socket.io: 4.6.1
socket/redis-adapter: 8.1.0
redis: 4.6.5
Redis Cluster: 7.0.5

import { createAdapter } from "@socket.io/redis-adapter"
const redis = createCluster(cluster_opts);
const redis_pub_client = redis.duplicate()
const redis_sub_client = redis.duplicate()
await Promise.all([redis.connect(), redis_pub_client.connect(), redis_sub_client.connect()])
io.adapter(createAdapter(redis_pub_client, redis_sub_client));
...
const sockets = await io.fetchSockets();

Socket communication works as expected but a call to fetchSockets() crashes. Am I misusing this?

Error:

/project/node_modules/@redis/client/dist/lib/client/RESP2/encoder.js:6
    let strings = '*' + args.length + CRLF;
                             ^

TypeError: Cannot read properties of undefined (reading 'length')
    at encodeCommand (/project/node_modules/@redis/client/dist/lib/client/RESP2/encoder.js:6:30)
    at RedisCommandsQueue.getCommandToSend (/project/node_modules/@redis/client/dist/lib/client/commands-queue.js:138:45)
    at Commander._RedisClient_tick (/project/node_modules/@redis/client/dist/lib/client/index.js:507:76)
    at Commander._RedisClient_sendCommand (/project/node_modules/@redis/client/dist/lib/client/index.js:494:82)
    at Commander.sendCommand (/project/node_modules/@redis/client/dist/lib/client/index.js:196:100)
    at /project/node_modules/@redis/client/dist/lib/cluster/index.js:129:148
    at Commander._RedisCluster_execute (/project/node_modules/@redis/client/dist/lib/cluster/index.js:221:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Seems to be landing on this https://github.com/socketio/socket.io-redis-adapter/blob/main/lib/index.ts#L918

return this.pubClient
        .sendCommand(["pubsub", "numsub", this.requestChannel])
        .then((res) => parseInt(res[1], 10));

Historically the problem was node-redis didn't support cluster PubSub. It does support it now - redis/node-redis#2373 as of 4.6.1.

However the issue now is that socket.io-redis-adapter doesn't fully support the createCluster client because fetchSockets() ends up calling sendCommand() and the createCluster client has a different function signature there. It's sendCommand(firstKey, isReadonly, args, options) as opposed to sendCommand(args, options).

@winchell winchell changed the title fetchSockets() call leads to RESP2 encoder args issue node-redis: fetchSockets() call to sendCommand() might be outdated. Definitely can't work for cluster client Apr 15, 2023
@winchell winchell changed the title node-redis: fetchSockets() call to sendCommand() might be outdated. Definitely can't work for cluster client node-redis: fetchSockets() call to sendCommand() needs update for cluster client Apr 15, 2023
winchell added a commit to winchell/socket.io-redis-adapter that referenced this issue Apr 18, 2023
darrachequesne pushed a commit that referenced this issue Apr 23, 2023
Support for Redis cluster was added in `[email protected]`.

Related: #494
@darrachequesne
Copy link
Member

Released in 8.2.0. Thanks 👍

@darrachequesne darrachequesne added this to the 8.2.0 milestone May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants