Caching of tokio backend reqwest http client - #1285
Merged
Merged
Conversation
Contributor
Changeset ✓This PR includes a changeset covering all affected packages:
|
jhugman
approved these changes
Jul 27, 2026
jhugman
left a comment
Collaborator
There was a problem hiding this comment.
This looks fine to me. FWIW the intention is that client SDK should call set_ws_client and set_http_client to re-use the client's networking stack and certificate store.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
With the native-tokio feature (enabled via livekit-api/signal-client-tokio → livekit-net/native-tokio → internal __native-tokio marker), NativeTransport::request in livekit-net/src/native/mod.rs:110 uses reqwest for HTTP. Before this branch, it built a reqwest::Client::new() inside every request; This PR replaces that with a process-wide static LazyLockreqwest::Client, so all requests share one client — and with it reqwest's internal hyper connection pool (keep-alive TCP/TLS connections). The async backend (isahc) is untouched.
Proof by benchmark
I built a throwaway bench crate (in the session scratchpad, not your repo) that runs N sequential GETs two ways: cached goes through the real livekit_net::testing::native_http_client() code path, fresh replicates the old code verbatim. A local server counts accepted TCP connections; a second scenario hits a real HTTPS endpoint so a genuine TLS handshake is in play.
The connection count is the smoking gun: the old code opened a brand-new TCP connection for every single request, the cached client opened exactly one and reused it. On loopback that only costs ~40 µs per request, but against a real HTTPS server — which is what token endpoints are — every fresh connection pays DNS + TCP + TLS handshake, making the cached client 4–5× faster per request (I ran it in both orders to rule out warm-up bias).
Alternative
The alternative is storing the Client on NativeTransport instead of in a static — same pooling benefit per registered transport, but scoped to the object's lifetime rather than the process.