fix: reduce unclosed sessions in lazily loaded xarrays #66
Conversation
WalkthroughA new architecture document for the py-hamt library was added, providing a comprehensive overview of its design and implementation. Enhancements were made to the KuboCAS session management in Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant MainLoop as Main Event Loop
participant ThreadLoop as Thread Event Loop
participant KuboCAS as KuboCAS
participant Session as aiohttp.ClientSession
Test->>MainLoop: Create KuboCAS instance
MainLoop->>KuboCAS: Request session (current loop)
KuboCAS->>Session: Create session for main loop
Test->>ThreadLoop: In thread, request session
ThreadLoop->>KuboCAS: Request session (thread loop)
KuboCAS->>Session: Create session for thread loop
Test->>KuboCAS: Call aclose()
KuboCAS->>Session: Close sessions for all owned loops
KuboCAS->>Test: Cleanup complete
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (5)
tests/test_async.py (1)
16-16: Consider testing through public interfaces instead of private methods.While testing private methods (
_loop_session()) and attributes (_session_per_loop) can be necessary for verifying internal behavior, it creates brittle tests that break when implementation details change. Consider whether these behaviors could be tested through public APIs.Also applies to: 27-27, 40-40, 54-55, 59-60
py_hamt/store.py (1)
256-260: Consider adding tests for edge cases to improve coverage.The pipeline indicates missing coverage for:
- Lines 256-260: The case when no event loop is running
- Lines 281-283: Exception handling during session closure
While these are edge cases, adding tests would ensure robustness.
Would you like me to help generate test cases for these uncovered scenarios?
Also applies to: 281-283
🧰 Tools
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag fix/async-session-runtime-error
[error] 256-260: Coverage failure: lines 256-260 are missing coverage.
Architecture.md (3)
28-28: Fix grammar: "mode uses" instead of "modes uses".-- Supports both read-only and read-write modes -- Uses asyncio locks for thread safety in write mode +- Supports both read-only and read-write modes +- Uses asyncio locks for thread safety in write mode🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: Did you mean “modes use” or “mode uses”?
Context: ... Supports both read-only and read-write modes - Uses asyncio locks for thread safety in writ...(NNS_USES)
120-120: Add missing article "a".-- **Solution**: Use single event loop for all write operations +- **Solution**: Use a single event loop for all write operations🧰 Tools
🪛 LanguageTool
[uncategorized] ~120-~120: You might be missing the article “a” here.
Context: ... not threading.Lock - Solution: Use single event loop for all write operations ##...(AI_EN_LECTOR_MISSING_DETERMINER_A)
135-138: Fix multiple grammar issues in pruning section.-- **Problem**: Deletions can leave empty nodes in tree -- **Gotcha**: Empty nodes are automatically pruned except root -- **Why**: Content addressing means identical empty nodes have same hash -- **Solution**: Pruning is automatic, but be aware root can become empty +- **Problem**: Deletions can leave empty nodes in the tree +- **Gotcha**: Empty nodes are automatically pruned except the root +- **Why**: Content addressing means identical empty nodes have the same hash +- **Solution**: Pruning is automatic, but be aware the root can become empty🧰 Tools
🪛 LanguageTool
[uncategorized] ~135-~135: You might be missing the article “the” here.
Context: ...m**: Deletions can leave empty nodes in tree - Gotcha: Empty nodes are automatic...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~136-~136: You might be missing the article “the” here.
Context: ...y nodes are automatically pruned except root - Why: Content addressing means ide...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~137-~137: Possible agreement error -- use the past participle here.
Context: ...g means identical empty nodes have same hash - Solution: Pruning is automatic, b...(HAVE_PART_AGREEMENT)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...n**: Pruning is automatic, but be aware root can become empty ### 6. Serialization ...(AI_EN_LECTOR_MISSING_PREPOSITION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Architecture.md(1 hunks)py_hamt/store.py(2 hunks)tests/test_async.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag fix/async-session-runtime-error
py_hamt/store.py
[error] 256-260: Coverage failure: lines 256-260 are missing coverage.
[error] 281-283: Coverage failure: lines 281-283 are missing coverage.
🪛 LanguageTool
Architecture.md
[grammar] ~28-~28: Did you mean “modes use” or “mode uses”?
Context: ... Supports both read-only and read-write modes - Uses asyncio locks for thread safety in writ...
(NNS_USES)
[uncategorized] ~120-~120: You might be missing the article “a” here.
Context: ... not threading.Lock - Solution: Use single event loop for all write operations ##...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[style] ~130-~130: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ance vs memory trade-offs - Gotcha: Very small bucket sizes (1) force deep trees, larg...
(EN_WEAK_ADJECTIVE)
[uncategorized] ~135-~135: You might be missing the article “the” here.
Context: ...m**: Deletions can leave empty nodes in tree - Gotcha: Empty nodes are automatic...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~136-~136: You might be missing the article “the” here.
Context: ...y nodes are automatically pruned except root - Why: Content addressing means ide...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~137-~137: Possible agreement error -- use the past participle here.
Context: ...g means identical empty nodes have same hash - Solution: Pruning is automatic, b...
(HAVE_PART_AGREEMENT)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...n**: Pruning is automatic, but be aware root can become empty ### 6. Serialization ...
(AI_EN_LECTOR_MISSING_PREPOSITION)
[uncategorized] ~173-~173: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e**: O(log₂₅₆ n) average case, O(depth) worst case - Iteration: O(n) for all keys/valu...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~175-~175: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...keys/values - Tree depth: Typically 1-4 levels for most datasets ### Space Com...
(HYPHEN_TO_EN)
🪛 markdownlint-cli2 (0.17.2)
Architecture.md
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (3)
py_hamt/store.py (2)
214-236: LGTM! Good improvements to session cleanup configuration.The addition of
force_close=Trueandenable_cleanup_closed=Trueto the TCPConnector should help prevent unclosed session warnings by ensuring connections are properly cleaned up.
240-290: Excellent implementation of multi-loop session cleanup!The rewritten
aclosemethod properly handles the complex scenario of sessions existing across multiple event loops. The separation of sessions by loop ownership and the careful handling of cross-loop limitations is well thought out.🧰 Tools
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag fix/async-session-runtime-error
[error] 256-260: Coverage failure: lines 256-260 are missing coverage.
[error] 281-283: Coverage failure: lines 281-283 are missing coverage.
Architecture.md (1)
1-214: Excellent architecture documentation!This comprehensive document provides valuable insights into the py-hamt library's design, implementation details, and potential gotchas. The clear structure and detailed explanations will be very helpful for developers working with this library.
🧰 Tools
🪛 LanguageTool
[grammar] ~28-~28: Did you mean “modes use” or “mode uses”?
Context: ... Supports both read-only and read-write modes - Uses asyncio locks for thread safety in writ...(NNS_USES)
[uncategorized] ~120-~120: You might be missing the article “a” here.
Context: ... not threading.Lock - Solution: Use single event loop for all write operations ##...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[style] ~130-~130: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ance vs memory trade-offs - Gotcha: Very small bucket sizes (1) force deep trees, larg...(EN_WEAK_ADJECTIVE)
[uncategorized] ~135-~135: You might be missing the article “the” here.
Context: ...m**: Deletions can leave empty nodes in tree - Gotcha: Empty nodes are automatic...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~136-~136: You might be missing the article “the” here.
Context: ...y nodes are automatically pruned except root - Why: Content addressing means ide...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~137-~137: Possible agreement error -- use the past participle here.
Context: ...g means identical empty nodes have same hash - Solution: Pruning is automatic, b...(HAVE_PART_AGREEMENT)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...n**: Pruning is automatic, but be aware root can become empty ### 6. Serialization ...(AI_EN_LECTOR_MISSING_PREPOSITION)
[uncategorized] ~173-~173: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...e**: O(log₂₅₆ n) average case, O(depth) worst case - Iteration: O(n) for all keys/valu...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~175-~175: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...keys/values - Tree depth: Typically 1-4 levels for most datasets ### Space Com...(HYPHEN_TO_EN)
🪛 markdownlint-cli2 (0.17.2)
18-18: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 597 603 +6
=========================================
+ Hits 597 603 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/test_async.py (1)
213-251: 🛠️ Refactor suggestionProblematic use of fake loop object for testing.
Using
object()as a fake event loop in line 234 may not be the best approach as it doesn't behave like a real event loop and could lead to unpredictable behavior.Consider creating a more realistic test scenario or using a proper mock event loop:
- kubo._session_per_loop[object()] = sess1 # Fake a second loop + # Create a second real loop for more realistic testing + import weakref + fake_loop = asyncio.new_event_loop() + kubo._session_per_loop[fake_loop] = sess1 + fake_loop.close() # Close immediately to simulate closed loop
🧹 Nitpick comments (2)
tests/test_async.py (2)
135-136: Remove misplaced comment.This comment appears to be a leftover from development and doesn't belong in this file.
-# tests/test_kubocas_session.py (add this test to the existing file) -
174-211: Consider consolidating similar error handling tests.This test is very similar to the previous one. Consider refactoring to reduce duplication or clearly document the difference in scenarios being tested.
The main difference appears to be testing multiple calls to close the same session, but this could be combined with the previous test or made more distinct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d596e8f and 400297e3736b7376ad8666a7ff797d2b9c6b4fdf.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/test_async.py(1 hunks)
🔇 Additional comments (6)
tests/test_async.py (6)
1-8: LGTM: Clean import structure and appropriate test setup.The imports are well-organized and include all necessary modules for the async testing scenarios.
10-77: Comprehensive cross-loop testing with robust cleanup logic.This test effectively validates the core issue addressed in the PR - handling sessions across multiple event loops without raising "attached to a different loop" errors. The cleanup logic is thorough and prevents resource leaks.
78-96: Good simulation of the real-world usage scenario.This test validates the exact context manager pattern that users would typically employ, ensuring the fix works in practical scenarios.
98-117: Effective use of mocking for edge case testing.The test properly simulates the no-running-loop scenario using
unittest.mock.patch, which is appropriate for this edge case.
119-133: Simple and effective test for already-closed sessions.This test ensures graceful handling of sessions that are already closed, which is an important edge case.
138-172: Excellent error handling test with proper exception suppression.This test correctly validates that
aclose()gracefully handles session close failures without propagating exceptions, which is crucial for robust cleanup.
Faolain
force-pushed
the
fix/async-session-runtime-error
branch
from
400297e to
0128b0f
Compare
…t-session-issue Fix unclosed session warning
|
Looks like codex solved it (?) |
Problem
Previously a run of the following standard code in a python script like
caused the following error & warnings:
Cause
From Claude:
Solution
Summary by CodeRabbit
Documentation
Bug Fixes
Tests