Fix async connection leak and co #72
Conversation
- Fix compilation warnings "warning: elided lifetime has a name" - Update version in readme
WalkthroughThe pull request introduces several changes to the FalkorDB Rust client, primarily focusing on updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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
CodeRabbit Configuration File (
|
![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 (1)
src/connection/asynchronous.rs (1)
119-119: Consider handling potential errors in the spawned task.While spawning a task to return the connection to the pool is a good fix for the connection leak, the error from
return_to_pool()is silently ignored. Consider logging the error if the return fails.-tokio::spawn(async { self.return_to_pool().await }); +tokio::spawn(async { + if let Err(e) = self.return_to_pool().await { + tracing::error!("Failed to return connection to pool: {}", e); + } +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
README.md(6 hunks)src/connection/asynchronous.rs(1 hunks)src/graph/blocking.rs(5 hunks)
🔇 Additional comments (2)
README.md (1)
24-24: LGTM! Version updates are consistent.
The version has been correctly updated from 0.1.8 to 0.1.9 across all feature combinations in the installation instructions.
Also applies to: 72-72, 126-126, 130-130, 136-136, 140-140, 149-149
src/graph/blocking.rs (1)
113-113: LGTM! Lifetime parameters correctly added.
The addition of explicit lifetime parameters to the return types improves type safety and addresses the compilation warnings about elided lifetimes.
Let's verify that all similar methods have been updated:
Also applies to: 128-128, 159-159, 175-175, 191-191
✅ Verification successful
LGTM! All lifetime parameters are correctly added and consistent
After reviewing the file content, I can confirm that all QueryBuilder return types have been properly updated with lifetime parameters where needed. The changes are consistent across all methods including:
profile(line 113)explain(line 128)ro_query(line 159)call_procedure(line 175)call_procedure_ro(line 191)
The lifetime parameters are correctly propagated through the return types and match the input parameters, ensuring proper lifetime tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining methods that might need lifetime parameters
# Search for methods that might still have elided lifetimes
ast-grep --pattern 'fn $name<$_>($_: &$_) -> QueryBuilder<$ret, $_, $_>'
Length of output: 74
Script:
#!/bin/bash
# Let's try a more specific pattern to find QueryBuilder return types
ast-grep --pattern 'fn $_($_) -> QueryBuilder<$_, $_, $_>'
# Also search for the specific file content to verify the changes
cat src/graph/blocking.rs
Length of output: 21311
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
==========================================
+ Coverage 84.93% 85.11% +0.18%
==========================================
Files 28 28
Lines 3909 3910 +1
Branches 3909 3910 +1
==========================================
+ Hits 3320 3328 +8
+ Misses 399 390 -9
- Partials 190 192 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/graph/asynchronous.rs (2)
Line range hint
38-47: Consider enhancing error handling for connection failuresWhile the connection borrowing logic is sound, consider adding specific error handling for connection failures to improve reliability. This could help prevent connection leaks when errors occur.
Example enhancement:
async fn execute_command( &self, command: &str, subcommand: Option<&str>, params: Option<&[&str]>, ) -> FalkorResult<redis::Value> { - self.client - .borrow_connection(self.client.clone()) - .await? - .execute_command(Some(self.graph_name.as_str()), command, subcommand, params) - .await + let connection = match self.client.borrow_connection(self.client.clone()).await { + Ok(conn) => conn, + Err(e) => { + tracing::error!("Failed to borrow connection: {}", e); + return Err(e); + } + }; + connection + .execute_command(Some(self.graph_name.as_str()), command, subcommand, params) + .await }
Line range hint
347-485: Add test coverage for connection failure scenariosThe test suite is comprehensive for successful operations, but consider adding tests for connection failure scenarios to ensure proper handling of connection leaks mentioned in the PR objectives.
Example test case:
#[tokio::test(flavor = "multi_thread")] async fn test_connection_failure_handling() { let mut graph = create_async_test_client().await.select_graph("test_connection_failure"); // Simulate connection failure (implementation depends on your test utilities) // For example, by stopping the database or using a mock client let result = graph.query("RETURN 1").execute().await; assert!(result.is_err()); // Verify that resources are properly cleaned up }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/graph/asynchronous.rs(5 hunks)
🔇 Additional comments (1)
src/graph/asynchronous.rs (1)
119-119: LGTM! Improved lifetime management in method signatures
The explicit lifetime parameters in the QueryBuilder return types enhance type safety and help prevent potential memory issues by ensuring proper lifetime tracking of borrowed references.
Also applies to: 134-134, 165-165, 181-181, 197-197
Summary by CodeRabbit
New Features
0.1.8to0.1.9, reflecting changes in installation instructions and feature specifications.Bug Fixes
Documentation