Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/runtime/valkey_jsc/js_valkey.rs
Original file line number Diff line number Diff line change
Expand Up @@ -866,9 +866,6 @@ impl JSValkeyClient {
tls,
database: client.database,
flags: valkey::ConnectionFlags {
// If the user manually closed the connection, then duplicating a closed client
// means the new client remains finalized.
is_manually_closed: client.flags.is_manually_closed,
enable_offline_queue: if sub_ctx.is_subscriber {
sub_ctx.original_enable_offline_queue
} else {
Expand All @@ -881,8 +878,6 @@ impl JSValkeyClient {
} else {
client.flags.enable_auto_pipelining
},
// Duplicating a finalized client means it stays finalized.
finalized: client.flags.finalized,
..Default::default()
},
max_retries: client.max_retries,
Expand Down
4 changes: 1 addition & 3 deletions src/runtime/valkey_jsc/valkey.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,9 +595,7 @@ impl ValkeyClient {

// A failure the client detected itself (idle timeout, protocol or
// handshake error) has always been a deliberate close; `on_close` reads
// `failed` and skips the retry policy. It is not `is_manually_closed`:
// that flag is copied into `duplicate()`, and a duplicate of a failed
// client should still reconnect.
// `failed` and skips the retry policy.
let closed = self.close(uws::CloseCode::Failure); // unconditionally, whatever `val` is
val.and(closed)
}
Expand Down
45 changes: 42 additions & 3 deletions test/js/valkey/reliability/connection-failures.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -896,9 +896,48 @@ describe("Valkey: Recovering After fail()", () => {
await expect(client.ping()).rejects.toMatchObject({ code: "ERR_REDIS_INVALID_RESPONSE_TYPE" });
expect(client.connected).toBe(false);
duplicate = await client.duplicate();
// A duplicate copies the original's manual-close state; a failure is not
// one, so the drop of connection 2 goes through the retry policy: no
// onclose, a second onconnect, and the queued PING answered by connection 3.
// A duplicate carries no close history from its source, so the drop of
// connection 2 goes through the retry policy: no onclose, a second
// onconnect, and the next PING answered by connection 3.
const reconnected = Promise.withResolvers<void>();
let connects = 0;
duplicate.onconnect = () => {
if (++connects === 2) reconnected.resolve();
};
duplicate.onclose = err => reconnected.reject(err);
expect(await duplicate.ping()).toBe("PONG");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
await reconnected.promise;
expect(await duplicate.ping()).toBe("PONG");
expect(fake.connections).toBe(3);
} finally {
duplicate?.close();
client.close();
fake.server.close();
}
});

test("a duplicate of a closed client auto-reconnects", async () => {
// Connection 2 (the duplicate's first) is dropped by the server right
// after it answers PING.
const fake = helloServer({
PING: (connection, socket) => {
if (connection === 2) {
socket.end("+PONG\r\n");
return null;
}
return "+PONG\r\n";
},
});
const port = await fake.listen();
const client = new RedisClient(`redis://127.0.0.1:${port}`, { autoReconnect: true });
let duplicate: RedisClient | undefined;
try {
await client.connect();
client.close();
duplicate = await client.duplicate();
// The duplicate has no close history of its own, so the drop of
// connection 2 goes through the retry policy: no onclose, a second
// onconnect, and the next PING answered by connection 3.
const reconnected = Promise.withResolvers<void>();
let connects = 0;
duplicate.onconnect = () => {
Expand Down
Loading