Fix: close consensus node gRPC channels in Client.close() - #2203
Conversation
Up to standards ✅🟢 Issues
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88977a9a-5f67-46b3-bc40-bb4c4e7bc5c2
📒 Files selected for processing (1)
src/hiero_sdk_python/client/client.py
59d1552 to
e307b6c
Compare
e307b6c to
1dffb12
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/hiero_sdk_python/client/client.py (1)
210-213:⚠️ Potential issue | 🔴 CriticalCritical:
node.close()does not exist —Client.close()will still raiseAttributeError.Per
src/hiero_sdk_python/node.py(lines 80–114),_Nodeonly defines a private_close()method; there is no publicclose(). AnyClientwith at least one node will raiseAttributeError: '_Node' object has no attribute 'close'whenclose()(or__exit__) runs — the opposite of this PR's stated goal.This was previously flagged on a prior commit; the
.values()issue was fixed, but the call was changed tonode.close()instead ofnode._close(), which is still broken. A unit test that actually executesClient.close()on a real client (or exits awith Client(...)block) and asserts no exception is raised — and that eachnode._channelisNoneafterward — would catch this and should be added alongside the fix.🐛 Proposed fix
- # Fix: Close all consensus node channels - if self.network and self.network.nodes: - for node in self.network.nodes: - node.close() + # Close all consensus node channels + if self.network is not None and self.network.nodes: + for node in self.network.nodes: + node._close() # pylint: disable=protected-access
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 528d7635-1b58-4565-9a9c-5918f585cceb
📒 Files selected for processing (1)
src/hiero_sdk_python/client/client.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/hiero_sdk_python/client/client.py (1)
210-213:⚠️ Potential issue | 🔴 Critical
Client.close()is syntactically broken here.Line 212 introduces a
forloop with no body, so this file will not parse. If this block was truncated accidentally, the body should delegate tosrc/hiero_sdk_python/node.py:_Node._close()rather than reimplementing channel teardown here.🐛 Proposed fix
- # Fix: Close all consensus node channels - if self.network and self.network.nodes: - for node in self.network.nodes: + # Close all consensus node channels + if self.network and self.network.nodes: + for node in self.network.nodes: + node._close() # pylint: disable=protected-accessYou can verify the current file still lacks a loop body with:
#!/bin/bash sed -n '208,215p' src/hiero_sdk_python/client/client.py
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ab94894-591d-4d7c-9227-98cb86ed905d
📒 Files selected for processing (1)
src/hiero_sdk_python/client/client.py
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12bca3ee-c772-45c4-9590-e155e9e773be
📒 Files selected for processing (1)
src/hiero_sdk_python/client/client.py
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #2203 +/- ##
=======================================
Coverage 93.73% 93.73%
=======================================
Files 145 145
Lines 9477 9480 +3
=======================================
+ Hits 8883 8886 +3
Misses 594 594 🚀 New features to boost your workflow:
|
|
@mohityadav8, thanks for the PR please apply the pre-commit you can follow the docs here https://github.com/hiero-ledger/hiero-sdk-python/blob/main/docs/sdk_developers/setup.md |
tech0priyanshu
left a comment
There was a problem hiding this comment.
LGTM Please apply the pre-commit. Thanks for your contribution
a7c15aa to
8b87e10
Compare
|
@mohityadav8, can you please update the branch again |
|
Done @manishdait |
|
cc @exploreriii |
Up to standards ✅🟢 Issues
|
|
cc @hiero-ledger/hiero-sdk-python-maintainers |
done |
|
@mohityadav8, your first two commit is missing the |
78dce33 to
a4f3b77
Compare
Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
a4f3b77 to
0982bee
Compare
@manishdait ready for review |
|
Update branch please |
…er#2203) Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
Description:
Fix resource leak in Client.close() by closing all consensus node gRPC channels.
Previously,
Client.close()only closed the mirror node channel, leaving allconsensus node channels (
_Node._channel) open. This caused lingering gRPCconnections even when using the client as a context manager.
Changes
_Node._channelinstances inclient.network.nodesClient.close()Fixes #2202
Notes for reviewer:
client.close()with Client(...))Checklist