Skip to content

Conversation

Jarred-Sumner
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner commented Apr 6, 2025

What does this PR do?

import { redis } from "bun";

// Set a key
await redis.set("greeting", "Hello from Bun!");

// Get a key
const greeting = await redis.get("greeting");
console.log(greeting); // "Hello from Bun!"

// Increment a counter
await redis.set("counter", "0");
await redis.incr("counter");

// Check if a key exists
const exists = await redis.exists("greeting");

// Delete a key
await redis.del("greeting");

fixes #18112

How did you verify your code works?

There are tests. Need to setup a managed redis before the CI will pass and work on the tests some more.

@robobun

This comment was marked as outdated.

this.reconnect_timer.state = .FIRED;

// Increment ref to ensure 'this' stays alive throughout the function
this.ref();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary since reconnect already do ref/unref

const handshake_success = if (success == 1) true else false;
this.ref();
defer this.deref();
if (handshake_success) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If handshake_success is false we need to fail this.client.failWithJSValue(this.globalObject, ssl_error.toJS(this.globalObject)); because it means the server rejected the connection

Comment on lines +1243 to +1249
const valkey = JSC.API.Valkey.create(globalThis, &[_]JSValue{.undefined}) catch |err| {
if (err != error.JSError) {
_ = globalThis.throwError(err, "Failed to create Redis client") catch {};
return .zero;
}
return .zero;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const valkey = JSC.API.Valkey.create(globalThis, &[_]JSValue{.undefined}) catch |err| {
if (err != error.JSError) {
_ = globalThis.throwError(err, "Failed to create Redis client") catch {};
return .zero;
}
return .zero;
};
const valkey = JSC.API.Valkey.create(globalThis, &[_]JSValue{.undefined}) catch |err| switch (err) {
error.JSError => return .zero,
error.OutOfMemory => return global.throwOutOfMemoryValue(),
};

Copy link
Member

@dylan-conway dylan-conway Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice if these getters had JSError!JSValue return types

this.client.socket = _socket(socket);

// Do not allow half-open connections
socket.close(.normal);
Copy link
Member

@cirospaciari cirospaciari Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont need to call close here unless we pass LIBUS_SOCKET_ALLOW_HALF_OPEN in options when connecting because we will call the on_end and close in the sequence, but should be a no-op

@Jarred-Sumner Jarred-Sumner marked this pull request as ready for review April 8, 2025 10:33
@Jarred-Sumner Jarred-Sumner merged commit ec87a27 into main Apr 8, 2025
69 of 71 checks passed
@Jarred-Sumner Jarred-Sumner deleted the jarred/redis2 branch April 8, 2025 10:34
@mckoda09
Copy link

mckoda09 commented Apr 8, 2025

thanks bro

@Electroid Electroid mentioned this pull request May 16, 2025
67 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun KV

5 participants