fix(client): risk of resource leak and closing closed channel #208
Conversation
WalkthroughThis change updates the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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:
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 (1)
client/transport/stdio.go (1)
103-109: Good improvement to prevent panic on repeated Close() calls.The added check ensures that attempting to close an already closed channel won't cause a panic. This makes the method idempotent and safer to call multiple times.
Note: There's a small typo in the comment - "in-clight" should be "in-flight".
- // cancel all in-clight request + // cancel all in-flight requests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/transport/stdio.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/transport/stdio.go (2)
client/transport/interface.go (1)
JSONRPCResponse(36-45)testdata/mockstdio_server.go (1)
JSONRPCResponse(18-26)
🔇 Additional comments (3)
client/transport/stdio.go (3)
192-209: Excellent refactoring to prevent resource leaks.This refactoring improves the
SendRequestmethod in several ways:
- The request is now marshaled before registering the response channel
- The cleanup logic is centralized in a helper function
- Resource cleanup is properly handled in all error and cancellation scenarios
This change prevents potential resource leaks where response channels might remain in the map if:
- Request marshaling fails
- Writing to stdin fails
- Context is cancelled before receiving a response
212-213: Good error handling with proper cleanup.The addition of
deleteResponseChan()call ensures the response channel is properly removed from the map when writing the request fails, preventing resource leaks.
217-219: Good context cancellation handling with proper cleanup.The addition of
deleteResponseChan()call ensures the response channel is properly removed from the map when the context is cancelled, preventing resource leaks.
…abs#208) * fix resource leak in Client StdIo * fix potential risk of closing closed channel
Changes:
StdIo.SendRequest()to remove the potential risk of resource leak whose situation and boundary cases are similar to and have been discussed in pull request fix(client): resource leak inSSEClient.SendRequest()#206panic: close of closed channelwhen we call funcStdIo.Close()for more than one times. That is, we had better check whether the channelc.donein ClientStdIohave been closed or not before we executeclose(c.done)Summary by CodeRabbit
Bug Fixes
Chores