Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go: Implement connection logic in lib.rs #119

Closed
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
c0cb6fb
Add Rust connection logic and example
jonathanl-bq Feb 27, 2024
22bbf74
Add error handling logic for connections
jonathanl-bq Feb 27, 2024
79343ff
Add license headers
jonathanl-bq Feb 27, 2024
2a92415
Refactor cArgs
jonathanl-bq Feb 27, 2024
3278942
Update API for client creation
jonathanl-bq Feb 29, 2024
2b258e5
Remove non-connection logic
jonathanl-bq Mar 4, 2024
6ccc6c1
Update lib.h
jonathanl-bq Mar 4, 2024
bdf9c19
Add additional documentation to lib.rs functions
jonathanl-bq Mar 5, 2024
4bd25cc
Properly free connection response and error
jonathanl-bq Mar 6, 2024
dc4df76
Run go fmt
jonathanl-bq Mar 6, 2024
cd5c2da
Add error for when user closes client before creating it
jonathanl-bq Mar 6, 2024
055e74b
Add doc comments about avoiding memory leaks
jonathanl-bq Mar 6, 2024
f23b01a
Allow unused Client fields for now
jonathanl-bq Mar 6, 2024
aa1fb08
Add formatting directive to fmt.Errorf
jonathanl-bq Mar 6, 2024
2fe5bdd
Replace callback implementations with TODO implementations
jonathanl-bq Mar 6, 2024
6289bfb
Resolve merge conflicts
jonathanl-bq Mar 6, 2024
53c8d7d
Remove RedisErrorFFI struct and use glide-core RequetErrorType instead
jonathanl-bq Mar 7, 2024
926e050
Update go/glide/lib.h
jonathanl-bq Mar 7, 2024
b9e61c8
Update go/glide/lib.h
jonathanl-bq Mar 7, 2024
70fb74b
Update go/src/lib.rs
jonathanl-bq Mar 7, 2024
db8959b
Update go/src/lib.rs
jonathanl-bq Mar 7, 2024
b32408c
Update documentation for create_client
jonathanl-bq Mar 7, 2024
478acd6
Adjust documentation regarding memory leaks
jonathanl-bq Mar 7, 2024
0e52d91
Make free_connection_response also free its error
jonathanl-bq Mar 7, 2024
a02912f
Update doc comment for free_connection_response
jonathanl-bq Mar 8, 2024
7ea7bdd
Re-expose free_error for the error callback to use and update docs
jonathanl-bq Mar 8, 2024
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
3 changes: 3 additions & 0 deletions go/.gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
# Go compilation files
*.pb.go

# Example binary
examples/examples
jonathanl-bq marked this conversation as resolved.
Show resolved Hide resolved

