Skip to content

Refactor transports to help enable graceful shutdown #142

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Mar 31, 2025
Merged

Refactor transports to help enable graceful shutdown #142

merged 12 commits into from
Mar 31, 2025

Conversation

@halter73
Copy link
Contributor

I took this over from #122

  • This also paves the way for better multi-session support
  • We should definitely rethink names for the transport API. But for now, I kept the names similar as possible, so we can focus on the API shape.

stephentoub and others added 5 commits March 27, 2025 18:13
@stephentoub
Enable graceful shutdown of servers
4ae26c0
@halter73
Merge branch 'main' into gracefulshutdown
2ef38ff 
# Conflicts:
#	src/ModelContextProtocol/Client/McpClient.cs
#	src/ModelContextProtocol/Protocol/Transport/StdioServerTransport.cs
@halter73
Call IMcpServer.RunAsync in AspNetCoreSseServer
fbe1dbe
@halter73
Refactor transports to enable graceful shutdown
4b7aabb 
- This also paves the way for better multi-session support
- We should definitely rethink names for the transport API
- For now, I kept the names similar as possible, so we can
  focus on the API shape
@halter73
Merge branch 'main' into gracefulshutdown
c6e9958
@halter73 halter73 mentioned this pull request Mar 29, 2025
Closed
PederHP
PederHP reviewed Mar 30, 2025
View reviewed changes
@stephentoub stephentoub mentioned this pull request Mar 30, 2025
Closed
halter73 added 2 commits March 30, 2025 14:04
@halter73
Address PR feedback
1d44cec 
- Use SemaphoreSlim in McpJsonRpcEndpoint
- Dispose IClientTransport on connection failure
- Test cleanup
@halter73
Fix flaky Should_Start_In_Connected_State test
cba8523
@halter73
Copy link
Contributor Author

The faster test runs after these changes are really nice.

csharp-sdk [main] > dotnet test --filter ModelContextProtocol.Tests.ClientIntegrationTests
...
Test summary: total: 41, failed: 0, succeeded: 39, skipped: 2, duration: 96.3s
csharp-sdk [main] > checkout gracefulshutdown
csharp-sdk [main] > dotnet test --filter ModelContextProtocol.Tests.ClientIntegrationTests
...
Test summary: total: 41, failed: 0, succeeded: 39, skipped: 2, duration: 37.8s

@stephentoub
Copy link
Contributor

Yeah, those 15-30s sleeps in the tests were killer.

stephentoub
stephentoub reviewed Mar 30, 2025
View reviewed changes
@halter73 halter73 merged commit 2ab9db0 into modelcontextprotocol:main Mar 31, 2025
8 checks passed
This was referenced Mar 31, 2025
Closed
Closed
@cristipufu
Copy link

cristipufu commented Mar 31, 2025 •
edited
Loading

@halter73 @stephentoub how should we handle multi-nodes deployments?
eg:

  • session connects to sse on node1
  • session makes tool call request on node2
  • need to send tool call updates for session on node1

@eiriktsarpalis eiriktsarpalis mentioned this pull request Mar 31, 2025
Closed
@stephentoub stephentoub mentioned this pull request Apr 1, 2025
Merged
9 tasks
@halter73 halter73 mentioned this pull request Apr 25, 2025
Closed
This was referenced Jul 24, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis

+1 more reviewer

@PederHP PederHP PederHP left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@halter73 @stephentoub @cristipufu @PederHP