fix: handle CancellationException in WebSocketTransport#663
fix: handle CancellationException in WebSocketTransport#663
CancellationException in WebSocketTransport#663Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses flaky WebSocket transport shutdown behavior by treating coroutine cancellation as a normal close signal instead of an error, and re-enables the previously ignored WebSocket integration tests with a safety timeout to prevent CI hangs.
Changes:
- Suppress
_onErrorcallbacks when a WebSocket session completes withCancellationException, invoking the close callback instead. - Re-enable
WebSocketTransportTestcases and migrate them to JUnit Jupiter annotations. - Add a class-level JUnit timeout to reduce CI flakiness/hangs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/WebSocketMcpTransport.kt | Treats CancellationException as a normal shutdown path in the session completion handler. |
| integration-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/WebSocketTransportTest.kt | Re-enables WebSocket transport integration tests and adds a JUnit timeout to stabilize CI execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @OptIn(InternalCoroutinesApi::class) | ||
| session.coroutineContext.job.invokeOnCompletion { | ||
| if (it != null) { | ||
| if (it != null && it !is CancellationException) { |
There was a problem hiding this comment.
Shouldn’t the CancellationException be also rethrown?
There was a problem hiding this comment.
No, rethrowing here would be incorrect
devcrocod
force-pushed
the
devcrocod/websocket-transport-cancellation
branch
from
cbc9c57 to
c4fa2df
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCMessage | ||
| import io.modelcontextprotocol.kotlin.sdk.types.McpJson | ||
| import kotlinx.coroutines.CancellationException | ||
| import kotlinx.coroutines.CoroutineName |
There was a problem hiding this comment.
CancellationException is imported from kotlinx.coroutines here, but other commonMain code in this module uses kotlin.coroutines.cancellation.CancellationException (e.g., AbstractClientTransport). Consider switching to the stdlib import for consistency and to avoid confusion with java.util.concurrent.CancellationException on JVM.
closes #17
How Has This Been Tested?
integration tests
Breaking Changes
none
Types of changes
Checklist