node-redis icon indicating copy to clipboard operation
node-redis copied to clipboard

Fix race condition when slots are re-calculated

Open jjsimps opened this issue 10 months ago • 2 comments

Description

I was getting an error similar to https://github.com/redis/node-redis/issues/2704.

What was happening for me is that I was creating a redis cluster client and then calling client.eval(). Since eval doesn't seem to be cluster-aware, it returned a MOVE error since it was routed to an incorrect node. This seems to cause a call to discover() to re-discover the hash slot mapping.

The bug I am trying to address is a race condition between when the internal data is cleared to when it's re-populated again. In the current implementation, there's an asynchronous command being run in-between, which means if any other command was run in between, would cause this.slots[val] of cluster-slots.ts (such as in getSlotRandomNode()) to return undefined and throw an exception.

 async #discover(rootNode?: RedisClusterClientOptions) {
        this.#resetSlots(); // !!! Reset is being called here
        const addressesInUse = new Set<string>();

        try {
            const shards = await this.#getShards(rootNode), // !!! But there's an async command here
                promises: Array<Promise<unknown>> = [],
                eagerConnect = this.#options.minimizeConnections !== true;
            for (const { from, to, master, replicas } of shards) {

This changes moves when it is cleared so that it's cleared then populated, without any async stuff inbetween.


Checklist

  • [x] Does npm test pass with this change (including linting)?
  • [x] Is the new or changed code fully tested?
  • [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

jjsimps avatar Apr 02 '24 20:04 jjsimps

Hi, can this be taken a look at? I can't seem to add people as reviewers. Thank you.

jjsimps avatar May 03 '24 00:05 jjsimps

@jjsimps sorry for the delay. I'll take a look at the start of next week. If i forget - ping me.. ;)

leibale avatar May 03 '24 14:05 leibale