Go: Implement connection logic in lib.rs #119

jonathanl-bq · 2024-02-29T18:20:10Z

No description provided.

go/.gitignore

Yury-Fridlyand · 2024-02-29T18:47:12Z

go/glide/glide.go

+    C.command(glideRedisClient.coreClient, C.uintptr_t(resultChannelPtr), requestType, C.uintptr_t(1), &cArgs[0])
+    resultPayload := <-resultChannel
+
+    return resultPayload.value, nil


Suggested change

return resultPayload.value, nil

return resultPayload.value, resultPayload.errMessage

Will deal with this later when we add command logic

go/glide/lib.h

go/go.sum

go/glide/lib.h

go/src/lib.rs

acarbonetto

Let's merge this PR with Aaron's glide.go PR.
Add IT tests for connection.
Remove the set/get/ping/info content for a follow-up PR.

go/examples/main.go

acarbonetto · 2024-02-29T19:15:40Z

go/examples/main.go

+		request.Addresses,
+		&protobuf.NodeAddress{Host: "localhost", Port: uint32(6379)},
+	)
+	err := client.ConnectToRedis(request)


what happens if a user calls client.set() before they call ConnectToRedis?

go/examples/main.go

go/src/lib.rs

Yury-Fridlyand · 2024-03-06T01:21:41Z

go/glide/lib.h

+/**
+ * Failure callback that is called when a Redis command fails.
+ */
+typedef void (*FailureCallback)(uintptr_t channel_address, const struct RedisErrorFFI *error);


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.

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.

It is ok to adjust the design doc now

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

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?

go/glide/lib.h

go/glide/glide.go

Yury-Fridlyand · 2024-03-06T03:13:45Z

go/glide/glide.go

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


... or already closed

Yury-Fridlyand · 2024-03-06T03:14:27Z

go/glide/glide.go

+	if glideRedisClient.coreClient == nil {
+		return fmt.Errorf("Cannot close glide client before it has been created.")
+	}
+	C.close_client(glideRedisClient.coreClient)


Suggested change

C.close_client(glideRedisClient.coreClient)

C.close_client(glideRedisClient.coreClient)

glideRedisClient.coreClient = nil

go/glide/glide.go

Yury-Fridlyand · 2024-03-06T03:20:40Z

go/src/lib.rs

+}
+
+/// FFI compatible version of the ErrorType enum defined in protobuf.
+// TODO: Update when command errors are implemented


Add valkey-io#1077 reference for TODO
We will remove this enum after valkey-io#1077

1077 merged, you can update your code and remove TODO

go/src/lib.rs

Yury-Fridlyand · 2024-03-06T03:29:48Z

go/src/lib.rs

+            let msg = CString::new(err.to_string()).unwrap();
+            RedisErrorFFI {
+                message: msg.into_raw(),
+                error_type: ErrorType::ConnectionError,
+            }


Do you want to make a function for this? You repeat it 3 times

go/src/lib.rs

go/examples/main.go

go/glide/lib.h

aaron-congo · 2024-03-06T19:44:34Z

go/glide/lib.h

+typedef void (*FailureCallback)(uintptr_t channel_address, const struct RedisErrorFFI *error);
+
+/**
+ * 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.


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?

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.

go/glide/lib.h

aaron-congo · 2024-03-06T19:50:51Z

go/go.mod

@@ -10,6 +10,7 @@ require (

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


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?

go/src/lib.rs

Co-authored-by: Aaron <[email protected]>

Co-authored-by: Yury-Fridlyand <[email protected]>

Co-authored-by: Aaron <[email protected]>

go/src/lib.rs

Yury-Fridlyand · 2024-03-07T02:15:40Z

go/src/lib.rs

+        Ok(client) => ConnectionResponse {
+            conn_ptr: Box::into_raw(Box::new(client)) as *const c_void,
+            error_message: std::ptr::null(),
+            error_type: RequestErrorType::Unspecified,


Should we ash Shachar to define NoError enum variant? (later)

I think it would be nice. We'll see what he thinks though.

Yury-Fridlyand · 2024-03-07T02:17:08Z

CI is red

aaron-congo · 2024-03-08T01:16:48Z

go/src/lib.rs

+
+/// Deallocates a `ConnectionResponse`.
+///
+/// This function does not free the contained error, which needs to be freed separately using `free_error` afterwards to avoid memory leaks.


Looks like this doc will have to get updated since it now frees the error

Yury-Fridlyand · 2024-03-08T23:31:53Z

go/glide/lib.h

+ * * `error_msg_ptr` 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_msg_ptr` must not be null.
+ */
+void free_error(const char *error_msg_ptr);


Why re-expose?

I assumed that the failure callback would use it for the error it gets for commands. Not relevant for connection logic though, so I guess it can be reverted. Up to you, since you have all the changes in your branch now.

jonathanl-bq · 2024-03-10T03:45:32Z

Closing as all relevant changes are integrated into other branches now.

jonathanl-bq added 5 commits February 27, 2024 10:15

Add Rust connection logic and example

c0cb6fb

Add error handling logic for connections

22bbf74

Add license headers

79343ff

Refactor cArgs

