Skip to content

Conversation

@halter73
Copy link
Contributor

@halter73 halter73 commented Feb 6, 2026

Add more tests for #1250.

stephentoub
stephentoub previously approved these changes Feb 6, 2026
Copy link
Contributor Author

@halter73 halter73 left a 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?

Copy link
Contributor

Copilot AI commented Feb 6, 2026

@halter73 I've opened a new pull request, #1253, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI left a 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 MapMcp integration tests asserting DisposeAsync does not hang with OwnsSession=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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants