feat: add keepalive to Torii client gRPC connections #2690
Conversation
WalkthroughOhayo, sensei! The changes introduce a new optional parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WorldClient
participant Endpoint
Client->>WorldClient: new(dst, world_address, keepalive)
WorldClient->>Endpoint: create with dst
alt Not wasm32
Endpoint->>Endpoint: tcp_keepalive(Some(Duration::from_secs(60)))
end
WorldClient->>Client: Return Result
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/torii/client/src/client/mod.rs (1)
44-46: Consider adding documentation for the keepalive parameter, sensei!The optional keepalive parameter is well-designed, but it would benefit from documentation explaining:
- The purpose of the keepalive parameter
- The expected duration range
- What happens when None is provided
Add this documentation above the parameter:
pub async fn new( torii_url: String, rpc_url: String, relay_url: String, world: Felt, + /// Duration between keepalive checks for maintaining gRPC connection resilience. + /// When None is provided, the default gRPC keepalive settings are used. keepalive: Option<Duration>, ) -> Result<Self, Error> {crates/torii/grpc/src/client.rs (2)
53-57: Ohayo sensei! Please add documentation for the keepalive parameter.The new keepalive parameter would benefit from documentation explaining:
- Its purpose in maintaining connection resilience
- Expected duration values
- When to use it (e.g., in unstable network conditions)
Example doc comment:
/// Creates a new WorldClient instance. /// /// # Arguments /// * `dst` - The destination URL for the gRPC connection /// * `world_address` - The Felt address of the world /// * `keepalive` - Optional duration between keepalive checks. Use this in unstable network conditions /// to maintain connection resilience. Recommended values: 10-30 seconds.
58-60: Consider validating keepalive duration values.While the implementation is correct, consider adding validation for the keepalive duration to prevent potential issues with very short intervals.
let endpoint = Endpoint::from_shared(dst.clone()) .map_err(|e| Error::Endpoint(e.to_string()))? + .tcp_keepalive(keepalive.and_then(|d| { + if d.as_secs() < 5 { + None // Ignore too short intervals + } else { + Some(d) + } + }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/client/src/client/mod.rs(2 hunks)crates/torii/grpc/src/client.rs(2 hunks)
🔇 Additional comments (4)
crates/torii/client/src/client/mod.rs (2)
4-4: Ohayo! Clean import addition, sensei!
The Duration import is properly placed with other std imports and correctly supports the new keepalive functionality.
47-48: Verify WorldClient implementation, sensei!
The keepalive parameter is correctly passed to WorldClient::new, but let's verify the implementation.
✅ Verification successful
Keepalive implementation is properly handled, sensei!
The WorldClient::new implementation correctly:
- Takes the keepalive parameter as
Option<Duration> - Applies it to the endpoint configuration via
tcp_keepalive(keepalive) - Properly handles errors during connection setup
- Has appropriate cfg-gated implementations for both native and wasm targets
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check WorldClient implementation and error handling
# Find WorldClient::new implementation
ast-grep --pattern 'impl WorldClient {
$$$
async fn new($$$) {
$$$
}
$$$
}'
# Check for any keepalive-related error handling
rg -A 5 'tcp_keepalive|keep_alive'
Length of output: 646
Script:
#!/bin/bash
# Let's check the full WorldClient implementation to understand the keepalive handling
# Find the complete WorldClient implementation
ast-grep --pattern 'impl WorldClient {
$$$
}'
# Also check for any keepalive-related struct fields or types
rg -B 2 -A 2 'struct WorldClient'
Length of output: 15610
crates/torii/grpc/src/client.rs (2)
3-3: Ohayo! Clean import addition.
The Duration import is properly placed and necessary for the keepalive functionality.
53-60: Verify impact on existing client code.
The addition of the keepalive parameter requires updates to all existing WorldClient::new calls.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2690 +/- ##
==========================================
- Coverage 57.46% 57.23% -0.23%
==========================================
Files 400 408 +8
Lines 49963 51792 +1829
==========================================
+ Hits 28709 29644 +935
- Misses 21254 22148 +894 ☔ View full report in Codecov by Sentry. |
crates/torii/grpc/src/client.rs
Outdated
| Endpoint::from_shared(dst.clone()).map_err(|e| Error::Endpoint(e.to_string()))?; | ||
| let endpoint = Endpoint::from_shared(dst.clone()) | ||
| .map_err(|e| Error::Endpoint(e.to_string()))? | ||
| .tcp_keepalive(Some(Duration::from_secs(30))); |
There was a problem hiding this comment.
On what this value is based @edisontim? Something arbitrary that should work, or it's a value that is expected to work in most cases?
There was a problem hiding this comment.
It was an arbitrary value but after a quick research the recommended time is from 45-60 secs.
I'll set it to 60 secs as to not spam the server
Description
Allows grpc subscriptions to survive network issues by introducing a keepalive parameter, the
Somevalue represents the delay between keepalive checks between the grpc client and server.Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
keepaliveparameter for enhanced TCP keepalive configuration in the gRPC client.Bug Fixes