-
Notifications
You must be signed in to change notification settings - Fork 569
HttpServlet Streamable HTTP server implementation #290
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
HttpServlet Streamable HTTP server implementation #290
Conversation
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
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.
Hey 👋 Thanks for a comprehensive PR! I did my first round focusing on the main themes. Happy to offer guidance to cover the essential aspects (simple/stateful servers, multiple streams per session, lifecycle) if you'd like to push this forward.
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
Thank you very much for all of the input @chemicL! I will begin making changes accordingly this afternoon. |
Today I'm targeting origin header validation and moving the dedicated GET stream to the StreamableHttpSession class, then adding an integ test for GET on /mcp to start the listening stream. |
Hi @ZachGerman I tried using your StreamableHttpServerTransportProvider.java file + java MCP SDK 0.10.0. My backend server is Jetty 12. The request from MCP Interceptor hangs in the below call. return streamSession.handle(message).then(Mono.just(responseType)).onErrorReturn(ResponseType.IMMEDIATE); This is the actuall line which gets blocked Can you please check this? |
@ZachGerman what do you mean by |
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.
Added a few more comments.
if (sessionId == null) { | ||
response.setContentType(APPLICATION_JSON); | ||
response.setStatus(HttpServletResponse.SC_BAD_REQUEST); | ||
response.getWriter().write(createErrorJson("Session ID missing in request header")); |
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.
modelcontextprotocol/modelcontextprotocol#282 please check this issue. I'm leaning towards separating the JSON-RPC lifecycle from the lower level transport lifecycle. In that world, GET and POST and session concepts are not at the JSON-RPC layer, so the lifecycle does not apply. In fact, the previous SSE transport did begin with a HTTP GET request before the initialization request could have been sent. For the stateless servers it should definitely be improved in the spec to mention that initialization is not required before other requests.
[EDIT]
For the record, the above was wishful thinking. I agree we are only able to generate the session ID upon POST of the initialize request. The spec is in fact melding the json-rpc layer with http, so the session ID gets generated only when processing a initialization request:
A server using the Streamable HTTP transport MAY assign a session ID at initialization time, by including it in an
Mcp-Session-Id
header on the HTTP response containing theInitializeResult
// Subscribe to the SSE stream and write events to the response | ||
sseStream.getEventFlux().doOnNext(event -> { | ||
try { | ||
if (event.id() != null) { |
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.
Right, but do we emit events with no ID ? Reading the code I don't see a situation like this. My understanding of the mentioned spec is that if the ID is missing then the client would not use this id for Last-Event-ID tracking, but we are on the server side and we always generate an ID.
...ain/java/io/modelcontextprotocol/server/transport/StreamableHttpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
@sivankri: Changing the related logic after my meeting with Dariusz this morning, so (hopefully) your issue goes away with the new response type mapping. |
merge commit 6c4830b is bad |
Oops! Thanks for the heads up! Fixed! |
6c4830b
to
7817da6
Compare
@sivankri the new logic added |
...ain/java/io/modelcontextprotocol/server/transport/HttpServletSseServerTransportProvider.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncStreamableHttpServer.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncStreamableHttpServer.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncServer.java
Outdated
Show resolved
Hide resolved
@ZachGerman I tried running my mcp remote server against the mcp sdk generated from your branch. When i used the MCP inspector tool by setting query parameter transport=streamable-http, i could not get the connection established. Can you please help me with this? |
@sivankri I'm unfamiliar with the MCP inspector tool, but I imagine it's related to the fact that this JDK has |
mcp/src/main/java/io/modelcontextprotocol/spec/McpServerSession.java
Outdated
Show resolved
Hide resolved
244208d
to
25ff3dc
Compare
Once I realized that I needed to identify the transports in the exchange's session from the tool handlers, I realized I need to either pass the entire JSONRPCRequest to the handler (instead of the request.params()), or add a third request.id() parameter to tool handlers. Either way, it brought more attention to the fact that we were using Tomorrow I will be making the JSONRPCRequest Edit: Ended up going with a per-request exchange instance instead of passing transport ID to tool call handlers. This allows tool implementers to not worry about transport routing with their exchange method calls. |
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
…sync server tests Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
29a39f7
to
c23c5df
Compare
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
Signed-off-by: Dariusz Jędrzejczyk <[email protected]>
…protocol#425) - Add WebMvcStreamableServerTransportProvider with SSE support for streamable sessions - Support GET, POST, DELETE endpoints for MCP protocol operations - Implement thread-safe SSE operations using ReentrantLock in WebMvcSseServerTransportProvider - Add test infrastructure with AbstractMcpClientServerIntegrationTests - Refactor WebMvcStreamableIntegrationTests to use parameterized tests - Support testing with both HttpClient and WebFlux transports - Add streamable transport tests for both async and sync server modes - Refactor existing WebMVC SSE integration tests to use shared test base - Add error handling improvements in McpStreamableServerSession - Update dependencies: add json-unit-assertj for enhanced JSON testing - Reorganize POM dependencies and add mcp-spring-webflux test dependency This change enables Spring WebMVC applications to use the streamable MCP transport protocol with proper thread safety guarantees and comprehensive test coverage. Related to modelcontextprotocol#72 Signed-off-by: Christian Tzolov <[email protected]>
- Wrap consumer, tool, resource, prompt, and completion handler calls with Mono.defer() - Ensures proper lazy evaluation and error handling in reactive streams Signed-off-by: Christian Tzolov <[email protected]>
- Add HttpServletStreamableServerTransportProvider for MCP streamable transport - Provides HttpServlet equivalent of WebMvcStreamableServerTransportProvider without Spring dependencies - Support for GET (SSE connections/replay), POST (JSON-RPC messages), and DELETE (session cleanup) operations - Include comprehensive integration test suite with sampling, elicitation, roots, and tools testing - Add async/sync server test implementations for HttpServlet transport Signed-off-by: Christian Tzolov <[email protected]>
Signed-off-by: Christian Tzolov <[email protected]>
5c09fa5
to
b4b33d6
Compare
Thanks for great work. Is there a coordination with Spring AI MCP? Would love to take it for a test drive |
Not planned:
Motivation and Context
This has been created by collaboration between @ZachGerman, @chemicL, & @tzolov to bring the much desired feature of Streamable HTTP transport to the Java SDK with three transport providers to choose from.
How Has This Been Tested?
Unit testing along with a suite of abstract integration tests.
Breaking Changes
N/A
Types of changes
Checklist