From 0d2a4162c277310710d68f698d5d4699abbc33de Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 21:47:50 +0100 Subject: [PATCH 01/20] Tell Redis cluster to disable protected mode before running tests --- test/cluster/docker/main.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/cluster/docker/main.sh b/test/cluster/docker/main.sh index f5f75df7..cf91dead 100755 --- a/test/cluster/docker/main.sh +++ b/test/cluster/docker/main.sh @@ -1,4 +1,16 @@ -docker run -e "INITIAL_PORT=30000" -e "IP=0.0.0.0" -p 30000-30005:30000-30005 grokzen/redis-cluster:latest & +#!/bin/bash + +set -euo pipefail + +docker run --rm --name redis-cluster-ioredis-test -e "INITIAL_PORT=30000" -e "IP=0.0.0.0" -p 30000-30005:30000-30005 grokzen/redis-cluster:latest & +trap 'docker stop redis-cluster-ioredis-test' EXIT + npm install + sleep 15 + +for port in {30000..30005}; do + docker exec redis-cluster-ioredis-test /bin/bash -c "redis-cli -p $port CONFIG SET protected-mode no" +done + npm run test:js:cluster || npm run test:js:cluster || npm run test:js:cluster From 5385c8e191833f9966e1b1acd690f7b5abdf99f3 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 21:48:40 +0100 Subject: [PATCH 02/20] Try to enable Redis cluster tests on CI --- .github/workflows/test.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 040df430..3abe3fea 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -35,12 +35,12 @@ jobs: flag-name: node-${{matrix.node}} parallel: true - # test-cluster: - # runs-on: ubuntu-latest - # steps: - # - uses: actions/checkout@v2 - # - name: Build and test cluster - # run: bash test/cluster/docker/main.sh + test-cluster: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Build and test cluster + run: bash test/cluster/docker/main.sh code-coverage: needs: test From f67d73ab7e20ea94607749d31c57b7bf26f71e8a Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 21:54:17 +0100 Subject: [PATCH 03/20] Add a failing test around Redis cluster disconnection logic --- test/cluster/basic.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/cluster/basic.ts b/test/cluster/basic.ts index 98d5dafe..deb9882e 100644 --- a/test/cluster/basic.ts +++ b/test/cluster/basic.ts @@ -148,4 +148,25 @@ describe("cluster", () => { expect(await cluster2.get("prefix:foo")).to.eql("bar"); }); }); + + describe("disconnect and connect again", () => { + it("works 20 times in a row", async () => { + const cluster = new Cluster([{ host: "127.0.0.1", port: masters[0] }]); + + for (let i = 1; i <= 20; i++) { + await cluster.set("foo", `bar${i}`); + + const endPromise = new Promise((resolve) => + cluster.once("end", resolve) + ); + await cluster.quit(); + cluster.disconnect(); + await endPromise; + + cluster.connect(); + expect(await cluster.get("foo")).to.equal(`bar${i}`); + await cluster.del("foo"); + } + }); + }); }); From 66efb1a8e8dadfe9444670912099dbea5c93856f Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 21:58:38 +0100 Subject: [PATCH 04/20] Rename function parameter --- lib/cluster/ConnectionPool.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index dbab62c9..f5362f61 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -39,14 +39,14 @@ export default class ConnectionPool extends EventEmitter { /** * Find or create a connection to the node */ - findOrCreate(node: RedisOptions, readOnly = false): Redis { - const key = getNodeKey(node); + findOrCreate(redisOptions: RedisOptions, readOnly = false): Redis { + const key = getNodeKey(redisOptions); readOnly = Boolean(readOnly); if (this.specifiedOptions[key]) { - Object.assign(node, this.specifiedOptions[key]); + Object.assign(redisOptions, this.specifiedOptions[key]); } else { - this.specifiedOptions[key] = node; + this.specifiedOptions[key] = redisOptions; } let redis: Redis; @@ -79,7 +79,7 @@ export default class ConnectionPool extends EventEmitter { enableOfflineQueue: true, readOnly: readOnly, }, - node, + redisOptions, this.redisOptions, { lazyConnect: true } ) From f0cadc5a80a05335e3b12faa362b4931b31b837f Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:10:44 +0100 Subject: [PATCH 05/20] Turn node error listener into an arrow function so that `this` points to the connection pool instance --- lib/cluster/ConnectionPool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index f5362f61..815be530 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -97,7 +97,7 @@ export default class ConnectionPool extends EventEmitter { this.emit("+node", redis, key); - redis.on("error", function (error) { + redis.on("error", (error) => { this.emit("nodeError", error, key); }); } From 49e9edd5e21e3438386e76c8bdeaea8b52d1f0da Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:12:08 +0100 Subject: [PATCH 06/20] Extract node listeners into separate constants --- lib/cluster/ConnectionPool.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index 815be530..1dc69276 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -87,19 +87,21 @@ export default class ConnectionPool extends EventEmitter { this.nodes.all[key] = redis; this.nodes[readOnly ? "slave" : "master"][key] = redis; - redis.once("end", () => { + const endListener = () => { this.removeNode(key); this.emit("-node", redis, key); if (!Object.keys(this.nodes.all).length) { this.emit("drain"); } - }); + }; + redis.once("end", endListener); this.emit("+node", redis, key); - redis.on("error", (error) => { + const errorListener = (error: unknown) => { this.emit("nodeError", error, key); - }); + }; + redis.on("error", errorListener); } return redis; From dee2623978df85f91bd2330493b4f836c2fcbf5e Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:20:51 +0100 Subject: [PATCH 07/20] Keep track of listeners along with each Redis client --- lib/cluster/ConnectionPool.ts | 50 ++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index 1dc69276..a2dc1d78 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -7,9 +7,15 @@ const debug = Debug("cluster:connectionPool"); type NODE_TYPE = "all" | "master" | "slave"; +type Node = { + redis: Redis; + endListener: () => void; + errorListener: (error: unknown) => void; +}; + export default class ConnectionPool extends EventEmitter { // master + slave = all - private nodes: { [key in NODE_TYPE]: { [key: string]: Redis } } = { + private nodes: { [key in NODE_TYPE]: { [key: string]: Node } } = { all: {}, master: {}, slave: {}, @@ -23,23 +29,23 @@ export default class ConnectionPool extends EventEmitter { getNodes(role: NodeRole = "all"): Redis[] { const nodes = this.nodes[role]; - return Object.keys(nodes).map((key) => nodes[key]); + return Object.keys(nodes).map((key) => nodes[key].redis); } getInstanceByKey(key: NodeKey): Redis { - return this.nodes.all[key]; + return this.nodes.all[key].redis; } getSampleInstance(role: NodeRole): Redis { const keys = Object.keys(this.nodes[role]); const sampleKey = sample(keys); - return this.nodes[role][sampleKey]; + return this.nodes[role][sampleKey].redis; } /** * Find or create a connection to the node */ - findOrCreate(redisOptions: RedisOptions, readOnly = false): Redis { + findOrCreate(redisOptions: RedisOptions, readOnly = false): Node { const key = getNodeKey(redisOptions); readOnly = Boolean(readOnly); @@ -49,24 +55,24 @@ export default class ConnectionPool extends EventEmitter { this.specifiedOptions[key] = redisOptions; } - let redis: Redis; + let node: Node; if (this.nodes.all[key]) { - redis = this.nodes.all[key]; - if (redis.options.readOnly !== readOnly) { - redis.options.readOnly = readOnly; + node = this.nodes.all[key]; + if (node.redis.options.readOnly !== readOnly) { + node.redis.options.readOnly = readOnly; debug("Change role of %s to %s", key, readOnly ? "slave" : "master"); - redis[readOnly ? "readonly" : "readwrite"]().catch(noop); + node.redis[readOnly ? "readonly" : "readwrite"]().catch(noop); if (readOnly) { delete this.nodes.master[key]; - this.nodes.slave[key] = redis; + this.nodes.slave[key] = node; } else { delete this.nodes.slave[key]; - this.nodes.master[key] = redis; + this.nodes.master[key] = node; } } } else { debug("Connecting to %s as %s", key, readOnly ? "slave" : "master"); - redis = new Redis( + const redis = new Redis( defaults( { // Never try to reconnect when a node is lose, @@ -84,9 +90,6 @@ export default class ConnectionPool extends EventEmitter { { lazyConnect: true } ) ); - this.nodes.all[key] = redis; - this.nodes[readOnly ? "slave" : "master"][key] = redis; - const endListener = () => { this.removeNode(key); this.emit("-node", redis, key); @@ -94,17 +97,22 @@ export default class ConnectionPool extends EventEmitter { this.emit("drain"); } }; + const errorListener = (error: unknown) => { + this.emit("nodeError", error, key); + }; + node = { redis, endListener, errorListener }; + + this.nodes.all[key] = node; + this.nodes[readOnly ? "slave" : "master"][key] = node; + redis.once("end", endListener); this.emit("+node", redis, key); - const errorListener = (error: unknown) => { - this.emit("nodeError", error, key); - }; redis.on("error", errorListener); } - return redis; + return node; } /** @@ -127,7 +135,7 @@ export default class ConnectionPool extends EventEmitter { Object.keys(this.nodes.all).forEach((key) => { if (!newNodes[key]) { debug("Disconnect %s because the node does not hold any slot", key); - this.nodes.all[key].disconnect(); + this.nodes.all[key].redis.disconnect(); this.removeNode(key); } }); From 4d519a67317a61cf444e2f26bfb3a63ec7944c0b Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:23:40 +0100 Subject: [PATCH 08/20] Remove node listeners when the node is being removed --- lib/cluster/ConnectionPool.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index a2dc1d78..5ce58fa0 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -150,8 +150,11 @@ export default class ConnectionPool extends EventEmitter { */ private removeNode(key: string): void { const { nodes } = this; - if (nodes.all[key]) { + const node = nodes.all[key]; + if (node) { debug("Remove %s from the pool", key); + node.redis.removeListener("end", node.endListener); + node.redis.removeListener("error", node.errorListener); delete nodes.all[key]; } delete nodes.master[key]; From ed3e190a6ba9f2501d3418dc9b5e82e8ebcb9251 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:25:16 +0100 Subject: [PATCH 09/20] Emit node removal events whenever a node is removed --- lib/cluster/ConnectionPool.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index 5ce58fa0..5677eb49 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -92,10 +92,6 @@ export default class ConnectionPool extends EventEmitter { ); const endListener = () => { this.removeNode(key); - this.emit("-node", redis, key); - if (!Object.keys(this.nodes.all).length) { - this.emit("drain"); - } }; const errorListener = (error: unknown) => { this.emit("nodeError", error, key); @@ -156,8 +152,13 @@ export default class ConnectionPool extends EventEmitter { node.redis.removeListener("end", node.endListener); node.redis.removeListener("error", node.errorListener); delete nodes.all[key]; + delete nodes.master[key]; + delete nodes.slave[key]; + + this.emit("-node", node.redis, key); + if (!Object.keys(nodes.all).length) { + this.emit("drain"); + } } - delete nodes.master[key]; - delete nodes.slave[key]; } } From cd3b74a0abfd3d74bfa2c3517b3c8205bef5b8ee Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:25:47 +0100 Subject: [PATCH 10/20] When resetting, add nodes before removing old ones --- lib/cluster/ConnectionPool.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index 5677eb49..bcf426e9 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -128,6 +128,10 @@ export default class ConnectionPool extends EventEmitter { } }); + Object.keys(newNodes).forEach((key) => { + const node = newNodes[key]; + this.findOrCreate(node, node.readOnly); + }); Object.keys(this.nodes.all).forEach((key) => { if (!newNodes[key]) { debug("Disconnect %s because the node does not hold any slot", key); @@ -135,10 +139,6 @@ export default class ConnectionPool extends EventEmitter { this.removeNode(key); } }); - Object.keys(newNodes).forEach((key) => { - const node = newNodes[key]; - this.findOrCreate(node, node.readOnly); - }); } /** From 2b47e9449c768a6637be5c48c946f96edc40cb52 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:26:58 +0100 Subject: [PATCH 11/20] Rename Node type to NodeRecord for clarity --- lib/cluster/ConnectionPool.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index bcf426e9..141c2d29 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -7,7 +7,7 @@ const debug = Debug("cluster:connectionPool"); type NODE_TYPE = "all" | "master" | "slave"; -type Node = { +type NodeRecord = { redis: Redis; endListener: () => void; errorListener: (error: unknown) => void; @@ -15,7 +15,7 @@ type Node = { export default class ConnectionPool extends EventEmitter { // master + slave = all - private nodes: { [key in NODE_TYPE]: { [key: string]: Node } } = { + private nodes: { [key in NODE_TYPE]: { [key: string]: NodeRecord } } = { all: {}, master: {}, slave: {}, @@ -45,7 +45,7 @@ export default class ConnectionPool extends EventEmitter { /** * Find or create a connection to the node */ - findOrCreate(redisOptions: RedisOptions, readOnly = false): Node { + findOrCreate(redisOptions: RedisOptions, readOnly = false): NodeRecord { const key = getNodeKey(redisOptions); readOnly = Boolean(readOnly); @@ -55,7 +55,7 @@ export default class ConnectionPool extends EventEmitter { this.specifiedOptions[key] = redisOptions; } - let node: Node; + let node: NodeRecord; if (this.nodes.all[key]) { node = this.nodes.all[key]; if (node.redis.options.readOnly !== readOnly) { From e8351857e3299b430269d10a35335aea223f89cc Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 22:31:09 +0100 Subject: [PATCH 12/20] Also rename the field holding node records --- lib/cluster/ConnectionPool.ts | 44 +++++++++++++++++------------------ 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index 141c2d29..bb046537 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -15,7 +15,7 @@ type NodeRecord = { export default class ConnectionPool extends EventEmitter { // master + slave = all - private nodes: { [key in NODE_TYPE]: { [key: string]: NodeRecord } } = { + private nodeRecords: { [key in NODE_TYPE]: { [key: string]: NodeRecord } } = { all: {}, master: {}, slave: {}, @@ -28,18 +28,18 @@ export default class ConnectionPool extends EventEmitter { } getNodes(role: NodeRole = "all"): Redis[] { - const nodes = this.nodes[role]; - return Object.keys(nodes).map((key) => nodes[key].redis); + const nodeRecords = this.nodeRecords[role]; + return Object.keys(nodeRecords).map((key) => nodeRecords[key].redis); } getInstanceByKey(key: NodeKey): Redis { - return this.nodes.all[key].redis; + return this.nodeRecords.all[key].redis; } getSampleInstance(role: NodeRole): Redis { - const keys = Object.keys(this.nodes[role]); + const keys = Object.keys(this.nodeRecords[role]); const sampleKey = sample(keys); - return this.nodes[role][sampleKey].redis; + return this.nodeRecords[role][sampleKey].redis; } /** @@ -56,18 +56,18 @@ export default class ConnectionPool extends EventEmitter { } let node: NodeRecord; - if (this.nodes.all[key]) { - node = this.nodes.all[key]; + if (this.nodeRecords.all[key]) { + node = this.nodeRecords.all[key]; if (node.redis.options.readOnly !== readOnly) { node.redis.options.readOnly = readOnly; debug("Change role of %s to %s", key, readOnly ? "slave" : "master"); node.redis[readOnly ? "readonly" : "readwrite"]().catch(noop); if (readOnly) { - delete this.nodes.master[key]; - this.nodes.slave[key] = node; + delete this.nodeRecords.master[key]; + this.nodeRecords.slave[key] = node; } else { - delete this.nodes.slave[key]; - this.nodes.master[key] = node; + delete this.nodeRecords.slave[key]; + this.nodeRecords.master[key] = node; } } } else { @@ -98,8 +98,8 @@ export default class ConnectionPool extends EventEmitter { }; node = { redis, endListener, errorListener }; - this.nodes.all[key] = node; - this.nodes[readOnly ? "slave" : "master"][key] = node; + this.nodeRecords.all[key] = node; + this.nodeRecords[readOnly ? "slave" : "master"][key] = node; redis.once("end", endListener); @@ -132,10 +132,10 @@ export default class ConnectionPool extends EventEmitter { const node = newNodes[key]; this.findOrCreate(node, node.readOnly); }); - Object.keys(this.nodes.all).forEach((key) => { + Object.keys(this.nodeRecords.all).forEach((key) => { if (!newNodes[key]) { debug("Disconnect %s because the node does not hold any slot", key); - this.nodes.all[key].redis.disconnect(); + this.nodeRecords.all[key].redis.disconnect(); this.removeNode(key); } }); @@ -145,18 +145,18 @@ export default class ConnectionPool extends EventEmitter { * Remove a node from the pool. */ private removeNode(key: string): void { - const { nodes } = this; - const node = nodes.all[key]; + const { nodeRecords } = this; + const node = nodeRecords.all[key]; if (node) { debug("Remove %s from the pool", key); node.redis.removeListener("end", node.endListener); node.redis.removeListener("error", node.errorListener); - delete nodes.all[key]; - delete nodes.master[key]; - delete nodes.slave[key]; + delete nodeRecords.all[key]; + delete nodeRecords.master[key]; + delete nodeRecords.slave[key]; this.emit("-node", node.redis, key); - if (!Object.keys(nodes.all).length) { + if (!Object.keys(nodeRecords.all).length) { this.emit("drain"); } } From eb2d07c2a35a2611a2a003c36ce64d0436086730 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 23:05:08 +0100 Subject: [PATCH 13/20] Rename variable to nodeRecord --- lib/cluster/ConnectionPool.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index bb046537..a064b18c 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -55,19 +55,19 @@ export default class ConnectionPool extends EventEmitter { this.specifiedOptions[key] = redisOptions; } - let node: NodeRecord; + let nodeRecord: NodeRecord; if (this.nodeRecords.all[key]) { - node = this.nodeRecords.all[key]; - if (node.redis.options.readOnly !== readOnly) { - node.redis.options.readOnly = readOnly; + nodeRecord = this.nodeRecords.all[key]; + if (nodeRecord.redis.options.readOnly !== readOnly) { + nodeRecord.redis.options.readOnly = readOnly; debug("Change role of %s to %s", key, readOnly ? "slave" : "master"); - node.redis[readOnly ? "readonly" : "readwrite"]().catch(noop); + nodeRecord.redis[readOnly ? "readonly" : "readwrite"]().catch(noop); if (readOnly) { delete this.nodeRecords.master[key]; - this.nodeRecords.slave[key] = node; + this.nodeRecords.slave[key] = nodeRecord; } else { delete this.nodeRecords.slave[key]; - this.nodeRecords.master[key] = node; + this.nodeRecords.master[key] = nodeRecord; } } } else { @@ -96,10 +96,10 @@ export default class ConnectionPool extends EventEmitter { const errorListener = (error: unknown) => { this.emit("nodeError", error, key); }; - node = { redis, endListener, errorListener }; + nodeRecord = { redis, endListener, errorListener }; - this.nodeRecords.all[key] = node; - this.nodeRecords[readOnly ? "slave" : "master"][key] = node; + this.nodeRecords.all[key] = nodeRecord; + this.nodeRecords[readOnly ? "slave" : "master"][key] = nodeRecord; redis.once("end", endListener); @@ -108,7 +108,7 @@ export default class ConnectionPool extends EventEmitter { redis.on("error", errorListener); } - return node; + return nodeRecord; } /** From 62427a8edbf82061010d94ef7926d580f9d5b2be Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Wed, 6 Mar 2024 23:33:16 +0100 Subject: [PATCH 14/20] Rename another variable to nodeRecord --- lib/cluster/ConnectionPool.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index a064b18c..dea1be14 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -146,16 +146,16 @@ export default class ConnectionPool extends EventEmitter { */ private removeNode(key: string): void { const { nodeRecords } = this; - const node = nodeRecords.all[key]; - if (node) { + const nodeRecord = nodeRecords.all[key]; + if (nodeRecord) { debug("Remove %s from the pool", key); - node.redis.removeListener("end", node.endListener); - node.redis.removeListener("error", node.errorListener); + nodeRecord.redis.removeListener("end", nodeRecord.endListener); + nodeRecord.redis.removeListener("error", nodeRecord.errorListener); delete nodeRecords.all[key]; delete nodeRecords.master[key]; delete nodeRecords.slave[key]; - this.emit("-node", node.redis, key); + this.emit("-node", nodeRecord.redis, key); if (!Object.keys(nodeRecords.all).length) { this.emit("drain"); } From 2979176acaf056b8f9e642a93ef201c3862e9edc Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Fri, 8 Mar 2024 21:16:52 +0100 Subject: [PATCH 15/20] Fix a reference to connection pool nodes --- lib/Pipeline.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Pipeline.ts b/lib/Pipeline.ts index cf128fbf..32a4d639 100644 --- a/lib/Pipeline.ts +++ b/lib/Pipeline.ts @@ -343,7 +343,7 @@ Pipeline.prototype.exec = function (callback: Callback): Promise> { if (_this.isCluster) { node = { slot: pipelineSlot, - redis: _this.redis.connectionPool.nodes.all[_this.preferKey], + redis: _this.redis.connectionPool.getNodes()[_this.preferKey], }; } From 4d4ba69fbc7946e8dfed5691c3eacb7a4df2ca75 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 25 Mar 2024 02:08:17 +0100 Subject: [PATCH 16/20] Do not fail when retrieving a node by non-existing key --- lib/cluster/ConnectionPool.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cluster/ConnectionPool.ts b/lib/cluster/ConnectionPool.ts index dea1be14..5a117bad 100644 --- a/lib/cluster/ConnectionPool.ts +++ b/lib/cluster/ConnectionPool.ts @@ -33,7 +33,7 @@ export default class ConnectionPool extends EventEmitter { } getInstanceByKey(key: NodeKey): Redis { - return this.nodeRecords.all[key].redis; + return this.nodeRecords.all[key]?.redis; } getSampleInstance(role: NodeRole): Redis { From 5a490c1639d94a1e36dbe2c54620488d3c566bd3 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Thu, 15 Aug 2024 22:40:56 +0200 Subject: [PATCH 17/20] Revert "Fix a reference to connection pool nodes" This reverts commit 2979176acaf056b8f9e642a93ef201c3862e9edc. --- lib/Pipeline.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Pipeline.ts b/lib/Pipeline.ts index 32a4d639..cf128fbf 100644 --- a/lib/Pipeline.ts +++ b/lib/Pipeline.ts @@ -343,7 +343,7 @@ Pipeline.prototype.exec = function (callback: Callback): Promise> { if (_this.isCluster) { node = { slot: pipelineSlot, - redis: _this.redis.connectionPool.getNodes()[_this.preferKey], + redis: _this.redis.connectionPool.nodes.all[_this.preferKey], }; } From d67b1b6507a68c74c4ef6de47bc11eb7afd586d3 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Thu, 15 Aug 2024 22:47:00 +0200 Subject: [PATCH 18/20] Fix a reference to connection pool nodes, this time a bit more correctly --- lib/Pipeline.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Pipeline.ts b/lib/Pipeline.ts index cf128fbf..7be5cb40 100644 --- a/lib/Pipeline.ts +++ b/lib/Pipeline.ts @@ -343,7 +343,7 @@ Pipeline.prototype.exec = function (callback: Callback): Promise> { if (_this.isCluster) { node = { slot: pipelineSlot, - redis: _this.redis.connectionPool.nodes.all[_this.preferKey], + redis: _this.redis.connectionPool.getInstanceByKey(_this.preferKey), }; } From d779cb465ef00a508b5c0db94d907b48bf5f91d5 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Thu, 15 Aug 2024 22:47:09 +0200 Subject: [PATCH 19/20] Add a valid slots table to mock server in tests that expect to be able to connect using the Cluster client --- test/functional/cluster/connect.ts | 7 ++++++- test/functional/cluster/disconnection.ts | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/test/functional/cluster/connect.ts b/test/functional/cluster/connect.ts index a2346f23..c0caf114 100644 --- a/test/functional/cluster/connect.ts +++ b/test/functional/cluster/connect.ts @@ -391,7 +391,12 @@ describe("cluster:connect", () => { describe("multiple reconnect", () => { it("should reconnect after multiple consecutive disconnect(true) are called", (done) => { - new MockServer(30001); + const slotTable = [[0, 16383, ["127.0.0.1", 30001]]]; + new MockServer(30001, (argv) => { + if (argv[0] === "cluster" && argv[1] === "SLOTS") { + return slotTable; + } + }); const cluster = new Cluster([{ host: "127.0.0.1", port: "30001" }], { enableReadyCheck: false, }); diff --git a/test/functional/cluster/disconnection.ts b/test/functional/cluster/disconnection.ts index 40e33e01..1adafcb3 100644 --- a/test/functional/cluster/disconnection.ts +++ b/test/functional/cluster/disconnection.ts @@ -9,7 +9,12 @@ describe("disconnection", () => { }); it("should clear all timers on disconnect", (done) => { - const server = new MockServer(30000); + const slotTable = [[0, 16383, ["127.0.0.1", 30000]]]; + const server = new MockServer(30000, (argv) => { + if (argv[0] === "cluster" && argv[1] === "SLOTS") { + return slotTable; + } + }); const setIntervalCalls = sinon.spy(global, "setInterval"); const clearIntervalCalls = sinon.spy(global, "clearInterval"); From 3165ff02008860ec6b1811ce1542d8272ff8584a Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Thu, 15 Aug 2024 23:07:42 +0200 Subject: [PATCH 20/20] Do not assume that node removal will occur *after* refreshSlotsCache() is finished --- test/functional/cluster/index.ts | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/test/functional/cluster/index.ts b/test/functional/cluster/index.ts index e31bdd91..c4b09a7d 100644 --- a/test/functional/cluster/index.ts +++ b/test/functional/cluster/index.ts @@ -370,27 +370,28 @@ describe("cluster", () => { }); cluster.on("ready", () => { expect(cluster.nodes("master")).to.have.lengthOf(2); + expect(cluster.nodes("all")).to.have.lengthOf(3); slotTable = [ [0, 5460, ["127.0.0.1", 30003]], [5461, 10922, ["127.0.0.1", 30002]], ]; - cluster.refreshSlotsCache(() => { - cluster.once("-node", function (removed) { - expect(removed.options.port).to.eql(30001); - expect(cluster.nodes("master")).to.have.lengthOf(2); - expect( - [ - cluster.nodes("master")[0].options.port, - cluster.nodes("master")[1].options.port, - ].sort() - ).to.eql([30002, 30003]); - cluster.nodes("master").forEach(function (node) { - expect(node.options).to.have.property("readOnly", false); - }); - cluster.disconnect(); - done(); + cluster.once("-node", function (removed) { + expect(removed.options.port).to.eql(30001); + expect(cluster.nodes("master")).to.have.lengthOf(2); + expect(cluster.nodes("all")).to.have.lengthOf(2); + expect( + [ + cluster.nodes("master")[0].options.port, + cluster.nodes("master")[1].options.port, + ].sort() + ).to.eql([30002, 30003]); + cluster.nodes("master").forEach(function (node) { + expect(node.options).to.have.property("readOnly", false); }); + cluster.disconnect(); + done(); }); + cluster.refreshSlotsCache(); }); }); });