Skip to content

Commit fcbbbe8

Browse files
PavelPashovPavelPashov
authored
fix: move CLIENT SETINFO commands to connection handshake (#2033)
* refactor: move CLIENT SETINFO commands to connection handshake * test: add client setinfo autopipelining and reconnectOnError tests * refactor: remove setInfo from Condition
1 parent b179039 commit fcbbbe8

File tree

9 files changed

+110
-63
lines changed

9 files changed

+110
-63
lines changed

‎lib/Command.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,10 @@ export interface CommandNameFlags {
5555
EXIT_SUBSCRIBER_MODE: ["unsubscribe", "punsubscribe", "sunsubscribe"];
5656
// Commands that will make client disconnect from server TODO shutdown?
5757
WILL_DISCONNECT: ["quit"];
58+
// Commands that are sent when the client is connecting
59+
HANDSHAKE_COMMANDS: ["auth", "select", "client", "readonly", "info"];
60+
// Commands that should not trigger a reconnection when errors occur
61+
IGNORE_RECONNECT_ON_ERROR: ["client"];
5862
}
5963

6064
/**
@@ -95,6 +99,8 @@ export default class Command implements Respondable {
9599
ENTER_SUBSCRIBER_MODE: ["subscribe", "psubscribe", "ssubscribe"],
96100
EXIT_SUBSCRIBER_MODE: ["unsubscribe", "punsubscribe", "sunsubscribe"],
97101
WILL_DISCONNECT: ["quit"],
102+
HANDSHAKE_COMMANDS: ["auth", "select", "client", "readonly", "info"],
103+
IGNORE_RECONNECT_ON_ERROR: ["client"],
98104
};
99105

100106
private static flagMap?: FlagMap;

‎lib/Redis.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,8 @@ class Redis extends Commander implements DataHandledable {
450450
(!stream &&
451451
this.status === "connect" &&
452452
exists(command.name) &&
453-
hasFlag(command.name, "loading"));
453+
(hasFlag(command.name, "loading") ||
454+
Command.checkFlag("HANDSHAKE_COMMANDS", command.name)));
454455
if (!this.stream) {
455456
writable = false;
456457
} else if (!this.stream.writable) {
@@ -646,7 +647,10 @@ class Redis extends Commander implements DataHandledable {
646647
*/
647648
handleReconnection(err: Error, item: CommandItem) {
648649
let needReconnect: ReturnType<ReconnectOnError> = false;
649-
if (this.options.reconnectOnError) {
650+
if (
651+
this.options.reconnectOnError &&
652+
!Command.checkFlag("IGNORE_RECONNECT_ON_ERROR", item.command.name)
653+
) {
650654
needReconnect = this.options.reconnectOnError(err);
651655
}
652656

‎lib/autoPipelining.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export const notAllowedAutoPipelineCommands = [
1818
"unsubscribe",
1919
"unpsubscribe",
2020
"select",
21+
"client",
2122
];
2223

2324
function executeAutoPipeline(client, slotKey: string) {

‎lib/redis/event_handler.ts‎

Lines changed: 60 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,6 @@ export function connectHandler(self) {
6666
});
6767
}
6868

69-
if (!self.options.enableReadyCheck) {
70-
exports.readyHandler(self)();
71-
}
72-
7369
/*
7470
No need to keep the reference of DataHandler here
7571
because we don't need to do the cleanup.
@@ -79,27 +75,69 @@ export function connectHandler(self) {
7975
stringNumbers: self.options.stringNumbers,
8076
});
8177

82-
if (self.options.enableReadyCheck) {
83-
self._readyCheck(function (err, info) {
84-
if (connectionEpoch !== self.connectionEpoch) {
85-
return;
78+
const clientCommandPromises = [];
79+
80+
if (self.options.connectionName) {
81+
debug("set the connection name [%s]", self.options.connectionName);
82+
clientCommandPromises.push(
83+
self.client("setname", self.options.connectionName).catch(noop)
84+
);
85+
}
86+
87+
if (!self.options.disableClientInfo) {
88+
debug("set the client info");
89+
clientCommandPromises.push(
90+
getPackageMeta()
91+
.then((packageMeta) => {
92+
return self
93+
.client("SETINFO", "LIB-VER", packageMeta.version)
94+
.catch(noop);
95+
})
96+
.catch(noop)
97+
);
98+
99+
clientCommandPromises.push(
100+
self
101+
.client(
102+
"SETINFO",
103+
"LIB-NAME",
104+
self.options?.clientInfoTag
105+
? `ioredis(${self.options.clientInfoTag})`
106+
: "ioredis"
107+
)
108+
.catch(noop)
109+
);
110+
}
111+
112+
Promise.all(clientCommandPromises)
113+
.catch(noop)
114+
.finally(() => {
115+
if (!self.options.enableReadyCheck) {
116+
exports.readyHandler(self)();
86117
}
87-
if (err) {
88-
if (!flushed) {
89-
self.recoverFromFatalError(
90-
new Error("Ready check failed: " + err.message),
91-
err
92-
);
93-
}
94-
} else {
95-
if (self.connector.check(info)) {
96-
exports.readyHandler(self)();
97-
} else {
98-
self.disconnect(true);
99-
}
118+
119+
if (self.options.enableReadyCheck) {
120+
self._readyCheck(function (err, info) {
121+
if (connectionEpoch !== self.connectionEpoch) {
122+
return;
123+
}
124+
if (err) {
125+
if (!flushed) {
126+
self.recoverFromFatalError(
127+
new Error("Ready check failed: " + err.message),
128+
err
129+
);
130+
}
131+
} else {
132+
if (self.connector.check(info)) {
133+
exports.readyHandler(self)();
134+
} else {
135+
self.disconnect(true);
136+
}
137+
}
138+
});
100139
}
101140
});
102-
}
103141
};
104142
}
105143

@@ -264,38 +302,6 @@ export function readyHandler(self) {
264302
? self.prevCondition.select
265303
: self.condition.select;
266304

267-
if (self.options.connectionName) {
268-
debug("set the connection name [%s]", self.options.connectionName);
269-
self.client("setname", self.options.connectionName).catch(noop);
270-
}
271-
272-
if (!self.options?.disableClientInfo) {
273-
debug("set the client info");
274-
275-
let version = null;
276-
277-
getPackageMeta()
278-
.then((packageMeta) => {
279-
version = packageMeta?.version;
280-
})
281-
.catch(noop)
282-
.finally(() => {
283-
self
284-
.client("SETINFO", "LIB-VER", version)
285-
.catch(noop);
286-
});
287-
288-
self
289-
.client(
290-
"SETINFO",
291-
"LIB-NAME",
292-
self.options?.clientInfoTag
293-
? `ioredis(${self.options.clientInfoTag})`
294-
: "ioredis"
295-
)
296-
.catch(noop);
297-
}
298-
299305
if (self.options.readOnly) {
300306
debug("set the connection to readonly mode");
301307
self.readonly().catch(noop);

‎test/cluster/basic.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,17 @@ describe("cluster", () => {
128128
cluster.set("foo", "auto pipelining");
129129
expect(await cluster.get("foo")).to.eql("auto pipelining");
130130
});
131+
132+
it("client setinfo works with auto pipelining", async () => {
133+
const cluster = new Cluster([{ host: "127.0.0.1", port: masters[0] }], {
134+
enableAutoPipelining: true,
135+
});
136+
137+
for (let i = 0; i < 100; i++) {
138+
await cluster.set(`foo${i}`, "bar");
139+
expect(await cluster.get(`foo${i}`)).to.eql("bar");
140+
}
141+
});
131142
});
132143

133144
describe("key prefixing", () => {

‎test/functional/client_info.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ describe("clientInfo", function () {
3434
it("should send client info by default", async () => {
3535
redis = new Redis({ port: 30001 });
3636

37-
// Wait for the client info to be sent, as it happens after the ready event
37+
// Wait for the client info to be sent, to make sure the connection is established
3838
await redis.ping();
3939

4040
expect(clientInfoCommands).to.have.length(2);
@@ -56,7 +56,7 @@ describe("clientInfo", function () {
5656
it("should not send client info when disableClientInfo is true", async () => {
5757
redis = new Redis({ port: 30001, disableClientInfo: true });
5858

59-
// Wait for the client info to be sent, as it happens after the ready event
59+
// Wait for the client info to be sent, to make sure the commands are sent
6060
await redis.ping();
6161

6262
expect(clientInfoCommands).to.have.length(0);
@@ -65,7 +65,7 @@ describe("clientInfo", function () {
6565
it("should append tag to library name when clientInfoTag is set", async () => {
6666
redis = new Redis({ port: 30001, clientInfoTag: "tag-test" });
6767

68-
// Wait for the client info to be sent, as it happens after the ready event
68+
// Wait for the client info to be sent, to make sure the commands are sent
6969
await redis.ping();
7070

7171
expect(clientInfoCommands).to.have.length(2);
@@ -80,7 +80,7 @@ describe("clientInfo", function () {
8080
it("should send client info after reconnection", async () => {
8181
redis = new Redis({ port: 30001 });
8282

83-
// Wait for the client info to be sent, as it happens after the ready event
83+
// Wait for the client info to be sent, to make sure the commands are sent
8484
await redis.ping();
8585
redis.disconnect();
8686

@@ -171,7 +171,7 @@ describe("clientInfo", function () {
171171
it("should send client info by default", async () => {
172172
cluster = new Redis.Cluster([{ host: "127.0.0.1", port: 30001 }]);
173173

174-
// Wait for cluster to be ready and send a command to ensure connection
174+
// Wait for the client info to be sent, to make sure the commands are sent
175175
await cluster.ping();
176176

177177
// Should have sent 2 SETINFO commands (LIB-VER and LIB-NAME)

‎test/functional/cluster/autopipelining.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ describe("autoPipelining for cluster", () => {
123123

124124
promises.push(cluster.subscribe("subscribe").catch(() => {}));
125125
promises.push(cluster.unsubscribe("subscribe").catch(() => {}));
126+
promises.push(cluster.client("help").catch(() => {}));
126127

127128
expect(cluster.autoPipelineQueueSize).to.eql(0);
128129
await promises;

‎test/functional/connection.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,11 @@ describe("connection", function () {
3131
times += 1;
3232
if (times === 1) {
3333
expect(command.name).to.eql("auth");
34-
} else if (times === 2) {
34+
} else if (times === 2 || times === 3) {
35+
expect(command.name).to.eql("client");
36+
} else if (times === 4) {
3537
expect(command.name).to.eql("info");
36-
} else if (times === 3) {
38+
} else if (times === 5) {
3739
redis.disconnect();
3840
setImmediate(() => done());
3941
}

‎test/functional/reconnect_on_error.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,4 +147,20 @@ describe("reconnectOnError", () => {
147147
done();
148148
});
149149
});
150+
151+
it("should not reconnect if reconnectOnError if ignored command fails", (done) => {
152+
const redis = new Redis({
153+
reconnectOnError: function (err) {
154+
return true;
155+
},
156+
});
157+
158+
redis.disconnect = () => {
159+
throw new Error("should not disconnect");
160+
};
161+
162+
redis.client("some-wrong-arg", function (err) {
163+
done();
164+
});
165+
});
150166
});

0 commit comments

Comments
 (0)