-
Notifications
You must be signed in to change notification settings - Fork 122
refactor SseClientTransport
#142
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the SSE client transport implementation—improving cancellation handling and cleaning up connection logic—and updates integration and unit tests to use dynamic ports and ensure resources are closed.
- Tests now capture and close the client, use port 0 with
actualPort()
, and wrap cleanup intry/finally
. SseClientTransport
was reorganized: message collection extracted, addedhandleEndpoint
/handleMessage
, and consolidated resource cleanup.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
SseIntegrationTest.kt | Introduced client variable, catch/fail wrapper, and ensured client/server closures |
SseTransportTest.kt | Switched to dynamic ports via actualPort() , added try/finally around server stop |
SSEClientTransport.kt | Rewrote start , extracted collectMessages , handleEndpoint , handleMessage , and unified cleanup in closeResources |
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SSEClientTransport.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SSEClientTransport.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, please check if we need to handle more general exception type
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SSEClientTransport.kt
Outdated
Show resolved
Hide resolved
…ndling and try/catch `cancel` in closeResources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
How Has This Been Tested?
locally
Breaking Changes
no
Types of changes
Checklist