-
Notifications
You must be signed in to change notification settings - Fork 618
Add tests to verify McpClient.DisposeAsync doesn't hang #1252
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
tests/ModelContextProtocol.AspNetCore.Tests/MapMcpStreamableHttpTests.cs
Outdated
Show resolved
Hide resolved
tests/ModelContextProtocol.AspNetCore.Tests/StreamableHttpClientConformanceTests.cs
Outdated
Show resolved
Hide resolved
halter73
left a comment
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.
@copilot Can you update this PR to use Task.WaitAsync instead of combining Task.WhenAny with Task.Delay?
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
Adds regression coverage for issue #1250 to ensure McpClient.DisposeAsync() completes when HttpClientTransportOptions.OwnsSession=false in Streamable HTTP mode, including scenarios with an active GET/SSE stream and unsolicited server notifications.
Changes:
- Added a new conformance test that reproduces an indefinitely-open GET event stream and asserts disposal completes within a bounded time.
- Added two
MapMcpintegration tests assertingDisposeAsyncdoes not hang withOwnsSession=false, including after an unsolicited notification is sent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/ModelContextProtocol.AspNetCore.Tests/StreamableHttpClientConformanceTests.cs | Adds a regression test simulating an active GET/SSE stream to verify client disposal completes when the client doesn’t own the session. |
| tests/ModelContextProtocol.AspNetCore.Tests/MapMcpStreamableHttpTests.cs | Adds integration tests for MapMcp Streamable HTTP verifying disposal doesn’t hang (with/without unsolicited notifications). |
Add more tests for #1250.