-
Notifications
You must be signed in to change notification settings - Fork 136
Introduce server sessions #198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
var session: ServerSession? = null | ||
try { | ||
session = server.connectSession(transport) | ||
awaitCancellation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finishes without session
, not sure if it's a right approach to fix it
* @return The initialized and connected server session. | ||
*/ | ||
// TODO: Rename connectSession to connect after the full connect deprecation | ||
public suspend fun connectSession(transport: Transport): ServerSession { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be renamed to connect after several deprecation cycles, but now I can not name it like this due to Protocal
inheritance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should still consider changing the behavior of connect
?
If we simply mark connect
as deprecated, we’ll eventually have to remove it. But then we might end up deprecating connectSession
and reintroducing connect
again. And that will create new issues for us, because we forced users to move from connect
to connectSession
.
In that case, I think we have two options:
- Change the current
connect
(which would be a breaking change) - Deprecate
connect
and stick withconnectSession
In my opinion, the first option is preferable
f33add5
to
0265956
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me overall!
Please take a look at my comments.
My main concern is about the behavior of the Server
It would also be great to update the existing samples and the README accordingly
@@ -2934,6 +2934,11 @@ public final class io/modelcontextprotocol/kotlin/sdk/client/KtorClientKt { | |||
public static synthetic fun mcpSseTransport-5_5nbZA$default (Lio/ktor/client/HttpClient;Ljava/lang/String;Lkotlin/time/Duration;Lkotlin/jvm/functions/Function1;ILjava/lang/Object;)Lio/modelcontextprotocol/kotlin/sdk/client/SseClientTransport; | |||
} | |||
|
|||
public final class io/modelcontextprotocol/kotlin/sdk/client/MainKt { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that main.kt
was used for local testing and was mistakenly included in the API. Please remove it
} | ||
} | ||
|
||
@Suppress("FunctionName") | ||
@Deprecated("Use mcp() instead", ReplaceWith("mcp(block)"), DeprecationLevel.WARNING) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think the deprecated functions should have been removed
instead, the DeprecationLevel
should be increased to Error
@@ -83,23 +85,35 @@ public open class Server( | |||
private val serverInfo: Implementation, | |||
options: ServerOptions, | |||
) : Protocol(options) { | |||
private val sessions = atomic(persistentSetOf<ServerSession>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is set
instead of queue
?
@@ -111,8 +125,9 @@ public open class Server( | |||
get() = _resources.value | |||
|
|||
init { | |||
logger.debug { "Initializing MCP server with capabilities: $capabilities" } | |||
logger.debug { "Initializing MCP server with option: $options" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString
is not overridden for ServerOptions
, so this will output information that is useless for the user
@@ -156,9 +171,104 @@ public open class Server( | |||
} | |||
} | |||
|
|||
@Deprecated( | |||
"Will be removed with Protocol inheritance. Use connectSession instead.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use connectSession instead.
The deprecation message doesn’t seem entirely accurate. connectionSession
is a function and it doesn’t replace onClose
function.
I think it would be better to simply state that the architecture has changed to use Server Sessions
* @return The initialized and connected server session. | ||
*/ | ||
// TODO: Rename connectSession to connect after the full connect deprecation | ||
public suspend fun connectSession(transport: Transport): ServerSession { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should still consider changing the behavior of connect
?
If we simply mark connect
as deprecated, we’ll eventually have to remove it. But then we might end up deprecating connectSession
and reintroducing connect
again. And that will create new issues for us, because we forced users to move from connect
to connectSession
.
In that case, I think we have two options:
- Change the current
connect
(which would be a breaking change) - Deprecate
connect
and stick withconnectSession
In my opinion, the first option is preferable
@@ -667,6 +806,33 @@ public open class Server( | |||
return ListResourceTemplatesResult(listOf()) | |||
} | |||
|
|||
|
|||
@Deprecated( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m a bit confused about this part.
First, we can remove private functions, they’re not part of the public API.
Second, this is used in the init
block.
What will the behavior be in that case? Should we consider rewriting the init block as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces server sessions to manage multiple connections to MCP servers. The implementation moves protocol logic from the Server
class to a new ServerSession
class, allowing the server to handle multiple simultaneous connections while maintaining a single server instance.
- Introduces
ServerSession
class for managing individual client connections - Moves protocol-level functionality from
Server
toServerSession
- Updates WebSocket and SSE transport implementations to support multiple sessions
- Adds comprehensive integration tests for both single and multiple connection scenarios
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
SseIntegrationTest.kt | New integration test verifying SSE transport with single and multiple client connections |
WebSocketIntegrationTest.kt | New integration test verifying WebSocket transport with single and multiple client connections |
SseTransportTest.kt | Updates to use new SseTransportManager instead of direct transport map |
WebSocketMcpTransport.kt | Adds debug logging for WebSocket transport operations |
Protocol.kt | Refactors logger variable naming for consistency |
WebSocketMcpServerTransport.kt | Adds debug logging for session header validation |
WebSocketMcpKtorServerExtensions.kt | Major refactor to support session-based architecture with new API and deprecation warnings |
ServerSession.kt | New class implementing session-specific protocol handling moved from Server |
Server.kt | Refactored to manage multiple sessions while maintaining resource management |
SSEServerTransport.kt | Removes unused import |
KtorServer.kt | Introduces SseTransportManager for better session management |
WebSocketMcpKtorClientExtensions.kt | Adds debug logging for client connection process |
WebSocketClientTransport.kt | Adds debug logging for session initialization |
SSEClientTransport.kt | Adds debug logging for message sending and endpoint connection |
Client.kt | Improves error handling during connection initialization |
kotlin-sdk.api | Updates API surface to include new classes and deprecation warnings |
Comments suppressed due to low confidence (1)
src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt:37
- The test method name 'client should be able to connect to websocket server 2' is unclear. The '2' suffix doesn't indicate what distinguishes this test from others. Consider renaming to something more descriptive like 'client establishes websocket connection successfully'.
fun `client should be able to connect to websocket server 2`() = runTest {
} | ||
|
||
/** | ||
* Test Case #1: Two open connections, each client gets a client-specific prompt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says 'Test Case #1' but this appears to be a different test case than the previous 'Test Case #1' at line 55. Consider using sequential numbering or more descriptive identifiers.
* Test Case #1: Two open connections, each client gets a client-specific prompt | |
* Test Case #2: Two open connections, each client gets a client-specific prompt |
Copilot uses AI. Check for mistakes.
private suspend fun initClient(): Client { | ||
return HttpClient(ClientCIO) { install(SSE) }.mcpSse("http://$URL:$PORT") | ||
/** | ||
* Test Case #1: Two open connections, each client gets a client-specific prompt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says 'Test Case #1' but this appears to be a different test case than the previous 'Test Case #1' at line 53. Consider using sequential numbering or more descriptive identifiers.
* Test Case #1: Two open connections, each client gets a client-specific prompt | |
* Test Case #2: Two open connections, each client gets a client-specific prompt |
Copilot uses AI. Check for mistakes.
@Deprecated( | ||
"Use mcpWebSocket with a lambda that returns a Server instance instead", | ||
ReplaceWith("mcpWebSocket(block)"), | ||
DeprecationLevel.WARNING | ||
) | ||
public fun Route.mcpWebSocket( | ||
block: () -> Server | ||
) { | ||
webSocket { | ||
block().connect(createMcpTransport(this)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deprecated overload with the same signature as the new non-deprecated method on line 26 could cause confusion. Consider removing this overload or providing a different signature to avoid ambiguity.
@Deprecated( | |
"Use mcpWebSocket with a lambda that returns a Server instance instead", | |
ReplaceWith("mcpWebSocket(block)"), | |
DeprecationLevel.WARNING | |
) | |
public fun Route.mcpWebSocket( | |
block: () -> Server | |
) { | |
webSocket { | |
block().connect(createMcpTransport(this)) | |
} | |
} | |
// Removed the deprecated mcpWebSocket method with the block: () -> Server parameter. |
Copilot uses AI. Check for mistakes.
/** | ||
* Sends a notification to the client indicating that the list of tools has changed. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are duplicate javadoc comments here - one for 'Sends a notification to the client indicating that the list of prompts has changed' and another for 'Sends a notification to the client indicating that the list of tools has changed'. The second comment appears to be incorrect.
/** | |
* Sends a notification to the client indicating that the list of tools has changed. | |
*/ |
Copilot uses AI. Check for mistakes.
@@ -158,14 +158,13 @@ public open class Client( | |||
serverVersion = result.serverInfo | |||
|
|||
notification(InitializedNotification()) | |||
} catch (error: CancellationException) { | |||
throw IllegalStateException("Error connecting to transport: ${error.message}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CancellationException is being caught and re-thrown as IllegalStateException, but CancellationException typically indicates normal cancellation rather than an error state. Consider handling this differently or allowing it to propagate naturally.
throw IllegalStateException("Error connecting to transport: ${error.message}") | |
logger.info { "Connection was cancelled: ${error.message}" } | |
throw error |
Copilot uses AI. Check for mistakes.
Introduce a
ServerSession
to manage multiple connections to the mcp server.Protocall
logic of theServer
was moved toServerSession
, while MCP resources logic remained inServer
Motivation and Context
How Has This Been Tested?
Added
SseIntegrationTest
andWebSocketIntegrationTest
integration tests to check connection with single and multiple clientsBreaking Changes
This change caused several depreciations:
Server
->Protocol
inheritance (which caused single connection issue) should be removed in future releases, now, all the methods inherited fromProtocall
marked asDeprecated
with aWarning
Types of changes
Checklist
Additional context