mirror of
https://github.com/oven-sh/bun
synced 2026-02-14 12:51:54 +00:00
### What does this PR do? This PR fixes https://github.com/oven-sh/bun/issues/19131. I am not 100% certain that this fix is correct as I am still nebulous regarding some decisions I've made in this PR. I'll try to provide my reasoning and would love to be proven wrong: #### Re-authentication - The `is_authenticated` flag needs to be reset to false. When the lifecycle reaches a point of attempting to connect, it sends out a `HELLO 3`, and receives a response. `handleResponse()` is fired and does not correctly handle it because there is a guard at the top of the function: ```zig if (!this.flags.is_authenticated) { this.handleHelloResponse(value); // We've handled the HELLO response without consuming anything from the command queue return; } ``` Rather, it treats this packet as a regular data packet and complains that it doesn't have a promise to associate it to. By resetting the `is_authenticated` flag to false, we guarantee that we handle the `HELLO 3` packet as an authentication packet. It also seems to make semantic sense since dropping a connection implies you dropped authentication. #### Retry Attempts I've deleted the `retry_attempts = 0` in `reconnect()` because I noticed that we would never actually re-attempt to reconnect after the first attempt. Specifically, I was expecting `valkey.zig:459` to potentially fire multiple times, but it only ever fired once. Removing this reset to zero caused successful reattempts (in my case 3 of them). ```zig debug("reconnect in {d}ms (attempt {d}/{d})", .{ delay_ms, this.retry_attempts, this.max_retries }); ``` I'm still iffy on whether this is necessary, but I think it makes sense. ```zig this.client.retry_attempts = 0 ``` ### How did you verify your code works? I have added a small unit test. I have compared mainline `bun`, which fails that test, to this fix, which passes the test. --------- Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
200 lines
7.1 KiB
TypeScript
200 lines
7.1 KiB
TypeScript
import { randomUUIDv7, RedisClient } from "bun";
|
|
import { beforeEach, describe, expect, test } from "bun:test";
|
|
import { ConnectionType, createClient, ctx, DEFAULT_REDIS_URL, expectType, isEnabled } from "./test-utils";
|
|
|
|
describe.skipIf(!isEnabled)("Valkey Redis Client", () => {
|
|
beforeEach(() => {
|
|
if (ctx.redis?.connected) {
|
|
ctx.redis.close?.();
|
|
}
|
|
ctx.redis = createClient(ConnectionType.TCP);
|
|
});
|
|
|
|
describe("Basic Operations", () => {
|
|
test("should set and get strings", async () => {
|
|
const redis = ctx.redis;
|
|
const testKey = "greeting";
|
|
const testValue = "Hello from Bun Redis!";
|
|
|
|
// Using direct set and get methods
|
|
const setResult = await redis.set(testKey, testValue);
|
|
expect(setResult).toMatchInlineSnapshot(`"OK"`);
|
|
|
|
const setResult2 = await redis.set(testKey, testValue, "GET");
|
|
expect(setResult2).toMatchInlineSnapshot(`"Hello from Bun Redis!"`);
|
|
|
|
// GET should return the value we set
|
|
const getValue = await redis.get(testKey);
|
|
expect(getValue).toMatchInlineSnapshot(`"Hello from Bun Redis!"`);
|
|
});
|
|
|
|
test("should test key existence", async () => {
|
|
const redis = ctx.redis;
|
|
// Let's set a key first
|
|
await redis.set("greeting", "test existence");
|
|
|
|
// EXISTS in Redis normally returns integer 1 if key exists, 0 if not
|
|
// The current implementation doesn't transform exists correctly yet
|
|
const exists = await redis.exists("greeting");
|
|
expect(exists).toBeDefined();
|
|
// Should be true for existing keys (fixed in special handling for EXISTS)
|
|
expect(exists).toBe(true);
|
|
|
|
// For non-existent keys
|
|
const randomKey = "nonexistent-key-" + randomUUIDv7();
|
|
const notExists = await redis.exists(randomKey);
|
|
expect(notExists).toBeDefined();
|
|
// Should be false for non-existing keys
|
|
expect(notExists).toBe(false);
|
|
});
|
|
|
|
test("should increment and decrement counters", async () => {
|
|
const redis = ctx.redis;
|
|
const counterKey = "counter";
|
|
// First set a counter value
|
|
await redis.set(counterKey, "10");
|
|
|
|
// INCR should increment and return the new value
|
|
const incrementedValue = await redis.incr(counterKey);
|
|
expect(incrementedValue).toBeDefined();
|
|
expect(typeof incrementedValue).toBe("number");
|
|
expect(incrementedValue).toBe(11);
|
|
|
|
// DECR should decrement and return the new value
|
|
const decrementedValue = await redis.decr(counterKey);
|
|
expect(decrementedValue).toBeDefined();
|
|
expect(typeof decrementedValue).toBe("number");
|
|
expect(decrementedValue).toBe(10);
|
|
});
|
|
|
|
test("should manage key expiration", async () => {
|
|
const redis = ctx.redis;
|
|
// Set a key first
|
|
const tempKey = "temporary";
|
|
await redis.set(tempKey, "will expire");
|
|
|
|
// EXPIRE should return 1 if the timeout was set, 0 otherwise
|
|
const result = await redis.expire(tempKey, 60);
|
|
// Using native expire command instead of send()
|
|
expect(result).toMatchInlineSnapshot(`1`);
|
|
|
|
// Use the TTL command directly
|
|
const ttl = await redis.ttl(tempKey);
|
|
expectType<number>(ttl, "number");
|
|
expect(ttl).toBeGreaterThan(0);
|
|
expect(ttl).toBeLessThanOrEqual(60); // Should be positive and not exceed our set time
|
|
});
|
|
|
|
test("should implement TTL command correctly for different cases", async () => {
|
|
const redis = ctx.redis;
|
|
// 1. Key with expiration
|
|
const tempKey = "ttl-test-key";
|
|
await redis.set(tempKey, "ttl test value");
|
|
await redis.expire(tempKey, 60);
|
|
|
|
// Use native ttl command
|
|
const ttl = await redis.ttl(tempKey);
|
|
expectType<number>(ttl, "number");
|
|
expect(ttl).toBeGreaterThan(0);
|
|
expect(ttl).toBeLessThanOrEqual(60);
|
|
|
|
// 2. Key with no expiration
|
|
const permanentKey = "permanent-key";
|
|
await redis.set(permanentKey, "no expiry");
|
|
const noExpiry = await redis.ttl(permanentKey);
|
|
expect(noExpiry).toMatchInlineSnapshot(`-1`); // -1 indicates no expiration
|
|
|
|
// 3. Non-existent key
|
|
const nonExistentKey = "non-existent-" + randomUUIDv7();
|
|
const noKey = await redis.ttl(nonExistentKey);
|
|
expect(noKey).toMatchInlineSnapshot(`-2`); // -2 indicates key doesn't exist
|
|
});
|
|
});
|
|
|
|
describe("Connection State", () => {
|
|
test("should have a connected property", () => {
|
|
const redis = ctx.redis;
|
|
// The client should expose a connected property
|
|
expect(typeof redis.connected).toBe("boolean");
|
|
});
|
|
});
|
|
|
|
describe("RESP3 Data Types", () => {
|
|
test("should handle hash maps (dictionaries) as command responses", async () => {
|
|
const redis = ctx.redis;
|
|
// HSET multiple fields
|
|
const userId = "user:" + randomUUIDv7().substring(0, 8);
|
|
const setResult = await redis.send("HSET", [userId, "name", "John", "age", "30", "active", "true"]);
|
|
expect(setResult).toBeDefined();
|
|
|
|
// HGETALL returns object with key-value pairs
|
|
const hash = await redis.send("HGETALL", [userId]);
|
|
expect(hash).toBeDefined();
|
|
|
|
// Proper structure checking when RESP3 maps are fixed
|
|
if (typeof hash === "object" && hash !== null) {
|
|
expect(hash).toHaveProperty("name");
|
|
expect(hash).toHaveProperty("age");
|
|
expect(hash).toHaveProperty("active");
|
|
|
|
expect(hash.name).toBe("John");
|
|
expect(hash.age).toBe("30");
|
|
expect(hash.active).toBe("true");
|
|
}
|
|
});
|
|
|
|
test("should handle sets as command responses", async () => {
|
|
const redis = ctx.redis;
|
|
// Add items to a set
|
|
const setKey = "colors:" + randomUUIDv7().substring(0, 8);
|
|
const addResult = await redis.send("SADD", [setKey, "red", "blue", "green"]);
|
|
expect(addResult).toBeDefined();
|
|
|
|
// Get set members
|
|
const setMembers = await redis.send("SMEMBERS", [setKey]);
|
|
expect(setMembers).toBeDefined();
|
|
|
|
// Check if the response is an array
|
|
expect(Array.isArray(setMembers)).toBe(true);
|
|
|
|
// Should contain our colors
|
|
expect(setMembers).toContain("red");
|
|
expect(setMembers).toContain("blue");
|
|
expect(setMembers).toContain("green");
|
|
});
|
|
});
|
|
|
|
describe("Connection Options", () => {
|
|
test("connection errors", async () => {
|
|
const url = new URL(DEFAULT_REDIS_URL);
|
|
url.username = "badusername";
|
|
url.password = "secretpassword";
|
|
const customRedis = new RedisClient(url.toString());
|
|
|
|
expect(async () => {
|
|
await customRedis.get("test");
|
|
}).toThrowErrorMatchingInlineSnapshot(`"WRONGPASS invalid username-password pair or user is disabled."`);
|
|
});
|
|
});
|
|
|
|
describe("Reconnections", () => {
|
|
test("should automatically reconnect after connection drop", async () => {
|
|
const TEST_KEY = "test-key";
|
|
const TEST_VALUE = "test-value";
|
|
|
|
const valueBeforeStart = await ctx.redis.get(TEST_KEY);
|
|
expect(valueBeforeStart).toBeNull();
|
|
|
|
// Set some value
|
|
await ctx.redis.set(TEST_KEY, TEST_VALUE);
|
|
const valueAfterSet = await ctx.redis.get(TEST_KEY);
|
|
expect(valueAfterSet).toBe(TEST_VALUE);
|
|
|
|
await ctx.restartServer();
|
|
|
|
const valueAfterStop = await ctx.redis.get(TEST_KEY);
|
|
expect(valueAfterStop).toBe(TEST_VALUE);
|
|
});
|
|
});
|
|
});
|