2a92415

Update API for client creation

3278942

jonathanl-bq requested review from aaron-congo, Yury-Fridlyand, acarbonetto, Elen-Ghulam and SanHalacogluImproving February 29, 2024 18:20

Yury-Fridlyand reviewed Feb 29, 2024

View reviewed changes

acarbonetto reviewed Feb 29, 2024

View reviewed changes

Yury-Fridlyand reviewed Feb 29, 2024

View reviewed changes

go/src/lib.rs Outdated Show resolved Hide resolved

aaron-congo reviewed Feb 29, 2024

View reviewed changes

go/src/lib.rs Outdated Show resolved Hide resolved

aaron-congo reviewed Feb 29, 2024

View reviewed changes

go/src/lib.rs Outdated Show resolved Hide resolved

jonathanl-bq added 3 commits March 3, 2024 23:00

Remove non-connection logic

2b258e5

Update lib.h

6ccc6c1

Add additional documentation to lib.rs functions

bdf9c19

Yury-Fridlyand reviewed Mar 6, 2024

View reviewed changes

jonathanl-bq added 2 commits March 5, 2024 17:36

Properly free connection response and error

4bd25cc

Run go fmt

dc4df76

Yury-Fridlyand reviewed Mar 6, 2024

View reviewed changes

go/glide/lib.h Show resolved Hide resolved

go/glide/lib.h Show resolved Hide resolved

jonathanl-bq added 3 commits March 5, 2024 18:02

Add error for when user closes client before creating it

cd5c2da

Add doc comments about avoiding memory leaks

055e74b

Allow unused Client fields for now

f23b01a

Yury-Fridlyand reviewed Mar 6, 2024

View reviewed changes

Add formatting directive to fmt.Errorf

aa1fb08

aaron-congo reviewed Mar 6, 2024

View reviewed changes

jonathanl-bq added 2 commits March 6, 2024 14:16

Replace callback implementations with TODO implementations

2fe5bdd

Resolve merge conflicts

6289bfb

jonathanl-bq and others added 6 commits March 6, 2024 17:45

Remove RedisErrorFFI struct and use glide-core RequetErrorType instead

53c8d7d

Update go/glide/lib.h

926e050

Co-authored-by: Aaron <[email protected]>

Update go/glide/lib.h

b9e61c8

Co-authored-by: Aaron <[email protected]>

Update go/src/lib.rs

70fb74b

Co-authored-by: Yury-Fridlyand <[email protected]>

Update go/src/lib.rs

db8959b

Co-authored-by: Aaron <[email protected]>

Update documentation for create_client

b32408c

Yury-Fridlyand approved these changes Mar 7, 2024

View reviewed changes

aaron-congo approved these changes Mar 7, 2024

View reviewed changes

jonathanl-bq added 2 commits March 7, 2024 10:24

Adjust documentation regarding memory leaks

478acd6

Make free_connection_response also free its error

0e52d91

aaron-congo reviewed Mar 8, 2024

View reviewed changes

jonathanl-bq added 2 commits March 8, 2024 13:31

Update doc comment for free_connection_response

a02912f

Re-expose free_error for the error callback to use and update docs

7ea7bdd

Yury-Fridlyand mentioned this pull request Mar 8, 2024

Aggregate FFI client code in one common place. #133

Draft

Yury-Fridlyand reviewed Mar 8, 2024

View reviewed changes

jonathanl-bq closed this Mar 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: Implement connection logic in lib.rs #119

Go: Implement connection logic in lib.rs #119

jonathanl-bq commented Feb 29, 2024

Yury-Fridlyand Feb 29, 2024

jonathanl-bq Mar 5, 2024

acarbonetto left a comment

acarbonetto Feb 29, 2024

Yury-Fridlyand Mar 6, 2024 •

edited

Loading

jonathanl-bq Mar 6, 2024

Yury-Fridlyand Mar 6, 2024 •

edited

Loading

jonathanl-bq Mar 6, 2024

aaron-congo Mar 6, 2024

Yury-Fridlyand Mar 6, 2024

Yury-Fridlyand Mar 6, 2024

Yury-Fridlyand Mar 6, 2024

Yury-Fridlyand Mar 6, 2024

Yury-Fridlyand Mar 6, 2024

aaron-congo Mar 6, 2024

jonathanl-bq Mar 7, 2024

aaron-congo Mar 6, 2024

Yury-Fridlyand Mar 7, 2024

jonathanl-bq Mar 7, 2024

Yury-Fridlyand commented Mar 7, 2024

aaron-congo Mar 8, 2024

Yury-Fridlyand Mar 8, 2024

jonathanl-bq Mar 10, 2024

jonathanl-bq commented Mar 10, 2024

	return resultPayload.value, nil
	return resultPayload.value, resultPayload.errMessage

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

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

Go: Implement connection logic in lib.rs #119

Go: Implement connection logic in lib.rs #119

Conversation

jonathanl-bq commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acarbonetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanl-bq commented Mar 10, 2024

Yury-Fridlyand Mar 6, 2024 •

edited

Loading

Yury-Fridlyand Mar 6, 2024 •

edited

Loading