Inital Blocking "RPC" Proof of Concept #87
Conversation
Benchmark for ac44f9cClick to view benchmark
|
nonblock-rpc/android/app/src/main/jniLibs/arm64-v8a/librpc_core.so
Outdated
Show resolved
Hide resolved
Benchmark for 47b2a50Click to view benchmark
|
nonblock-rpc/rust/Cargo.toml
Outdated
There was a problem hiding this comment.
The Rust crate should be located directly in ark-rust/nonblock-rpc not in /rust
There was a problem hiding this comment.
Actually, we might need split the crate in 2 parts: one in this repo, one in ark-android repo.
On other hand, we have some bindings in this repo.
@tareknaser what should be final structure of the crate?
There was a problem hiding this comment.
About the crate name, maybe ark-rpc or just rpc?
There was a problem hiding this comment.
what should be final structure of the crate?
I think we should keep the bindings and the code that generates them in the same repo, since they depend on each other. For example, the command to build the bindings is:
cargo run --bin uniffi-bingen generate --library ./target/debug/rpc_core.dll --language kotlin --out-dir ../android/app/src/main/java/ark/rpc_coreThis relies on the Rust code being built in the current folder, so any changes to the code would affect the bindings. That's why they should stay in the same Git repo.
I see two options:
- Keep all the Rust code in
ark-core/nonblock-rpcand the bindings inark-core/nonblock-rpc/android. - Move the crate and bindings to a new repo. The crate can still depend on other
ark-corecrates as external dependencies.
There was a problem hiding this comment.
About the crate name, maybe
ark-rpcor justrpc?
I think rpc, block-rpc, or non-block-rpc would work well.
All the crates in this repository are related to ARK, it doesn't need ark in the name.
There was a problem hiding this comment.
I think RPC makes more sense since in the future we want this one crate to support both blocking and non-blocking requests
There was a problem hiding this comment.
Perhaps a different name like dispatcher or function registry will be more appropriate since this a single process method dispatch. The same name can be used for both sync and async.
nonblock-rpc/rust/Cargo.toml
Outdated
There was a problem hiding this comment.
We should also include it in the ark-rust Cargo.toml file so it's part of the cargo workspace.
There was a problem hiding this comment.
I think ark-rust is the older name of this repo, new one is ark-core. Just for clarity, technically we can split the repos by languages again. I don't remember why we decided to keep java folder here.
There was a problem hiding this comment.
I don't remember why we decided to keep
javafolder here.
Because they are bindings to the Rust code in this repository. For example, here, we are using the compiled Rust code to build the bindings:
// Set the JVM argument for the java.library.path (To import rust compiled library)
val rustLibPath = projectDir.resolve("../../target/release").absolutePath
Keeping the bindings in the same crate allows us to treat everything as one bundle of code. For example, we want the CI to run Java bindings checks for each PR in this repo, and that wouldn’t be possible if the bindings were in a different repository (as far as I know).
Another approach would be to move the code that generates the bindings in Rust to a separate repository that depends on ark-core. For instance, files in the fs-storage/src/jni folder could be moved to their own repository, with an external dependency on ark-core.
The benefit of this is better organization, as we would divide the repositories by language. However, the downside is that we would have to maintain them separately. Any changes to the API in ark-core would need to be manually propagated in the other repository.
nonblock-rpc/rust/Cargo.toml
Outdated
| [workspace] | ||
|
|
||
| members = [ | ||
| "core", | ||
| "uniffi-bindgen", | ||
| ] |
There was a problem hiding this comment.
This setup creates nested Rust workspaces, which is generally discouraged.
Do you think there's a way to refactor this? Perhaps uniffi-bindgen could be included as a binary within the same crate.
nonblock-rpc/README.md
Outdated
There was a problem hiding this comment.
Thanks for the build guide! Could we also add some usage instructions and perhaps an example to show the crate in action?
There was a problem hiding this comment.
Thanks for including the full integration test with Kotlin. Having a couple of pure Rust tests that use the Router will also be very helpful. They will also be useful in quickly testing out changes. One other thing to consider is to include tests that call HandlerFunctions with different number of arguments like 3-4.
nonblock-rpc/README.md
Outdated
| ### Build the dylib | ||
|
|
||
| ```sh | ||
| cargo build |
There was a problem hiding this comment.
Could we consider adding the --release flag as well?
| cargo build | |
| cargo build --release |
nonblock-rpc/rust/core/src/api.rs
Outdated
| Some(handler) => { | ||
| handler.call(data) | ||
| }, | ||
| None => "Unknown function".to_string(), |
There was a problem hiding this comment.
We're currently suppressing errors here. This method should return Result<String>, where the error type is a uniffi error type instead.
nonblock-rpc/rust/core/src/router.rs
Outdated
| #[derive(Serialize)] | ||
| pub struct Response<T> { | ||
| pub result: Option<T>, | ||
| pub error: Option<String>, | ||
| pub is_success: bool, | ||
| } |
There was a problem hiding this comment.
The fields of Response<T> shouldn't be public.
nonblock-rpc/rust/core/src/router.rs
Outdated
| pub fn call(&self, name: &str, args: Vec<String>) -> String { | ||
| match self.routes.get(name) { | ||
| Some(handler) => handler.call(args), | ||
| None => "Unknown function".to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Unknown function should be wrapped in an error type. The call() method should return Result<String> instead.
nonblock-rpc/rust/core/src/router.rs
Outdated
| pub is_success: bool, | ||
| } | ||
|
|
||
| const CATASTROPHIC_ERROR: &str = "{\"result\": null, \"error\": \"CATASTROPHIC_ERROR: Failed to serialize response\", \"is_success\": false}"; |
There was a problem hiding this comment.
‘CATASTROPHIC_ERROR’ sounds pretty intense 😃
Is there a specific reason for using such a strong term?
By the way, I think this might be something that could be serialized into JSON.
nonblock-rpc/rust/core/src/api.rs
Outdated
| use crate::ROUTER; | ||
|
|
||
| #[uniffi::export] | ||
| pub fn call(path: String, data: Vec<String>) -> String { |
There was a problem hiding this comment.
Nit: I think the term 'path' might be a bit confusing here. Perhaps 'route' would be a better fit.
Benchmark for f9a3451Click to view benchmark
|
Benchmark for a2e89ecClick to view benchmark
|
Benchmark for f5947faClick to view benchmark
|
Benchmark for 61336a4Click to view benchmark
|
Benchmark for 56bcdbbClick to view benchmark
|
Benchmark for 6f83905Click to view benchmark
|
Benchmark for 24c5c06Click to view benchmark
|
|
@kirillt suggested I take a look at this for a review. I do not have full context on it's implementation, so please help me in my understanding. Having implemented something similar previously, it seems to be a "client-server" like architecture where kotlin code is the client and can send requests to the rust server which can send back responses. The benefit of this approach is that it greatly simplifies dealing with generics because request/responses are strings or vec of strings. I have some points to offer to enhance the understand-ability and maintainability of this module.
These terms have standard meanings which will make it much easier to understand and follow the logic. Of course some documentation will also help. I found the |
|
@twitu good writeup, thanks. Yes, your understanding about client-server architecture is correct, and indeed we aim at applying it to a probably simpler task, where both sides are located not only on same device but also in same process. I like your idea for the naming, "dispatch" sounds like a better word. Regarding "callback" term: technically, you are correct but it also might be an oversimplification. I like the terminology taken from Web, with "requests" and "responses". Especially, because we're aiming on async handling as well. Although when we implement subscription pattern, such a terminology could stand in our way. Could you please split this list into smaller comments and leave them on related lines, so it would be easier to discuss and address? |
There was a problem hiding this comment.
I've left a few comments on naming and design. If this works well then it's possible that we don't need JNI bindings for individual storages. The storages can be registered to this dispatcher and the application and can send "requests" to the dispatcher and get back "responses". This will also eliminate the problem of exposing generic bindings.
Is the plan to replace JNI bindings with this style?
| } | ||
| } | ||
| } | ||
| pub trait HandlerFunction<Marker>: Send + Sync + 'static { |
There was a problem hiding this comment.
Renaming this to Callback will be more appropriate because it represents a function that will be boxed and put on the heap and called with some value.
Moreover, having a different name will reduce confusion between it and FunctionHandler.
| impl_handler_function!(T0, T1, T2, T3); | ||
| impl_handler_function!(T0, T1, T2, T3, T4); | ||
|
|
||
| struct FunctionHandler<F, Marker> { |
There was a problem hiding this comment.
Renaming this to RequestHandler will be more appropriate since it will accept a request and pass it to a "callback". It will also help differentiate it from HandlerFunction which can renamed as Callback.
| } | ||
| } | ||
|
|
||
| pub trait Handler { |
There was a problem hiding this comment.
I don't understand what this is doing and how it is different from HandlerFunction.
| impl Router { | ||
| pub fn new() -> Self { | ||
| Router { | ||
| routes: HashMap::new(), |
There was a problem hiding this comment.
Pub-sub pattern can be supported by adding a routing table here which is a HashMap that stores the topic and a Vec of callbacks - HashMap<String, Vec<HandlerFunction>>. And exposing publish and subscribe, unsubscribe that allow publishing data and modifying subscriptions.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| pub struct Router { | ||
| pub routes: HashMap<String, Box<dyn Handler + 'static + Send + Sync>>, |
There was a problem hiding this comment.
Consider running the Router in a separate thread and using a single-thread per-core runtime like tokio-uring. This will make the code and types significantly less complex - you will be able to remove 'static, Send and Sync while still using an async runtime.
https://emschwartz.me/async-rust-can-be-a-pleasure-to-work-with-without-send-sync-static/
| } | ||
| } | ||
|
|
||
| macro_rules! impl_handler_function { |
There was a problem hiding this comment.
Some documentation for this will be very helpful.
Benchmark for aeeb442Click to view benchmark
|
Benchmark for 1967138Click to view benchmark
|
No description provided.