refactor(torii): torii-runner #2881
Conversation
|
Warning Rate limit exceeded@Larkooo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughOhayo, sensei! The pull request introduces a significant refactoring of the Torii binary, centralizing its execution logic into a new Changes
Sequence DiagramsequenceDiagram
participant Main as Main Application
participant Runner as Runner
participant SQLite as SQLite Database
participant JSONRPC as JSON-RPC Client
participant Executor as Transaction Executor
participant Server as Servers
Main->>Runner: Create with arguments
Runner->>Runner: Validate world address
Runner->>Runner: Setup logging
Runner->>SQLite: Initialize connection pool
Runner->>JSONRPC: Create client
Runner->>Executor: Initialize
Runner->>Server: Configure and spawn (GraphQL, gRPC, Relay)
Runner->>Runner: Await task completion or shutdown
Possibly related PRs
Suggested reviewers
Finishing Touches
🪧 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: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(1 hunks)bin/torii/Cargo.toml(1 hunks)bin/torii/src/main.rs(1 hunks)crates/torii/runner/Cargo.toml(1 hunks)crates/torii/runner/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (5)
bin/torii/src/main.rs (1)
21-23: Ohayo, sensei! Great modularization with theRunnerstructEncapsulating the application initialization into
Runnerenhances the codebase's modularity and maintainability.bin/torii/Cargo.toml (1)
8-11: Ohayo, sensei! Nice reduction of dependenciesStreamlining the dependencies enhances build times and reduces complexity.
crates/torii/runner/Cargo.toml (2)
1-6: Ohayo! Package configuration looks good, sensei!The package metadata properly inherits workspace settings while maintaining its unique identity.
60-63: Features configuration is well-organized, sensei!The default features and their propagation to underlying crates are properly configured.
Cargo.toml (1)
128-128: Ohayo! The workspace dependency addition looks perfect, sensei!The
torii-runneris properly integrated into the workspace dependencies with the correct path.
| // Set the number of threads based on CPU count | ||
| let cpu_count = std::thread::available_parallelism().unwrap().get(); | ||
| let thread_count = cmp::min(cpu_count, 8); | ||
| sqlx::query(&format!("PRAGMA threads = {};", thread_count)).execute(&pool).await?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sensei, consider parameterizing SQL queries to prevent injection risks
While thread_count is an internal value, using parameterized queries enhances security and follows best practices.
Apply this diff to use a parameterized query:
-sqlx::query(&format!("PRAGMA threads = {};", thread_count)).execute(&pool).await?;
+sqlx::query("PRAGMA threads = ?;").bind(thread_count).execute(&pool).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sqlx::query(&format!("PRAGMA threads = {};", thread_count)).execute(&pool).await?; | |
| sqlx::query("PRAGMA threads = ?;").bind(thread_count).execute(&pool).await?; |
| futures.workspace = true | ||
| http-body = "0.4.5" | ||
| http.workspace = true | ||
| hyper-reverse-proxy = { git = "https://github.com/tarrencev/hyper-reverse-proxy" } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! Yes, please pin that git dependency to commit 4bfaf98, sensei!
The repository has no tags or releases, making it even more important to pin to a specific commit. Update the dependency to:
hyper-reverse-proxy = { git = "https://github.com/tarrencev/hyper-reverse-proxy", rev = "4bfaf98b7ae61a49c6238ee1bd38ad030e7fa7f6" }🔗 Analysis chain
Consider pinning the git dependency to a specific commit, sensei!
Using a git dependency without a specific commit or tag for hyper-reverse-proxy could lead to reproducibility issues.
Run this script to check the dependency's commit history:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the commit history of the hyper-reverse-proxy dependency
gh repo view tarrencev/hyper-reverse-proxy --json updatedAt,latestTag,defaultBranchRef \
--jq '{
lastUpdated: .updatedAt,
latestTag: .latestTag.name,
defaultBranch: .defaultBranchRef.name
}'
Length of output: 1344
Script:
#!/bin/bash
# Check if repo exists and get basic info
echo "=== Repository Info ==="
gh repo view tarrencev/hyper-reverse-proxy --json updatedAt,pushedAt,url 2>/dev/null || echo "Repository not found"
echo -e "\n=== Latest Release Info ==="
gh repo view tarrencev/hyper-reverse-proxy --json latestRelease --jq '.latestRelease' 2>/dev/null || echo "No release info available"
echo -e "\n=== Recent Commits ==="
git ls-remote https://github.com/tarrencev/hyper-reverse-proxy.git HEAD refs/tags/* 2>/dev/null || echo "Cannot fetch git references"
Length of output: 612
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/torii/runner/src/lib.rs (3)
83-86: Consider handling send errors in the Ctrl-C handlerThe current implementation silently ignores send errors. While this might be acceptable for shutdown scenarios, logging the error could help with debugging.
- let _ = shutdown_tx_clone.send(()); + if let Err(e) = shutdown_tx_clone.send(()) { + eprintln!("Failed to send shutdown signal: {}", e); + }
259-266: Add documentation for metrics endpoint configurationWhile the metrics setup is well-implemented, adding documentation about the metrics endpoint and available metrics would improve maintainability.
Consider adding a doc comment:
+ // Start the metrics server if metrics are enabled + // Exposes Prometheus metrics at http://{metrics_addr}:{metrics_port}/metrics + // Available metrics: + // - torii_process_* : Process-level metrics + // - Add other available metrics here if self.args.metrics.metrics {
277-286: Consider adding timeout handling for long-running tasksThe current implementation waits indefinitely for tasks to complete. Adding a timeout mechanism could prevent hanging in case of task deadlocks.
Consider adding a timeout:
- tokio::select! { + tokio::select! { + _ = tokio::time::sleep(Duration::from_secs(300)) => { + error!(target: LOG_TARGET, "Timeout waiting for tasks to complete"); + return Ok(()); + } res = engine_handle => res??, res = executor_handle => res??, // ... other tasks ... };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)bin/torii/Cargo.toml(1 hunks)crates/torii/runner/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (3)
bin/torii/Cargo.toml (1)
7-9: Ohayo! Clean dependency management, sensei!The reduction in dependencies and addition of workspace-level dependencies
torii-cliandtorii-runneraligns well with the refactoring goals, making the package structure more modular and maintainable.crates/torii/runner/src/lib.rs (2)
120-120: Ohayo sensei! Consider parameterizing SQL queries to prevent injection risksWhile
thread_countis an internal value, using parameterized queries enhances security and follows best practices.
59-63: Clean error handling, sensei!The early validation of the world address with a clear error message is a good practice.
| async fn spawn_rebuilding_graphql_server( | ||
| shutdown_tx: Sender<()>, | ||
| pool: Arc<SqlitePool>, | ||
| proxy_server: Arc<Proxy>, | ||
| ) { | ||
| let mut broker = SimpleBroker::<Model>::subscribe(); | ||
|
|
||
| loop { | ||
| let shutdown_rx = shutdown_tx.subscribe(); | ||
| let (new_addr, new_server) = torii_graphql::server::new(shutdown_rx, &pool).await; | ||
|
|
||
| tokio::spawn(new_server); | ||
|
|
||
| proxy_server.set_graphql_addr(new_addr).await; | ||
|
|
||
| // Break the loop if there are no more events | ||
| if broker.next().await.is_none() { | ||
| break; | ||
| } else { | ||
| tokio::time::sleep(Duration::from_secs(1)).await; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in GraphQL server spawning
The current implementation has two potential issues:
- No error handling for server creation
- Fixed sleep duration might not be optimal for all scenarios
Consider:
- Adding error handling for server creation
- Making the sleep duration configurable
async fn spawn_rebuilding_graphql_server(
shutdown_tx: Sender<()>,
pool: Arc<SqlitePool>,
proxy_server: Arc<Proxy>,
+ retry_delay: Option<Duration>,
) {
let mut broker = SimpleBroker::<Model>::subscribe();
+ let delay = retry_delay.unwrap_or(Duration::from_secs(1));
loop {
let shutdown_rx = shutdown_tx.subscribe();
- let (new_addr, new_server) = torii_graphql::server::new(shutdown_rx, &pool).await;
+ let server_result = torii_graphql::server::new(shutdown_rx, &pool).await;
+ match server_result {
+ Ok((new_addr, new_server)) => {
+ tokio::spawn(new_server);
+ proxy_server.set_graphql_addr(new_addr).await;
+ }
+ Err(e) => {
+ error!(target: LOG_TARGET, "Failed to create GraphQL server: {}", e);
+ }
+ }
- tokio::spawn(new_server);
-
- proxy_server.set_graphql_addr(new_addr).await;
-
if broker.next().await.is_none() {
break;
} else {
- tokio::time::sleep(Duration::from_secs(1)).await;
+ tokio::time::sleep(delay).await;
}
}
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2881 +/- ##
==========================================
+ Coverage 55.78% 55.80% +0.01%
==========================================
Files 448 449 +1
Lines 57735 57720 -15
==========================================
Hits 32209 32209
+ Misses 25526 25511 -15 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
Dependency Updates
torii-runneras a new workspace dependencyArchitecture Changes
Runnerclass to manage application initialization and executionPerformance Improvements