add close and aclose on FalkorDB connections #127
Conversation
|
Warning Rate limit exceeded@gkorland has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 36 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)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds sync and async context-manager lifecycle to FalkorDB: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Application
participant DB as FalkorDB (sync)
participant RC as Redis Client
User->>App: with FalkorDB(...) as db:
App->>DB: __enter__()
DB-->>App: self
App->>RC: (operations)
App->>DB: __exit__(exc_type, exc_val, exc_tb)
DB->>DB: close()
alt close succeeds
DB->>RC: close()
RC-->>DB: ok
else RedisError
note right of DB: catch RedisError (best-effort)
end
DB-->>App: exit complete
sequenceDiagram
autonumber
actor User
participant App as Application
participant ADB as FalkorDB (async)
participant ARC as Redis Client (async)
User->>App: async with FalkorDB(...) as adb:
App->>ADB: __aenter__()
ADB-->>App: self
App->>ARC: (async operations)
App->>ADB: __aexit__(exc_type, exc_val, exc_tb)
ADB->>ADB: aclose()
alt aclose succeeds
ADB->>ARC: aclose()
ARC-->>ADB: ok
else RedisError
note right of ADB: catch RedisError (best-effort)
end
ADB-->>App: exit complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds connection management functionality to both sync and async FalkorDB classes by implementing close methods and context manager support. This allows proper cleanup of Redis connections when the client is no longer needed.
- Adds
close()method and context manager support (__enter__/__exit__) to the sync FalkorDB class - Adds
aclose()method and async context manager support (__aenter__/__aexit__) to the async FalkorDB class - Implements best-effort error handling that suppresses Redis errors during connection cleanup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| falkordb/falkordb.py | Adds connection cleanup methods and context manager support for sync operations |
| falkordb/asyncio/falkordb.py | Adds async connection cleanup methods and async context manager support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
+ Coverage 91.69% 91.70% +0.01%
==========================================
Files 36 37 +1
Lines 2685 2737 +52
==========================================
+ Hits 2462 2510 +48
- Misses 223 227 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gkorland
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_close.py (1)
37-53: Add fallback test for async close.The test correctly validates that
aclose()is awaited in both explicit calls and async context managers. However, similar to the sync version, consider adding a test for the fallback scenario when the connection lacks anaclose()method.Add a test similar to
test_sync_close_fallback_disconnectfor the async case:@pytest.mark.asyncio async def test_async_aclose_fallback_disconnect(): db = object.__new__(AsyncFalkorDB) # connection has no aclose(), only a connection_pool.disconnect() mock_conn = SimpleNamespace() mock_conn.connection_pool = SimpleNamespace(disconnect=AsyncMock()) db.connection = mock_conn await db.aclose() mock_conn.connection_pool.disconnect.assert_awaited_once()Note: This will also fail with the current implementation and requires the same fallback logic fix in
falkordb/asyncio/falkordb.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_close.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_close.py (2)
falkordb/falkordb.py (1)
close(242-251)falkordb/asyncio/falkordb.py (1)
aclose(202-211)
🪛 GitHub Actions: Test
tests/test_close.py
[error] 32-32: Test failure in test_sync_close_fallback_disconnect: AttributeError raised when closing DB connection; indicates fallback disconnect path may be incorrect.
🔇 Additional comments (2)
tests/test_close.py (2)
1-7: LGTM!Imports are appropriate for testing both synchronous and asynchronous close behavior with mocked connections.
10-21: LGTM!The test correctly verifies that the context manager calls
close()on the underlying connection. The use ofobject.__new__to bypass initialization is appropriate for unit testing.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| async def __aenter__(self) -> "FalkorDB": | ||
| """Return self to support async with-statement usage.""" | ||
|
|
||
| return self | ||
|
|
||
| async def __aexit__(self, exc_type, exc_val, exc_tb) -> None: | ||
| """Close the connection when exiting an async with-statement.""" | ||
|
|
||
| await self.aclose() |
There was a problem hiding this comment.
[nitpick] Remove the extra blank line between the aclose method and aenter method to maintain consistent spacing with the sync version.
Add support for
withclause by adding close and aclosefix #128
Summary by CodeRabbit