Add Streamable Http Transport #87
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces streamable HTTP transport to handle JSON-RPC over SSE, enabling stateful streaming of responses. Key changes include:
- Changing JSONRPCResponse.id to be nullable.
- Adding a new transport implementation, StreamableHttpServerTransport, to manage SSE connections and handle HTTP POST, GET, and DELETE requests.
- Incorporating error handling for invalid headers, media types, and session validation.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt | Updated JSONRPCResponse to allow a nullable id, supporting error responses. |
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt | Added implementation for streamable HTTP server transport with methods for handling POST, GET, and DELETE requests over SSE. |
Files not reviewed (1)
- api/kotlin-sdk.api: Language not supported
Comments suppressed due to low confidence (1)
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt:74
- [nitpick] Consider using the standard 'Content-Type' header key instead of ContentType.toString() to ensure the header name is correctly set.
correspondingCall.response.headers.append(ContentType.toString(), ContentType.Application.Json.toString())
| // TODO Check if we need to clear the callMapping or if call timeout after awhile | ||
| _onClose.invoke() |
There was a problem hiding this comment.
Review whether callMapping should be cleared in the close() method to free resources and avoid potential memory leaks.
| // TODO Check if we need to clear the callMapping or if call timeout after awhile | |
| _onClose.invoke() | |
| callMapping.clear() // Clear callMapping to free resources and avoid potential memory leaks |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Hey @SeanChinJunKai, thanks for the PR. Please check the comments.
It also would be great to have some tests
...commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt
Show resolved
Hide resolved
...commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt
Outdated
Show resolved
Hide resolved
| try { | ||
| val acceptHeader = call.request.headers["Accept"]?.split(",") ?: listOf() | ||
|
|
||
| if (!acceptHeader.contains("text/event-stream") || !acceptHeader.contains("application/json")) { |
There was a problem hiding this comment.
please use match instead of contains
There was a problem hiding this comment.
@e5l do you mean regex match with call.request.headers["Accept"]? Why do you prefer match over contains?
...commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt
Outdated
Show resolved
Hide resolved
...commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt
Show resolved
Hide resolved
| } | ||
|
|
||
| if (sessionId != null) { | ||
| call.response.header("Mcp-Session-Id", sessionId!!) |
There was a problem hiding this comment.
Constants should be extracted to the top-level scope with a proper visibility
There was a problem hiding this comment.
@e5l I have extracted it into a Constants file in shared module since we might need it in client module
|
Hi @e5l, thanks for the comments. Ill take a look. I have some questions regarding client transport implementation. In particular, when client makes post request to server server can open a sse session but how does client connect to this sse session? |
|
Hi @e5l, could i get some help on the above? |
|
Sure, let me check |
Closes #79
Pending:
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context