Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 queued 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