fix: close child httpx clients in AsyncStream.aclose()#230
Conversation
AsyncStream.aclose() only closed the main httpx client but not the cached video/chat/moderation sub-clients, each of which has its own httpx connection pool. After profiling with 50 session create+close cycles, 87 orphaned TCP connections remained. Override aclose() to close all cached child clients before closing the main client.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 31 seconds. ⌛ 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. 📝 WalkthroughWalkthroughAdds an async Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AsyncStream
participant Child (video/chat/moderation)
participant BaseClient
Caller->>AsyncStream: await aclose()
AsyncStream->>AsyncStream: for each child in ['video','chat','moderation']
alt child exists and has aclose
AsyncStream->>Child: await child.aclose()
Child-->>AsyncStream: closed
else child missing or no aclose
AsyncStream-->>AsyncStream: skip
end
AsyncStream->>BaseClient: await super().aclose()
BaseClient-->>AsyncStream: base closed
AsyncStream-->>Caller: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Tests that main, video, and chat httpx clients are closed after aclose(), and that aclose() works when child clients were never accessed (cached_property not triggered).
![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
🧹 Nitpick comments (1)
tests/test_async_stream_close.py (1)
6-35: Use a fixture to injectAsyncStreamacross these tests.All tests construct clients inline; please move client creation to a fixture and inject it into test methods to match the repository’s test structure requirements and centralize cleanup behavior.
As per coding guidelines, "Use fixtures to inject objects in tests; test client fixtures can use the Stream API client."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_async_stream_close.py` around lines 6 - 35, Replace inline AsyncStream construction with a pytest fixture that returns an AsyncStream and ensures proper async cleanup; create a fixture (e.g., async_stream) that instantiates AsyncStream(api_key="fake", api_secret="fake") and yields it, then awaits fixture.aclose() in the teardown, and update the tests (test_aclose_closes_main_client, test_aclose_closes_video_client, test_aclose_closes_chat_client, test_aclose_without_child_clients) to accept the fixture as an argument and remove their local client creation so they use the injected async_stream; keep uses that trigger cached properties (accessing .video and .chat) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@getstream/stream.py`:
- Around line 210-216: The aclose method must always close the main client even
if a child close fails and should call the child's aclose() to preserve
encapsulation: iterate the attributes ("video","chat","moderation") and for each
existing child call await child.aclose() inside a try/except that logs or
suppresses the error so one failure doesn't stop the loop, then after the loop
ensure await super().aclose() is always executed (i.e., place it after the loop,
not inside the try that could be aborted) so the main HTTPX client is closed
regardless of child errors.
In `@tests/test_async_stream_close.py`:
- Around line 15-35: Add a test mirroring the existing video/chat checks to
assert AsyncStream.aclose() closes the moderation child: instantiate
AsyncStream, access the moderation cached property (e.g., _ =
client.moderation), assert client.moderation.client.is_closed is False, await
client.aclose(), then assert client.moderation.client.is_closed is True; this
ensures AsyncStream.aclose() closes the moderation client like video and chat.
---
Nitpick comments:
In `@tests/test_async_stream_close.py`:
- Around line 6-35: Replace inline AsyncStream construction with a pytest
fixture that returns an AsyncStream and ensures proper async cleanup; create a
fixture (e.g., async_stream) that instantiates AsyncStream(api_key="fake",
api_secret="fake") and yields it, then awaits fixture.aclose() in the teardown,
and update the tests (test_aclose_closes_main_client,
test_aclose_closes_video_client, test_aclose_closes_chat_client,
test_aclose_without_child_clients) to accept the fixture as an argument and
remove their local client creation so they use the injected async_stream; keep
uses that trigger cached properties (accessing .video and .chat) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f69dcb25-6052-4a2b-8f28-16c4bdd0d2d2
📒 Files selected for processing (2)
getstream/stream.pytests/test_async_stream_close.py
Replaces try/finally with AsyncExitStack to ensure all child clients are closed even if one fails. Also checks __dict__ instead of hasattr to only close cached_property clients that were actually accessed.
2 participants
Why
AsyncStream.aclose()only closes the main httpx client, but the cachedvideo,chat, andmoderationsub-clients each create their own httpx connection pool via@cached_property. When the parent is closed, these child pools remain open, leaking TCP connections.Discovered during memory leak investigation in vision-agents: after 50 session create+close cycles, 87 orphaned TCP connections remained. After this fix + companion Vision-Agents PR, leaked connections dropped to 2 (-98%) and memory returns to baseline.
Changes
aclose()inAsyncStreamto close all cached child httpx clients before closing the main clientis_closed == Trueafteraclose()Companion PR
Summary by CodeRabbit
Bug Fixes
Tests