-
Notifications
You must be signed in to change notification settings - Fork 627
fix:avoid streamable listening sse duplicate creation #524
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?
fix:avoid streamable listening sse duplicate creation #524
Conversation
Are you working for TaoBao Group? |
String lastId = request.headers().asHttpHeaders().getFirst(HttpHeaders.LAST_EVENT_ID); | ||
return ServerResponse.ok() | ||
.contentType(MediaType.TEXT_EVENT_STREAM) | ||
.body(session.replay(lastId), ServerSentEvent.class); | ||
} | ||
if (listenedStream instanceof McpStreamableServerSessionStream) { | ||
logger.debug("Listening stream for session: {} exists.", sessionId); |
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.
if(logger.isDebugEnabled) ...
Hey, thanks for the proposal. I see this might be an issue, agreed. However, my worry is that someone might be making a new GET request because the previous one is broken. The way forward that I see is not to reject the new GET request but instead make sure that if one listening stream exists it is properly closed and replaced with this new one instead. WDYT? |
…o make way for the new listening SSE stream
Thanks for the helpful suggestion—code updated, tested, and confirmed working. |
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! I left my feedback to evolve the implementation towards a more atomic operation.
.../java/io/modelcontextprotocol/server/transport/WebFluxStreamableServerTransportProvider.java
Outdated
Show resolved
Hide resolved
.../java/io/modelcontextprotocol/server/transport/WebFluxStreamableServerTransportProvider.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.
I left a comment that needs addressing if you'd like to take it forward. Also, I think we need tests to validate that this change is doing what is expected. Let us know if you want to continue making progress here. Thanks!
mcp/src/main/java/io/modelcontextprotocol/spec/McpStreamableServerSession.java
Outdated
Show resolved
Hide resolved
I will continue to follow up and solve this problem.🤝 |
Motivation and Context
In the streamable context, repeated GET requests within the same session spawn multiple listening SSE connections, which
can lead the server to create unnecessary SSE
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context