RedisStore: args.length: Cannot read properties of undefined (reading length).

[thxrben]

Description

I cannot instantiate RedisStore objects due to a "undefined" problem.:

const { RedisStore } = require("rate-limit-redis");
const rateLimit = require("express-rate-limit");

[...]

 const apiLimiter = rateLimit({
    windowMs: 1 * 60 * 1000, // 1 minute
    max: 10, // Limit each IP to 50 requests per `window` (here, per 1 minute)
    standardHeaders: false, // Return rate limit info in the `RateLimit-*` headers
    legacyHeaders: false, // Disable the `X-RateLimit-*` headers
    store: new RedisStore({
      sendCommand: (...args) => redis.sendCommand(args),
    }),
  }); 

will return:

/home/thorben/Dokumente/Development/XXX/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 (/home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/client/RESP2/encoder.js:6:30)
    at RedisCommandsQueue.getCommandToSend (/home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/client/commands-queue.js:138:45)
    at Commander._RedisClient_tick (/home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/client/index.js:535:76)
    at Commander._RedisClient_sendCommand (/home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/client/index.js:522:82)
    at Commander.sendCommand (/home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/client/index.js:193:100)
    at /home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/cluster/index.js:129:148
    at Commander._RedisCluster_execute (/home/thorben/Dokumente/Development/XXX/node_modules/@redis/client/dist/lib/cluster/index.js:220:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Library version

4.2.0

Node version

18.20.3

Typescript version (if you are using it)

No response

Module system

CommonJS

[thxrben]

I have to mention:
The redis object I am using at .sendCommand is created using createCluster() from node-redis.
There is no mention of this in your docs, is a cluster supported?
Should be the same operations, I think (never looked this up) that the cluster is implementing the same functions as a client in node-redis.

[gamemaker1]

Hi @Gandalf1783,

Apologies for the late reply.

Looking through the source code for the node-redis cluster class, it looks like the function signature for sendCommand is different from a normal client:

https://github.com/redis/node-redis/blob/d5355d43275fedf1b2afc4db8a926f72b05f79c5/packages/client/lib/cluster/index.ts#L168-L179

It asks for two additional arguments: firstKey as well as isReadonly. I think firstKey can be obtained by using the static function extractFirstKey (or you can pass undefined), defined on the RedisCluster class, and you can pass false to isReadonly.

Thus, the final code should look something like this:

 // [ ... ]

 const apiLimiter = rateLimit({
    windowMs: 1 * 60 * 1000, // 1 minute
    max: 10, // Limit each IP to 50 requests per `window` (here, per 1 minute)
    standardHeaders: false, // Return rate limit info in the `RateLimit-*` headers
    legacyHeaders: false, // Disable the `X-RateLimit-*` headers
    store: new RedisStore({
      sendCommand: (...args) => cluster.sendCommand(undefined, false, args),
    }),
  }); 

Hope this helps.

[thxrben]

Hello gamemaker!

Thank you for your reply and for looking this up.

In afterthought, I could've found this myself.

Due to being sick, I might be able to test it in the next few days, if at all.

However, I think this solves the issue.
If I got time tomorrow, I can try the fix if my condition doesnt get worse :)

Thank you again and you will hear from me soon!

[gamemaker1]

Happy to help :)

I am doubtful about whether passing undefined for firstKey will work or not. I couldn't setup a redis cluster on my machine with docker to test it myself either, since the servers keeps giving me connection reset errors.

Please do take care of your health first, and let me know what works once you get the time.

[thxrben]

Soo, the results are in!
While your proposed code does not fail on starting / construction of the RedisStore, it results for me in this error:
"Error: NOSCRIPT No matching script. Please use EVAL.".

I will now try what I can do for firstKey, maybe this has something to do with it (being undefined)?

[gamemaker1]

"Error: NOSCRIPT No matching script. Please use EVAL.".

Ah that's most likely because it's storing the script on initialisation in one server and calling it later on another server :/

I will now try what I can do for firstKey, maybe this has something to do with it (being undefined)?

Yea maybe firstKey is what allows it to know which server the key for the operation is stored in? I'm not sure how it will handle the script though.

---

Maybe cluster support is not really feasible the way the library is currently written. As a workaround, you could define a new redis client with only one of the servers and use that for rate limiting.

[nfriedly]

It looks like node-redis has a way to define scripts when calling createCluster, but I think it would take some shoehorning to make that work with this lib.

[nfriedly]

I think the real solution is probably for rate-limit-redis to include a default redis client, make sure it supports clustering, and just keep the old sendCommand around for non-clustering backwards compatibility.

[Davrosh]

I wanted to comment on this issue, seeing as I ran into virtually the same problem as @Gandalf1783 , only a month after.

I also followed: @gamemaker1 's suggestion and used undefined as the first paramater but as stated above that did not work so well: #207 (comment)

Their other comment also helped me: #207 (comment)
I basically tried going to the sendCommand function which this library allows us to customize, catching the script loading that's happening there on init and sending that to every master node in the cluster manually.
Then, since firstKey is apparently used by the node-redis client to determine which node to send the command to, I had to assume the key would be at a fixed position each time (when running EVALSHA, for instance, the key is listed out further in the list of arguments).

I tried to test the "basic" functionality of the library, that is to keep track of total hits for incloming ip addresses and maintain their ttl's.
Not sure how else this library can be used but what I've tested seems to be working for me locally so far...

Please take this workaround with a grain of salt...

Changed the sendCommand function when creating the RedisStore to this:

sendCommand: (...args) => {
    const commandName = args[0]
    if (commandName === 'SCRIPT') {
        let sendCommandResult
        for (let i = 0; i < redisClient.masters.length; i++) {
            const result = redisClient.masters[i].client?.sendCommand(args)
            if (i === 0) {
                sendCommandResult = result
            }
        }
        return sendCommandResult
    }
    const firstArg = commandName === 'EVALSHA' ? args[3] : args[1]
    return redisClient.sendCommand(firstArg, false, args)
}

I'm assuming this function is called with await, since returning the raw promise here somehow works.
I just looked at this in the readme and filled the rest from there:

      // Redis store configuration
      store: new RedisStore({
      sendCommand: (...args: string[]) => client.sendCommand(args),
      }),

I also hardcoded the second argument for the client's sendCommand to be false (it's a boolean and its name is apparently readonly). I didn't try optimizing this part since I didn't think this rate limiter would be using many readonly Redis commands from what I've seen from it -- likely it's mostly relying on Lua scripts.

The downside of course is that if the cluster is ever scaled up to have more nodes, the server would have to be restarted in order for the constructor to be initialized and run the script on the new nodes as well. A full solution would have to account for this case as well.

With regards to trying to fix this, as @nfriedly already pointed out, there is a way to load Lua scripts to the client if you are to go with maintaining a full client.
I just wanted to add that I was looking at the node-redis repo as well while searching for an answer to the above and I came across this issue that apparently says there is a problem with using evalsha scripts in cluster mode (I don't think that scripts loaded with sendCommand are affected). There is an open PR but at the time of writing it's not merged yet: redis/node-redis#2811 (reference)
So to whoever winds up in charge of adding the built-in Redis client in the future, if that happens, it's worth keeping this in mind. I just thought it might help someone.

For the time being, maybe this can be further tested and added to the documentation? Or perhaps support for node-redis with Redis cluster can be added in a backwards-compatible way?