-
Notifications
You must be signed in to change notification settings - Fork 365
Fix transport session id mismatch with sessionId #176
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: JermaineHua <[email protected]>
good job,I also found this problem, now get the correct seesion, not inside the mcp session, but inside the transport~ |
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.
Thank you for the proposal! I agree we have a mismatch and we need a way to provide the session id to both the transport and the session objects. I would propose that the McpServerSession.Factory
expose a new method, generateId
which can be fed into both of these objects. Would you like to apply such a change?
Yes, I'll make some revisions to this part. |
Signed-off-by: JermaineHua <[email protected]>
# Conflicts: # mcp/src/main/java/io/modelcontextprotocol/server/McpAsyncServer.java
Signed-off-by: JermaineHua <[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.
Thanks for the work on this. It is almost ready, a few nitpicks about imports.
As a next step we should unify the way the server transports log and include the session id everywhere. But please don't create a PR for it just yet.
@tzolov in 0.11.0 release notes we need to highlight that McpServerSession.Factory went through a breaking change impacting custom transport implementors, not end users per se.
...rc/main/java/io/modelcontextprotocol/server/transport/WebFluxSseServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/modelcontextprotocol/server/transport/WebMvcSseServerTransportProvider.java
Show resolved
Hide resolved
...ain/java/io/modelcontextprotocol/server/transport/HttpServletSseServerTransportProvider.java
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/server/transport/StdioServerTransportProvider.java
Outdated
Show resolved
Hide resolved
mcp/src/test/java/io/modelcontextprotocol/MockMcpServerTransportProvider.java
Outdated
Show resolved
Hide resolved
...rc/test/java/io/modelcontextprotocol/server/transport/StdioServerTransportProviderTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: JermaineHua <[email protected]>
Motivation and Context
Fix #168
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context