fix: propagate CancellationException instead of logging as error (#706)
fixes #242 ## How Has This Been Tested? new and existing tests pass ## Breaking Changes none ## Types of changes - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [x] I have added appropriate error handling - [x] I have added or updated documentation as needed
This commit is contained in:
parent
bce6811b66
commit
e7b09d84f7
3 changed files with 109 additions and 25 deletions
|
|
@ -277,12 +277,15 @@ public abstract class Protocol(@PublishedApi internal val options: ProtocolOptio
|
|||
}
|
||||
try {
|
||||
handler(notification)
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
} catch (cause: Throwable) {
|
||||
logger.error(cause) { "Error handling notification: ${notification.method}" }
|
||||
onError(cause)
|
||||
}
|
||||
}
|
||||
|
||||
@Suppress("ThrowsCount")
|
||||
private suspend fun onRequest(request: JSONRPCRequest) {
|
||||
logger.trace { "Received request: ${request.method} (id: ${request.id})" }
|
||||
|
||||
|
|
@ -300,6 +303,8 @@ public abstract class Protocol(@PublishedApi internal val options: ProtocolOptio
|
|||
),
|
||||
),
|
||||
)
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
} catch (cause: Throwable) {
|
||||
logger.error(cause) { "Error sending method not found response" }
|
||||
onError(cause)
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ import kotlinx.coroutines.job
|
|||
import kotlinx.coroutines.launch
|
||||
import kotlin.concurrent.atomics.AtomicBoolean
|
||||
import kotlin.concurrent.atomics.ExperimentalAtomicApi
|
||||
import kotlin.coroutines.cancellation.CancellationException
|
||||
|
||||
/** WebSocket subprotocol identifier for MCP connections. */
|
||||
public const val MCP_SUBPROTOCOL: String = "mcp"
|
||||
|
|
@ -44,6 +45,7 @@ public abstract class WebSocketMcpTransport : AbstractTransport() {
|
|||
*/
|
||||
protected abstract suspend fun initializeSession()
|
||||
|
||||
@Suppress("ThrowsCount")
|
||||
override suspend fun start() {
|
||||
logger.debug { "Starting websocket transport" }
|
||||
|
||||
|
|
@ -74,6 +76,8 @@ public abstract class WebSocketMcpTransport : AbstractTransport() {
|
|||
try {
|
||||
val message = McpJson.decodeFromString<JSONRPCMessage>(message.readText())
|
||||
_onMessage.invoke(message)
|
||||
} catch (e: CancellationException) {
|
||||
throw e
|
||||
} catch (e: Exception) {
|
||||
_onError.invoke(e)
|
||||
throw e
|
||||
|
|
|
|||
|
|
@ -1,11 +1,14 @@
|
|||
package io.modelcontextprotocol.kotlin.sdk.shared
|
||||
|
||||
import io.kotest.assertions.throwables.shouldThrow
|
||||
import io.kotest.matchers.collections.shouldContainExactly
|
||||
import io.kotest.matchers.collections.shouldHaveSize
|
||||
import io.kotest.matchers.nulls.shouldNotBeNull
|
||||
import io.kotest.matchers.shouldBe
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.CustomRequest
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.EmptyResult
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCMessage
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCNotification
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCRequest
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.JSONRPCResponse
|
||||
import io.modelcontextprotocol.kotlin.sdk.types.McpJson
|
||||
|
|
@ -24,6 +27,7 @@ import kotlinx.serialization.json.encodeToJsonElement
|
|||
import kotlinx.serialization.json.int
|
||||
import kotlinx.serialization.json.jsonObject
|
||||
import kotlinx.serialization.json.jsonPrimitive
|
||||
import kotlin.coroutines.cancellation.CancellationException
|
||||
import kotlin.test.BeforeTest
|
||||
import kotlin.test.Test
|
||||
|
||||
|
|
@ -126,6 +130,36 @@ class ProtocolTest {
|
|||
inFlight.await()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should propagate CancellationException from notification handler without calling onError`() = runTest {
|
||||
protocol.connect(transport)
|
||||
|
||||
protocol.fallbackNotificationHandler = {
|
||||
throw CancellationException("test cancellation")
|
||||
}
|
||||
|
||||
shouldThrow<CancellationException> {
|
||||
transport.deliver(JSONRPCNotification(method = "test/notification"))
|
||||
}
|
||||
|
||||
protocol.errors shouldHaveSize 0
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should report non-cancellation exception from notification handler via onError`() = runTest {
|
||||
protocol.connect(transport)
|
||||
|
||||
protocol.fallbackNotificationHandler = {
|
||||
throw IllegalStateException("handler failed")
|
||||
}
|
||||
|
||||
// Non-CE exceptions are caught and reported, not propagated
|
||||
transport.deliver(JSONRPCNotification(method = "test/notification"))
|
||||
|
||||
protocol.errors shouldHaveSize 1
|
||||
protocol.errors[0].message shouldBe "handler failed"
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `should create params object when request params are null`() = runTest {
|
||||
protocol.connect(transport)
|
||||
|
|
@ -154,6 +188,12 @@ class ProtocolTest {
|
|||
}
|
||||
|
||||
private class TestProtocol : Protocol(null) {
|
||||
val errors = mutableListOf<Throwable>()
|
||||
|
||||
override fun onError(error: Throwable) {
|
||||
errors.add(error)
|
||||
}
|
||||
|
||||
override fun assertCapabilityForMethod(method: Method) {
|
||||
// noop
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue