Enable set_ws_client and set_http_client injectable interfaces - #1290
Enable set_ws_client and set_http_client injectable interfaces#1290jhugman wants to merge 2 commits into
Conversation
Changeset incompleteThis PR's changeset is missing version bumps for packages that are affected by the change. The following packages still require a bump:
Already covered:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
22edc1c to
1546ceb
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| /// Test probe: whether a process-wide HTTP client is registered. | ||
| #[cfg_attr(feature = "uniffi", uniffi::export)] | ||
| pub fn has_http_client() -> bool { | ||
| http_client().is_some() | ||
| } | ||
|
|
||
| /// Test probe: whether a process-wide WebSocket client is registered. | ||
| #[cfg_attr(feature = "uniffi", uniffi::export)] | ||
| pub fn has_ws_client() -> bool { | ||
| ws_client().is_some() | ||
| } |
There was a problem hiding this comment.
🔴 Registration probe reports a client even when none was registered, breaking the new test on native builds
The new probe reports that a client is present (http_client().is_some() at livekit-net/src/lib.rs:88) even when nothing was ever registered, because it counts the crate's built-in fallback, so callers relying on it get the opposite answer to what its description promises and the newly added test fails whenever the crate is built with a built-in backend.
Impact: The transport test suite fails in CI (it is run with the native backend enabled), and hosts using the probe cannot tell whether their own transport was actually installed.
Fallback resolution inside the probes vs. the OnceLock registry
has_http_client/has_ws_client delegate to http_client()/ws_client() (livekit-net/src/lib.rs:99-131), which return Some(native::native_http_client()) whenever the __native marker feature is on, regardless of whether set_http_client was called. The doc comments say "whether a process-wide HTTP client is registered".
The new test asserts the opposite at livekit-net/tests/self_test.rs:61-62 (assert!(!has_http_client())), and CI runs cargo test -p livekit-net --features native-tokio (.github/workflows/tests.yml:172), which enables __native, so both assertions fail.
A correct probe should inspect the registries directly (WS.get().is_some() / HTTP.get().is_some()), or the docs/test need to reflect the fallback.
| /// Test probe: whether a process-wide HTTP client is registered. | |
| #[cfg_attr(feature = "uniffi", uniffi::export)] | |
| pub fn has_http_client() -> bool { | |
| http_client().is_some() | |
| } | |
| /// Test probe: whether a process-wide WebSocket client is registered. | |
| #[cfg_attr(feature = "uniffi", uniffi::export)] | |
| pub fn has_ws_client() -> bool { | |
| ws_client().is_some() | |
| } | |
| /// Test probe: whether a process-wide HTTP client is registered. | |
| #[cfg_attr(feature = "uniffi", uniffi::export)] | |
| pub fn has_http_client() -> bool { | |
| HTTP.get().is_some() | |
| } | |
| /// Test probe: whether a process-wide WebSocket client is registered. | |
| #[cfg_attr(feature = "uniffi", uniffi::export)] | |
| pub fn has_ws_client() -> bool { | |
| WS.get().is_some() | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// Self-test: GET `url` via the registered HTTP client; returns the full response | ||
| /// (status + headers + body) so callers can assert the whole struct round-trips the FFI. | ||
| /// Errors if no client is registered or the transport fails. | ||
| #[cfg_attr(feature = "uniffi", uniffi::export)] | ||
| pub async fn self_test_http_get(url: String) -> Result<HttpResponse, TransportError> { | ||
| let c = | ||
| http_client().ok_or_else(|| TransportError::Other("no http client registered".into()))?; | ||
| c.request(HttpMethod::Get, url, Vec::new(), None).await | ||
| } | ||
|
|
||
| /// Self-test: connect, send `payload`, receive one frame, close; return the echoed bytes. | ||
| /// Errors if no client is registered, the transport fails, or the peer closes first. | ||
| #[cfg_attr(feature = "uniffi", uniffi::export)] | ||
| pub async fn self_test_ws_echo(url: String, payload: Vec<u8>) -> Result<Vec<u8>, TransportError> { | ||
| let c = ws_client().ok_or_else(|| TransportError::Other("no ws client registered".into()))?; | ||
| let conn = c.connect(url, Vec::new(), 5_000).await?.connection; | ||
| conn.send(payload).await?; | ||
| let got = conn.recv().await?.ok_or(TransportError::Closed)?; | ||
| conn.close().await; | ||
| Ok(got) | ||
| } | ||
|
|
||
| /// Test probe: whether a process-wide HTTP client is registered. | ||
| #[cfg_attr(feature = "uniffi", uniffi::export)] | ||
| pub fn has_http_client() -> bool { | ||
| http_client().is_some() | ||
| } | ||
|
|
||
| /// Test probe: whether a process-wide WebSocket client is registered. | ||
| #[cfg_attr(feature = "uniffi", uniffi::export)] | ||
| pub fn has_ws_client() -> bool { | ||
| ws_client().is_some() | ||
| } |
There was a problem hiding this comment.
🟡 Test-only helper functions are added to the crate's permanent public API
Four self-test helpers are made permanently public (pub async fn self_test_http_get at livekit-net/src/lib.rs:67 and the neighbouring probes) even when the bindings feature is off, so the library's supported surface grows with functions that exist only to exercise tests.
Impact: Consumers see and can depend on test scaffolding as if it were product API, which then cannot be changed without a breaking release.
AGENTS.md rule on new public API surface
AGENTS.md states: "When introducing new API surface, always default to private or pub(crate) unless there is a specific reason to expose publicly" and "Introduce new public APIs sparingly". The only stated reason for these helpers is FFI self-testing, so they should at minimum be gated behind #[cfg(feature = "uniffi")] (with the test using that feature), rather than exported unconditionally at livekit-net/src/lib.rs:63-95.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR enables the uniffi bindings for
livekit-net.This is not in use by any crates in the livekit-uniffi, so we can safely land it now. However, there should be follow ups to any SDK that uses
livekit-uniffi.Before you submit your PR
Make sure the following is true before submitting your PR:
PR description
Describe the changes in this PR. Explain what the PR is meant to solve and how to reproduce the issue in the first place.
Breaking changes
If this PR introduces breaking changes, list them here and document the rationale for introducing such a change.
MSRV
If the PR modifies the crate's MSRV (Minimum Supported Rust Version), document it here.
Testing
Ideally, unit test the code you add, but ensure you're not repeating existing test cases. Use as many already written scaffolding, utilities as possible; write your own, when needed. If external services, APIs, tokens are required (e.g., running an LK server instance), provide the necessary information. Make sure your tests perform useful, context-aware assertions and do not simply emulate "happy paths".
Async
We want the project to be runtime-agnostic, so please reuse what's already in livekit-runtime and feel free to add anything missing. It's ok to use Tokio directly, when writing unit tests, if necessary. When testing, do not use artificial delays for the state to "catch up"; instead, respect the event flow and subscribe properly using channels or other mechanisms.