Remove closed sessions from list in WebMvcSseServerTransport#19
Merged
tzolov merged 1 commit intomodelcontextprotocol:mainfrom Feb 25, 2025
Merged
Conversation
tzolov
approved these changes
Feb 25, 2025
Contributor
tzolov
left a comment
There was a problem hiding this comment.
Thanks you @denniskawurek
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove a session from the session list in
WebMvcSseServerTransportwhen it times out or closes.Motivation and Context
This is related to: spring-projects/spring-ai#2267
When a client closes its connection then the session is still kept in the list of sessions. The transport then tries to broadcast messages to the outdated sessions and it fails with exception:
How Has This Been Tested?
Unfortunately the active sessions can't be retrieved from within a test, so manual testing was done.
I created a client - server application where 100 clients connect to a server and close their connection after doing a tool request.
2025-02-22T15:23:14.835+01:00 DEBUG 396597 --- [mcp-server-basic] [io-8080-exec-10] i.m.s.t.WebMvcSseServerTransport : Attempting to broadcast message to 100 active sessionsAfter that another 100 clients do the same and the number of active sessions grows (although it should be 100):
2025-02-22T15:24:09.244+01:00 DEBUG 396597 --- [mcp-server-basic] [io-8080-exec-26] i.m.s.t.WebMvcSseServerTransport : Attempting to broadcast message to 200 active sessionsAlso, the following exception is thrown for each old, inactive session.
With the provided fix the sessions are removed from the list via callbacks on timeout and on complete.
The exception doesn't appear anymore.
Types of changes
Checklist