Export callback to StreamableHTTPHandler for closed transports#480
Open
fgrosse wants to merge 10 commits intomodelcontextprotocol:mainfrom
Open
Export callback to StreamableHTTPHandler for closed transports#480fgrosse wants to merge 10 commits intomodelcontextprotocol:mainfrom
fgrosse wants to merge 10 commits intomodelcontextprotocol:mainfrom
Conversation
Adding a new callback that callers can optionally set in order to get notified about a connection created by a `StreamableHTTPHandler` was closed. Connections can be closed by the MCP client as part of the regular connection lifecycle (happy path) or when there was a connection error (e.g., a timeout). Fixes modelcontextprotocol#479
fgrosse
commented
Sep 17, 2025
…ServerTransport.Close()
Contributor
Author
|
@findleyr : let's continue the discussion from #479 (comment) here: I agree to all points and implemented the feedback via c999042 Now I'm playing with he idea of adding an What I dislike is that it requires additional documentation in the SDK, as it's not immediately intuitive to implement a simple cleanup task like this. The library already provides several callback functions, so it's natural for future application developers to expect an |
Contributor
Author
|
So, the The way I see it right now, the |
findleyr
previously approved these changes
Sep 18, 2025
Contributor
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
Thanks, very nice. Please just add one TODO as a reminder.
findleyr
previously approved these changes
Sep 18, 2025
Contributor
Author
|
I see tests are hanging and then eventually timed out. I saw the same flaky issue already on the main branch. Every once in a while tests seem to run into a deadlock: Stack trace in the the details below DetailsCan somebody else confirm this on the main branch as well so I'm not hunting ghosts on this feature branch? You might need more than 10 counts. It helps to test when the system is under stress 👻 |
This comment was marked as duplicate.
This comment was marked as duplicate.
Closed
Contributor
Author
|
@findleyr , can you try triggering the failing CI jobs, just to see if its a flake? |
Contributor
|
@fgrosse I think it's related to this change: I re-triggered and they failed again. I can debug in a little bit. |
Contributor
Author
|
You are right - the issue was on the branch and I fixed it via f3b5d93 It was a surprisingly subtle issue in the order of teardown for |
Contributor
Author
|
I have rebased this branch onto |
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
2 participants
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.
This PR implements the changes suggested via #479
We are introducing a new callback that callers can optionally set to receive a notification when a
StreamableHTTPHandlerconnection is closed. Connections can be closed by the MCP client as part of the regular connection lifecycle (happy path) or when there was a connection error (e.g., a timeout).Fixes #479