Skip to content

Use Kestrel for all in-memory HTTP tests #225

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

Merged
merged 12 commits into from
Apr 7, 2025

Conversation

halter73
Copy link
Contributor

@halter73 halter73 commented Apr 6, 2025

The bulk of this PR is adding a Kestrel in-memory transport for testing, but it also does a few other things.

  • Add route pattern parameter to MapMcp
  • Add configureOptionsAsync parameter to MapMcp
  • Use ApplicationStopping token for SSE responses. SignalR does the same thing FWIW

One of the most unfortunate things I learned while making this PR is that SocketsHttpHandler doesn't consistently respect the CancellationToken parameter at least when reading an unbuffered SSE response stream. Without the following change:

- await foreach (SseItem<string> sseEvent in SseParser.Create(stream).EnumerateAsync(cancellationToken).ConfigureAwait(false))
+ var sseEventEnumerator = SseParser.Create(stream).EnumerateAsync(cancellationToken).GetAsyncEnumerator();
+ while (await sseEventEnumerator.MoveNextAsync().AsTask().WaitAsync(cancellationToken).ConfigureAwait(false))
{
+    SseItem<string> sseEvent = sseEventEnumerator.Current;

SseClientSessionTransport.DisposeAsync would hang waiting on this line HttpConnection.FillAsync were it calls ReadAsync on the response stream with CancellationToken.None.

I searched the runtime repo to see if anyone had already filed an issue, and I couldn't find one. If none of us can find an existing issue, I can file a new one. The code I added to work around the issue fixes our tests, but leaves zombie SSE response stream, unlike a real fix to SocketsHttpHandler.

I'm guessing this wouldn't be an issue given a real socket, because I didn't see shutdown take 30 seconds until I started using the ConnectCallback.

See #225 (comment) for the most up-to-date info.

@halter73
Copy link
Contributor Author

halter73 commented Apr 6, 2025

I still hope to have a PR open tomorrow with a lot more Streamable HTTP functionality similar to what's currently in these draft PRs:

But as you can tell from both of these still being draft PRs, we're still in the very early days of support for Streamable HTTP as defined in the 2025-03-26 spec.

I also expect the MapMcp signature to change significantly going forward. Instead of taking the configureOptionsAsync and runTransportAsync as direct parameters, I plan to add HttpMcpServerOptions to configure these callbacks. I also plan to move most of the implementation into a proper service with constructor-injected dependencies. I held off on this for now as this will require people update their code to call IMcpServerBuilder.WithAspNetCoreTransport() or something like that, and this PR was already getting large enough.

This sets the groundwork for better support by significantly improving the testing and development inner loop by making the test runs much faster and allowing you to easily put both server-side and client-side code in the same test method. I decided to send a PR now instead of when more of the Streamable HTTP work is done, because these changes affect more than just the ModelContextProtocol.AspNetCore package, and could change how other people write their HTTP-based tests.

@stephentoub
Copy link
Contributor

stephentoub commented Apr 7, 2025

would hang waiting on this line HttpConnection.FillAsync were it calls ReadAsync on the response stream with CancellationToken.None

Its current design is to register with the token around the whole operation:
https://github.com/dotnet/runtime/blob/367e0a8a23e444a94c029fcb72dcb4ac66bd6717/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L551
such that canceling the token will dispose of the connection and tear down the in-flight operation:
https://github.com/dotnet/runtime/blob/367e0a8a23e444a94c029fcb72dcb4ac66bd6717/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L964
Is that not happening? For the SSE path, I'd expect it would be the same answer, just with the registration happening around the whole top-level read, e.g.
https://github.com/dotnet/runtime/blob/367e0a8a23e444a94c029fcb72dcb4ac66bd6717/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RawConnectionStream.cs#L64-L76

(dotnet/runtime#112447 tracks reconsidering the design.)

halter73 added 3 commits April 6, 2025 18:26
# Conflicts:
#	tests/ModelContextProtocol.Tests/SseIntegrationTests.cs
…on the stream returned by the ConnectCallback

- Move workaround to KestrelInMemoryConnection instead of SseClientSessionTransport which is product code
@halter73
Copy link
Contributor Author

halter73 commented Apr 7, 2025

I figured out what was going on. SocketsHttpHandler will call Dispose on the Stream returned by the ConnectCallback, but not DisposeAsync. I updated KestrelInMemoryConnection.ClientStream to implement Dispose instead of DisposeAsync, and reverted the workaround in SseClientSessionTransport, so the workaround is now completely in test code.

I'll also submit a PR to the runtime repo to fix SocketsHttpHandler to call DisposeAsync in this case.

I do think it would be even better if the CancellationToken passed into ReadAsync also got cancelled, but at least what we have now works.

@halter73
Copy link
Contributor Author

halter73 commented Apr 7, 2025

@stephentoub It looks like DisposeAsyncCompletesImmediatelyWhenInvokedFromHandler is flaky.

https://github.com/modelcontextprotocol/csharp-sdk/actions/runs/14298882057/job/40069881748?pr=225

@stephentoub
Copy link
Contributor

stephentoub commented Apr 7, 2025

@stephentoub It looks like DisposeAsyncCompletesImmediatelyWhenInvokedFromHandler is flaky.

https://github.com/modelcontextprotocol/csharp-sdk/actions/runs/14298882057/job/40069881748?pr=225

Delete that assert (L245). It's bogus. The handler might not yet have fully exited by the time we get to that line, in which case the DisposeAsync may complete asynchronously.

halter73 added 2 commits April 6, 2025 18:57
halter73 added 2 commits April 6, 2025 19:18
- Go back to not processing messages until IMcpServer.RunAsync is called
- style: consistently use mcpServer from parameter rather than features

Failed test run: https://github.com/modelcontextprotocol/csharp-sdk/actions/runs/14299175970/job/40070612363?pr=225
- Get most of the changes that were supposed to be in the last commit
- Go back to not processing messages until IMcpServer.RunAsync is called

Failed test run: https://github.com/modelcontextprotocol/csharp-sdk/actions/runs/14299175970/job/40070612363?pr=225
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.

None yet

2 participants