From 90039df7082bf33071d70a48d3ae6b7a62ea38c6 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:50:21 +0200 Subject: [PATCH 01/20] Tell Redis cluster to disable protected mode before running tests Signed-off-by: Martin Slota --- 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 8b5478509cb4bb9d4da0978f36bf5c926ba71c01 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:51:04 +0200 Subject: [PATCH 02/20] Try to enable Redis cluster tests on CI Signed-off-by: Martin Slota --- .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 ba84c833..2f760210 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 da9b9f74ea997b490d89b6abbc7982b3d560d03f Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:51:40 +0200 Subject: [PATCH 03/20] Add a failing test around Redis cluster disconnection logic Signed-off-by: Martin Slota --- 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 f0e5f666438bb39fefde5f96013ec4bf58ab3927 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:52:13 +0200 Subject: [PATCH 04/20] Rename function parameter Signed-off-by: Martin Slota --- 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 78e22399183a53576bd01430e869674965364952 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:52:50 +0200 Subject: [PATCH 05/20] Turn node error listener into an arrow function so that points to the connection pool instance Signed-off-by: Martin Slota --- 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 fdb3db80ffc92b7f67a670ab0db0260c586cbe5a Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:53:25 +0200 Subject: [PATCH 06/20] Extract node listeners into separate constants Signed-off-by: Martin Slota --- 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 d4cec9c242a043b15f73572f3ef2bf9395d9ac43 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:53:59 +0200 Subject: [PATCH 07/20] Keep track of listeners along with each Redis client Signed-off-by: Martin Slota --- 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 c767ee04d2a287e2a0cf4e83e923ac113b33c3eb Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:54:40 +0200 Subject: [PATCH 08/20] Remove node listeners when the node is being removed Signed-off-by: Martin Slota --- 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 43b8522b2db6c25cb482e2e07f3eec99ec77d7c6 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:55:16 +0200 Subject: [PATCH 09/20] Emit node removal events whenever a node is removed Signed-off-by: Martin Slota --- 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 09d336d8476f0913427fe4174a07d3e50a600aae Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:55:54 +0200 Subject: [PATCH 10/20] When resetting, add nodes before removing old ones Signed-off-by: Martin Slota --- 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 f002230d583a883f62c8628e54e7eab77e5a13e8 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:56:24 +0200 Subject: [PATCH 11/20] Rename Node type to NodeRecord for clarity Signed-off-by: Martin Slota --- 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 41751c0277b0656058d8eb8b79f118c5189f317e Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:57:34 +0200 Subject: [PATCH 12/20] Also rename the field holding node records Signed-off-by: Martin Slota --- 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 c8fe73ae2842fed1b492fd4c160dc2f02465be65 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:58:06 +0200 Subject: [PATCH 13/20] Rename variable to nodeRecord Signed-off-by: Martin Slota --- 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 21b947d8db49cf7959f00f62a324b4c784053055 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:58:42 +0200 Subject: [PATCH 14/20] Rename another variable to nodeRecord Signed-off-by: Martin Slota --- 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 6536e2290eaee5cd08fde452e9d8dc6566a1395b Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:59:17 +0200 Subject: [PATCH 15/20] Fix a reference to connection pool nodes Signed-off-by: Martin Slota --- 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 158c64cfe5765d3b4b61bde2de716431d83a75cd Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 13:59:56 +0200 Subject: [PATCH 16/20] Do not fail when retrieving a node by non-existing key Signed-off-by: Martin Slota --- 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 788361c043695aac31d5d4bd0e34ae9d6a57593e Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 18:48:41 +0200 Subject: [PATCH 17/20] Revert "Fix a reference to connection pool nodes" This reverts commit 6536e2290eaee5cd08fde452e9d8dc6566a1395b. Signed-off-by: Martin Slota --- 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 0deaebaecdb6daa65295674ea19d14b4ad9fdbd9 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 18:50:19 +0200 Subject: [PATCH 18/20] Fix a reference to connection pool nodes, this time a bit more correctly Signed-off-by: Martin Slota --- 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 d5d85e95b9d97f915778ce0bd0d533f41a5bc6aa Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 18:53:30 +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 Signed-off-by: Martin Slota --- 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 d3f83c52cf9ef88d3ca8271356fd7f574777dad6 Mon Sep 17 00:00:00 2001 From: Martin Slota Date: Mon, 10 Jun 2024 18:55:55 +0200 Subject: [PATCH 20/20] Do not assume that node removal will occur *after* refreshSlotsCache() is finished Signed-off-by: Martin Slota --- 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(); }); }); });