- 
                Notifications
    
You must be signed in to change notification settings  - Fork 174
 
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
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 await in tests, not sure if it's a right approach to fix it, please take a look here! Really need some help here
        
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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
        
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/KtorServer.kt
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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 
ServerSessionclass for managing individual client connections - Moves protocol-level functionality from 
ServertoServerSession - 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 {
        
          
                ...commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt
          
            Show resolved
            Hide resolved
        
              
          
                src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/SseIntegrationTest.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...monMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/WebSocketMcpKtorServerExtensions.kt
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      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.
Hey @tiginamaria, thank you for the PR. Could you add a sample as a docs page illustrating what the difference will be between kotlin and the original TypeScript API?
        
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      560c13f    to
    f9f1a15      
    Compare
  
    fcf25b6    to
    18a4174      
    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.
lgtm!
e26cea1    to
    239fc97      
    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.
Thank you, @tiginamaria, @devcrocod. Callback chaining implementation is very simple and it can be improved in a separate PR along with the tests.
        
          
                kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/KtorServer.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/WebSocketIntegrationTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SseTransportTest.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...monMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/WebSocketMcpKtorClientExtensions.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...ore/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/WebSocketMcpTransport.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
          
            Show resolved
            Hide resolved
        
              
          
                ...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
          
            Show resolved
            Hide resolved
        
      | 
           @tiginamaria 
  | 
    
| 
           I found the problem: these tests use   | 
    
b66f0bf    to
    c609a01      
    Compare
  
    c622cfe    to
    6b3dcc4      
    Compare
  
    4ec36af    to
    b19353d      
    Compare
  
            
          
                ...lin/io/modelcontextprotocol/kotlin/sdk/integration/kotlin/AbstractResourceIntegrationTest.kt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | val serverEngine = initServer() | ||
| fun `client should be able to connect to sse server`() = runTest(timeout = 5.seconds) { | ||
| var server: EmbeddedServer<CIOApplicationEngine, CIOApplicationEngine.Configuration>? = null | ||
| var client: Client? = null | 
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.
| var client: Client? = null | |
| lateinit var client: Client | 
?
9d5cc91    to
    7c6e122      
    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.
Thank you! This PR looks good and useful to me. Let's get it merged and address any concerns in follow-up PRs
7c6e122    to
    5ba48b0      
    Compare
  
    | 
           Merging with the #297 in mind  | 
    
Introduce a
ServerSessionto manage multiple connections to the mcp server.Protocalllogic of theServerwas moved toServerSession, while MCP resources logic remained inServerMotivation and Context
How Has This Been Tested?
Added
SseIntegrationTestandWebSocketIntegrationTestintegration tests to check connection with single and multiple clientsBreaking Changes
This change caused several depreciations:
Server->Protocolinheritance (which caused single connection issue) should be removed in future releases, now, all the methods inherited fromProtocallmarked asDeprecatedwith aWarningTypes of changes
Checklist
Additional context