Skip to content

Commit ee9f477

Browse files
authored
fix hang caused by StreamableHttpClientTransport (#226) (#227)
Re-throw exceptions in `StreamableHttpClientTransport` so `Protocol` doesn't hang indefinitely. ## Motivation and Context Fixes the issue described in #226 ## How Has This Been Tested? I've verified that my test case reproduces the error without my fix, and verified with breakpoints that the hang occurred where I described it. ## Breaking Changes None ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [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 <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [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 - [ ] I have added or updated documentation as needed ## Additional context I am unsure about the dispatcher I used for the real-time timeout here. I could not get the test to work otherwise, as `kotlinx.coroutines.test` was always tripping my timeout, with or without the fix. This is seemingly unrelated to the test case though, as removing my timeout results in the [timeout in Protocol](https://github.com/modelcontextprotocol/kotlin-sdk/blob/6bae9874c0ba02c683dfe61a416eeee0de0c6f32/kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt#L454) being tripped, even though that's not the point of the indefinite hang.
1 parent 1b60df8 commit ee9f477

File tree

2 files changed

+56
-2
lines changed

2 files changed

+56
-2
lines changed

kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpClientTransport.kt

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,10 @@ public class StreamableHttpClientTransport(
142142
ContentType.Application.Json -> response.bodyAsText().takeIf { it.isNotEmpty() }?.let { json ->
143143
runCatching { McpJson.decodeFromString<JSONRPCMessage>(json) }
144144
.onSuccess { _onMessage(it) }
145-
.onFailure(_onError)
145+
.onFailure {
146+
_onError(it)
147+
throw it
148+
}
146149
}
147150

148151
ContentType.Text.EventStream -> handleInlineSse(
@@ -313,7 +316,10 @@ public class StreamableHttpClientTransport(
313316
_onMessage(msg)
314317
}
315318
}
316-
.onFailure(_onError)
319+
.onFailure {
320+
_onError(it)
321+
throw it
322+
}
317323
}
318324
// reset
319325
id = null

kotlin-sdk-client/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpClientTransportTest.kt

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,18 @@ import io.ktor.http.HttpStatusCode
1212
import io.ktor.http.content.TextContent
1313
import io.ktor.http.headersOf
1414
import io.ktor.utils.io.ByteReadChannel
15+
import io.modelcontextprotocol.kotlin.sdk.Implementation
1516
import io.modelcontextprotocol.kotlin.sdk.JSONRPCMessage
1617
import io.modelcontextprotocol.kotlin.sdk.JSONRPCNotification
1718
import io.modelcontextprotocol.kotlin.sdk.JSONRPCRequest
1819
import io.modelcontextprotocol.kotlin.sdk.RequestId
1920
import io.modelcontextprotocol.kotlin.sdk.shared.McpJson
21+
import kotlinx.coroutines.Dispatchers
22+
import kotlinx.coroutines.TimeoutCancellationException
2023
import kotlinx.coroutines.delay
2124
import kotlinx.coroutines.test.runTest
25+
import kotlinx.coroutines.withContext
26+
import kotlinx.coroutines.withTimeout
2227
import kotlinx.serialization.json.JsonObject
2328
import kotlinx.serialization.json.JsonPrimitive
2429
import kotlinx.serialization.json.buildJsonObject
@@ -27,6 +32,7 @@ import kotlin.test.Test
2732
import kotlin.test.assertEquals
2833
import kotlin.test.assertNull
2934
import kotlin.test.assertTrue
35+
import kotlin.test.fail
3036
import kotlin.time.Duration.Companion.seconds
3137

3238
class StreamableHttpClientTransportTest {
@@ -380,4 +386,46 @@ class StreamableHttpClientTransportTest {
380386
assertEquals("resume-100", resumptionTokenReceived)
381387
transport.close()
382388
}
389+
390+
@Test
391+
fun testClientConnectWithInvalidJson() = runTest {
392+
// Transport under test: respond with invalid JSON for the initialize request
393+
val transport = createTransport { _ ->
394+
respond(
395+
"this is not valid json",
396+
status = HttpStatusCode.OK,
397+
headers = headersOf(HttpHeaders.ContentType, ContentType.Application.Json.toString()),
398+
)
399+
}
400+
401+
val client = Client(
402+
clientInfo = Implementation(
403+
name = "test-client",
404+
version = "1.0",
405+
),
406+
)
407+
408+
runCatching {
409+
// Real time-keeping is needed; otherwise Protocol will always throw TimeoutCancellationException in tests
410+
withContext(Dispatchers.Default.limitedParallelism(1)) {
411+
withTimeout(5.seconds) {
412+
client.connect(transport)
413+
}
414+
}
415+
}.onSuccess {
416+
fail("Expected client.connect to fail on invalid JSON response")
417+
}.onFailure { e ->
418+
when (e) {
419+
is TimeoutCancellationException -> fail("Client connect caused a hang", e)
420+
421+
is IllegalStateException -> {
422+
// Expected behavior: connect finishes and fails with an exception.
423+
}
424+
425+
else -> fail("Unexpected exception during client.connect", e)
426+
}
427+
}.also {
428+
transport.close()
429+
}
430+
}
383431
}

0 commit comments

Comments
 (0)