-
Notifications
You must be signed in to change notification settings - Fork 722
fix(client): resource leak in SSEClient.SendRequest()
#206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Changes
📜 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 (
|
* refact SSEClient's SendRequest for resource leak * correct comments
* refact SSEClient's SendRequest for resource leak * correct comments
Currently, func
SendRequest()
in SSE client has potential risk of not removing theresponseChan
for request forever because funcSendRequest
createresponseChan
at the very beginning, and the following situations will cause resource leak in our program:http.Request
object when we call funchttp.NewRequestWithContext
http.Request
fails to send request to MCPserver when we callc.httpClient.Do(req)
http.StatusOK/http.StatusAccepted
How about using
defer
to removeresponseChan
?Note func
SSE.handleSSEEvent
will finally remove the channel for this request after we receive data from MCPServer and write back data toresponseChan
. Though deleting a key from a map for twice is ok, it is not efficient in concurrent scenario because we have tolock
the exclusive mutex in SSEClient every time when we want to modifySSEClient.responses
anddefer
is redundant but also tries tolock
when every thing goes well( actually, this is most of the cases will happen, the boundary cases mentioned above is rather low possible)Summary by CodeRabbit