fix(client): invoke onClose when SSE session disconnects (#437)#738
Open
jskjw157 wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
fix(client): invoke onClose when SSE session disconnects (#437)#738jskjw157 wants to merge 1 commit intomodelcontextprotocol:mainfrom
jskjw157 wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
…tprotocol#437) The SSE client transport's `collectMessages` coroutine releases resources when the underlying SSE stream completes, but never invoked the registered `onClose` callbacks. As a result, code listening for transport closure (e.g. the MCP `Client`) was not notified when a server closed the SSE connection, leaving callers unable to detect disconnects. Mirror the pattern already used by `StdioClientTransport`: after `closeResources()` in the `finally` block, call the protected `invokeOnCloseCallback()` helper. The helper is idempotent via an `AtomicBoolean`, so the user-initiated `close()` path remains safe.
Contributor
Author
|
@devcrocod when you have a moment — small fix for the SSE disconnect-detection bug from #437. The change is one line plus a regression test; the existing StdioClientTransport already does this, so this aligns SSE with the established pattern. Happy to adjust the test setup if you prefer a different approach to simulating the server-side disconnect. |
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.
Fixes the SSE client transport so that registered
onClosecallbacks fire when the underlying SSE session ends — whether the server closes the stream gracefully or an error event terminates it.closes #437
Motivation and Context
SseClientTransport.collectMessagesalready runscloseResources()in itsfinallyblock whensession.incomingcompletes, but it never invoked theonClosecallback chain. As a result, the MCPClient(and any user code callingtransport.onClose { ... }) had no way to detect that the server-side SSE connection had dropped.The other client transports already do this correctly:
StdioClientTransportcallsinvokeOnCloseCallback()from its read-loopfinally(kotlin-sdk-client/src/.../StdioClientTransport.kt:224).AbstractTransport.invokeOnCloseCallback()helper guards against duplicate firing with anAtomicBoolean, so it is safe to call from both the read-loopfinallyand the user-initiatedclose()path.The fix mirrors that established pattern in one line.
How Has This Been Tested?
Added
onClose callback fires when the SSE stream is disconnected by the servertoSseClientTransportTest. The test:MockSseClientEngine.disconnectSseStream()helper.onClosecallback fires (using Kotesteventually, since the close propagates on a real-time dispatcher).I also verified the test fails before the fix and passes after.
Breaking Changes
None.
invokeOnCloseCallback()isprotectedonAbstractTransport; this PR only adds a call site insideSseClientTransport. No public API surface changes.Types of changes
Checklist