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

RPC implementation + FFI #461

Merged
merged 110 commits into from
Oct 24, 2024
Merged

RPC implementation + FFI #461

merged 110 commits into from
Oct 24, 2024

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Oct 5, 2024

This fully implements the new RPC feature in the Rust SDK and exposes it via FFI to other SDKs.

It has one slight rough edge as there is a new FFI dance necessary for method handling. The register_rpc_method FFI cannot store the original handler, so instead it registers a wrapper handler that dispatches into the dependent SDK to actually handle the method and then get a response or error that can be sent back up the wire. So the implementation in Node/Python/Unity/etc requires both storing the handlers locally and forwarding to FFI on registerRpcMethod. Then it waits for RpcMethodInvocationEvent and invokes its stored handlers, sending the response/error in RpcMethodInvocationRequest over FFI.

This is my first time working with Rust so lmk if I've done anything particularly wrong. The RpcHandler type in particular feels very complicated but maybe that's just Rust.

Base implementation verified with a new example. FFI implementation verified with Node implementation in livekit/node-sdks#276. Both tested locally and against cloud.

@bcherry bcherry requested a review from theomonnom October 23, 2024 19:32
@bcherry bcherry requested a review from theomonnom October 23, 2024 20:33
@bcherry bcherry requested a review from theomonnom October 23, 2024 20:47
Comment on lines 315 to 318
pub fn get_latest_join_response(&self) -> proto::JoinResponse {
self.inner.latest_join_response.clone()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this is unused, can you remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The link doesn't work for me

@@ -167,6 +183,7 @@ struct EngineInner {
lk_runtime: Arc<LkRuntime>,
engine_tx: EngineEmitter,
options: EngineOptions,
latest_join_response: proto::JoinResponse,
Copy link
Member

Choose a reason for hiding this comment

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

for this field

@bcherry bcherry merged commit 2aa9500 into main Oct 24, 2024
10 of 16 checks passed
@bcherry bcherry deleted the bcherry/rpc-full branch October 24, 2024 21:29
async fn perform_quantum_hypergeometric_series(
room: &Arc<Room>,
) -> Result<(), Box<dyn std::error::Error>> {
println!("[{}] What's the quantum hypergeometric series of 42?", elapsed_time());
Copy link

Choose a reason for hiding this comment

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

What the heck :)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋🏻

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.

3 participants