-
Notifications
You must be signed in to change notification settings - Fork 484
Convert EverythingServer to use Streamable HTTP #709
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
Well this may not be ideal because you can't comment on unchanged lines, and that's exactly where we probably need comments. So I'll just note here:
|
Maybe put all the server logic in an assembly of its own and then reference it from each a STDIO project and an HTTP project with the requisite (quite slim) boilerplate for each? Makes it much more useful as a sample than more complex ways of keeping it in one project, I think. Also helps it be a reference for porting a server from one transport to another. On the other hand if there is a better and more idiomatic pattern for multi-transport server projects, it should follow that I suppose. |
I like that idea @PederHP. One of the primary goals of this PR is just to show in the sample how advanced features can work with the Streamable HTTP transport. There are some key differences from the STDIO transport, even if you don't rely on any HTTP-specific concepts, primarily because the IMcpServer is a singleton service in STDIO mode, but not in Streamable HTTP mode where there are multiple IMcpServer instances which are not 1:1 with the ASP.NET Core request scope. Splitting this sample so the handlers are all implemented in a class library with slim STDIO and HTTP projects that contain basically just Program.cs seems like the most straightforward way to achieve this. We've also discussed trying to improve the local development experience by adding a command line switch to go between stdio and HTTP transports. This would be nice because while stdio is preferred for most local MCP servers, it's harder to debug during development. We figure that you probably wouldn't need much customization of your ASP.NET Core app in this development-focused mode, and just adding a command line switch would be easier than managing three separate projects. The downside is that it's more complex and requires the ASP.NET Core runtime even if you decide to use just STDIO. For any scenario where you're considering having both a stdio MCP server and an HTTP MCP server for real remote usage rather than just development, I'd definitely go for a multi-project architecture like you suggest, so it's probably worth having at least one sample demonstrates that project structure. |
HashSet<string> subscriptions = []; | ||
var _minimumLoggingLevel = LoggingLevel.Debug; | ||
// Subscriptions tracks resource URIs to McpServer instances | ||
Dictionary<string, List<IMcpServer>> subscriptions = new(); |
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.
We'll need to use thread-safe data structures like ConcurrentDictionary and ConcurrentQueue to manage the subscriptions since handlers can run in parallel even in the context of a single session. And the sessions can arrive in parallel as well. We'll also want to remove entries from any singleton data structures like this when the session ends.
You can override RunSessionHandler to run custom logic before the session begins and after it ends. We'll probably need to track all the resource URIs each session is subscribed to in order to remove them after IMcpServer.RunAsync()
completes.
However, given that we fire-and-forget message handlers like the SubscribeToResourcesHandler, that means that IMcpServer.RunAsync()
can complete before all the handlers do, which could make cleanup racy if you're not aware of that.
csharp-sdk/src/ModelContextProtocol.Core/McpSession.cs
Lines 118 to 122 in b067261
// Fire and forget the message handling to avoid blocking the transport. | |
if (message.ExecutionContext is null) | |
{ | |
_ = ProcessMessageAsync(); | |
} |
I don't think this race would come up super often, but I still hope this scenario is motivation to fix that. I think IMcpServer.RunAsync()
should wait for all handlers to complete similar to how WebApplication.RunAsync()
will wait for Kestrel to complete all its RequestDelegates before completing unless the host shutdown timeout fires first.
Here's a start at converting the EverythingServer to Streamable HTTP.
There are some things still to fix, but I want to get this posted so folks can have a look.
And we still want to consider how we could make this functional for either STDIO or HTTP.