reports
2 changes: 2 additions & 0 deletions go/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ redis = { path = "../submodules/redis-rs/redis", features = ["aio", "tokio-comp"
glide-core = { path = "../glide-core" }
tokio = { version = "^1", features = ["rt", "macros", "rt-multi-thread", "time"] }
protobuf = { version = "3.3.0", features = [] }
logger_core = {path = "../logger_core"}
tracing-subscriber = "0.3.16"

[profile.release]
lto = true
Expand Down
38 changes: 38 additions & 0 deletions go/examples/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/**
* Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0
*/
package main

import (
"fmt"
"github.com/aws/glide-for-redis/go/glide/glide"
"github.com/aws/glide-for-redis/go/glide/protobuf"
"log"
)

func main() {
fmt.Println("Starting go-glide client...")
client := glide.GlideRedisClient{}
acarbonetto marked this conversation as resolved.
Show resolved Hide resolved

// TODO: Update when configuration is implemented
request := &protobuf.ConnectionRequest{
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
TlsMode: protobuf.TlsMode_NoTls,
ClusterModeEnabled: false,
ReadFrom: protobuf.ReadFrom_Primary,
}
request.Addresses = append(
request.Addresses,
&protobuf.NodeAddress{Host: "localhost", Port: uint32(6379)},
)
connectionErr := client.ConnectToRedis(request)
if connectionErr != nil {
log.Fatal(connectionErr)
}

closeClientErr := client.CloseClient()
if closeClientErr != nil {
log.Fatal(closeClientErr)
}

fmt.Println("Disconnected from Redis")
}
81 changes: 81 additions & 0 deletions go/glide/glide.go
Original file line number Diff line number Diff line change
@@ -1 +1,82 @@
/**
* Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0
*/
package glide

/*
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
#cgo LDFLAGS: -L../target/release -lglide_rs
#include "lib.h"

void successCallback(uintptr_t channelPtr, char *message);
void failureCallback(uintptr_t channelPtr, struct RedisErrorFFI *errMessage);
*/
import "C"

import (
"fmt"
"github.com/aws/glide-for-redis/go/glide/protobuf"
"github.com/golang/protobuf/proto"
"unsafe"
)

type GlideRedisClient struct {
coreClient unsafe.Pointer
}

type payload struct {
value string
errMessage error
}

type RequestType uint32

type ErrorType uint32

const (
ClosingError = iota
RequestError
TimeoutError
ExecAbortError
ConnectionError
)

//export successCallback
func successCallback(channelPtr C.uintptr_t, message *C.char) {
goMessage := C.GoString(message)
goChannelPointer := uintptr(channelPtr)
resultChannel := *(*chan payload)(unsafe.Pointer(goChannelPointer))
resultChannel <- payload{value: goMessage, errMessage: nil}
}

//export failureCallback
func failureCallback(channelPtr C.uintptr_t, errMessage *C.RedisErrorFFI) {
goMessage := C.GoString(errMessage.message)
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
goChannelPointer := uintptr(channelPtr)
resultChannel := *(*chan payload)(unsafe.Pointer(goChannelPointer))
resultChannel <- payload{value: "", errMessage: fmt.Errorf("error at redis operation: %s", goMessage)}
}

func (glideRedisClient *GlideRedisClient) ConnectToRedis(request *protobuf.ConnectionRequest) error {
marshalledRequest, err := proto.Marshal(request)
if err != nil {
return fmt.Errorf("Failed to encode connection request:", err)
}
byteCount := len(marshalledRequest)
requestBytes := C.CBytes(marshalledRequest)
response := (*C.struct_ConnectionResponse)(C.create_client((*C.uchar)(requestBytes), C.uintptr_t(byteCount), (C.SuccessCallback)(unsafe.Pointer(C.successCallback)), (C.FailureCallback)(unsafe.Pointer(C.failureCallback))))
defer C.free_connection_response(response)
if response.error != nil {
defer C.free_error(response.error)
return fmt.Errorf(C.GoString(response.error.message))
}
glideRedisClient.coreClient = response.conn_ptr
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

func (glideRedisClient *GlideRedisClient) CloseClient() error {
if glideRedisClient.coreClient == nil {
return fmt.Errorf("Cannot close glide client before it has been created.")

Choose a reason for hiding this comment

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

... or already closed

}
C.close_client(glideRedisClient.coreClient)

Choose a reason for hiding this comment

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

Suggested change
C.close_client(glideRedisClient.coreClient)
C.close_client(glideRedisClient.coreClient)
glideRedisClient.coreClient = nil

return nil
}
88 changes: 88 additions & 0 deletions go/glide/lib.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
#include <stdarg.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdlib.h>

/**
* FFI compatible version of the ErrorType enum defined in protobuf.
*/
typedef enum ErrorType {
ClosingError = 0,
RequestError = 1,
TimeoutError = 2,
ExecAbortError = 3,
ConnectionError = 4,
} ErrorType;

/**
* A Redis error.
*/
typedef struct RedisErrorFFI {
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
const char *message;
enum ErrorType error_type;
} RedisErrorFFI;

/**
* The connection response.
*
* It contains either a connection or an error. It is represented as a struct instead of an enum for ease of use in the wrapper language.
jonathanl-bq marked this conversation as resolved.
Show resolved Hide resolved
*/
typedef struct ConnectionResponse {
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
const void *conn_ptr;
const struct RedisErrorFFI *error;
} ConnectionResponse;

/**
* Success callback that is called when a Redis command succeeds.
*/
typedef void (*SuccessCallback)(uintptr_t channel_address, const char *message);
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved

/**
* Failure callback that is called when a Redis command fails.
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
*/
typedef void (*FailureCallback)(uintptr_t channel_address, const struct RedisErrorFFI *error);
Copy link

@Yury-Fridlyand Yury-Fridlyand Mar 6, 2024

Choose a reason for hiding this comment

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

To ease marshalling it is better to have

Suggested change
typedef void (*FailureCallback)(uintptr_t channel_address, const struct RedisErrorFFI *error);
typedef void (*FailureCallback)(uintptr_t channel_address, ErrorType error_type, const char *message);

Otherwise we have to deal with memory allocations/deallocations for that struct pointer.

Copy link
Author

Choose a reason for hiding this comment

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

I've already implemented the proper functions for freeing this struct and the string within. We can do this, but it goes against what's in the design doc and we'll have to make sure Aaron's work is aligned with this API change as well. I think we should discuss this first with the rest of the team before changing it.

Copy link

@Yury-Fridlyand Yury-Fridlyand Mar 6, 2024

Choose a reason for hiding this comment

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

It is ok to adjust the design doc now

Copy link
Author

Choose a reason for hiding this comment

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

Is it? @aaron-congo are you aware of these changes?

Choose a reason for hiding this comment

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

I wasn't aware. We need the RedisErrorFFI and a mechanism to free it for the connection request, since we are not using a callback and C functions can only return a single result. If we need these things anyways, should we just use it in the failure callback as planned?


/**
* Creates a new client to the given address. The success callback needs to copy the given string synchronously, since it will be dropped by Rust once the callback returns. All callbacks should be offloaded to separate threads in order not to exhaust the client's thread pool.

Choose a reason for hiding this comment

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

What is the given string you are referring to here? Would it make sense to move the success callback sentence here to be above the SuccessCallback doc instead of the create_client doc?

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to remember where I copied this doc comment from originally. I think it was the C# implementation? I can still move it if you like.

*
* # Safety
*
* * `connection_request_bytes` must point to `connection_request_len` consecutive properly initialized bytes.
* * `connection_request_len` must not be greater than `isize::MAX`. See the safety documentation of [`std::slice::from_raw_parts`](https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html).
*/
const struct ConnectionResponse *create_client(const uint8_t *connection_request_bytes,
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
uintptr_t connection_request_len,
SuccessCallback success_callback,
FailureCallback failure_callback);

Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
/**
* Closes the given client, deallocating it from the heap.
*
* # Safety
*
* * `client_ptr` must be able to be safely casted to a valid `Box<Client>` via `Box::from_raw`. See the safety documentation of [`std::boxed::Box::from_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.from_raw).
* * `client_ptr` must not be null.
*/
void close_client(const void *client_ptr);

/**
* Deallocates a `ConnectionResponse`.
*
* # Safety
*
* * `connection_response_ptr` must be able to be safely casted to a valid `Box<RedisErrorFFI>` via `Box::from_raw`. See the safety documentation of [`std::boxed::Box::from_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.from_raw).
jonathanl-bq marked this conversation as resolved.
Show resolved Hide resolved
* * `connection_response_ptr` must not be null.
*/
void free_connection_response(const struct ConnectionResponse *connection_response_ptr);

/**
* Deallocates a `RedisErrorFFI`.
*
* # Safety
*
* * `error_ptr` must be able to be safely casted to a valid `Box<RedisErrorFFI>` via `Box::from_raw`. See the safety documentation of [`std::boxed::Box::from_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.from_raw).
* * The error message must be able to be safely casted to a valid `CString` via `CString::from_raw`. See the safety documentation of [`std::ffi::CString::from_raw`](https://doc.rust-lang.org/std/ffi/struct.CString.html#method.from_raw).
* * `error_ptr` must not be null.
* * The error message pointer must not be null.
*/
void free_error(const struct RedisErrorFFI *error_ptr);
1 change: 1 addition & 0 deletions go/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (

require (
github.com/BurntSushi/toml v1.2.1 // indirect
github.com/golang/protobuf v1.5.3 // indirect

Choose a reason for hiding this comment

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

Do you know why this is added? On my connect branch it is not needed since protobuf is already listed on line 7. If you do go mod tidy does this get removed?

github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/spf13/cobra v1.8.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
Expand Down
7 changes: 7 additions & 0 deletions go/go.sum
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,15 @@ github.com/golang/mock v1.3.1/go.mod h1:sBzyDLLjw3U8JLTeZvSv8jJB+tU5PVekmnlKIyFU
github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk=
github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY=
github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ=
github.com/google/btree v1.0.0/go.mod h1:lNA+9X1NB3Zf8V7Ke586lFgjr2dZNuvo3lPJSGZ5JPQ=
github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
github.com/google/go-cmp v0.5.5 h1:Khx7svrCpmxxtHBq5j2mp/xVjsi8hQMfNLvJFAlrGgU=
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs=
github.com/google/pprof v0.0.0-20181206194817-3ea8567a2e57/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
github.com/google/pprof v0.0.0-20190515194954-54271f7e092f/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc=
Expand Down Expand Up @@ -272,6 +276,7 @@ golang.org/x/tools v0.12.1-0.20230825192346-2191a27a6dc5 h1:Vk4mysSz+GqQK2eqgWbo
golang.org/x/tools v0.12.1-0.20230825192346-2191a27a6dc5/go.mod h1:Sc0INKfu04TlqNoRA1hgpFZbhYXHPr4V5DzpSBTPqQM=
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE=
google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M=
google.golang.org/api v0.8.0/go.mod h1:o4eAsZoiT+ibD93RtjEohWalFOjRDx6CVaqeizhEnKg=
Expand All @@ -293,6 +298,8 @@ google.golang.org/genproto v0.0.0-20191108220845-16a3f7862a1a/go.mod h1:n3cpQtvx
google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
google.golang.org/grpc v1.20.1/go.mod h1:10oTOabMzJvdu6/UiuZezV6QK5dSlG84ov/aaiqXj38=
google.golang.org/grpc v1.21.1/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM=
google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
google.golang.org/protobuf v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
google.golang.org/protobuf v1.32.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=
Expand Down
Loading
Loading