Skip to content
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

Move the SseResponseStreamTransport out of sample #47

Merged

Conversation

halter73
Copy link
Contributor

@halter73 halter73 commented Mar 21, 2025

TODO:

  • Add tests
    • We now have some integration test coverage, because this is now used by the SseServerIntegrationTests.
  • Add logging I decided against adding logging since logging could be added by the creator of the transport like the ASP.NET Core sample or the HttpListenerSseServerTransport as demonstrated by this PR.
  • Use it in the HttpListenerSseServerTransport Done

I'm working on these things now, but if we like the API shape, I think we can merge it as-is to let people try it out and do the TODOs as follow ups.

@stephentoub
Copy link
Contributor

Use it in the HttpListenerSseServerTransport

Where is this being done?

@PederHP
Copy link
Collaborator

PederHP commented Mar 21, 2025

Use it in the HttpListenerSseServerTransport

Where is this being done?

I assume it's where HttpListenerSseServerTransport delegates SendMessageAsync to HttpListenerServerProvider which will :

public async Task SendEvent(string data, string eventId)
{
    if (_streamWriter == null)
    {
        throw new McpServerException("Stream writer not initialized");
    }
    if (eventId != null)
    {
        await _streamWriter.WriteLineAsync($"id: {eventId}").ConfigureAwait(false);
    }
    await _streamWriter.WriteLineAsync($"data: {data}").ConfigureAwait(false);
    await _streamWriter.WriteLineAsync().ConfigureAwait(false); // Empty line to finish the event
    await _streamWriter.FlushAsync().ConfigureAwait(false);
}

This could be provided by the shared SseResponseStreamTransport. HttpListenerServerProvider could have been folded directly into HttpListenerSseServerTransport (when it was changed from an implementation of an injected provider to a utility class). Using SseResponseStreamTransport would make it possible to do without HttpListenerServerProvider becoming bloated, so it's sounds like a good improvement to me.

@halter73
Copy link
Contributor Author

Where is this being done?

I added a commit to this PR. 265107f (#47)

@stephentoub
Copy link
Contributor

Where is this being done?

I added a commit to this PR. 265107f (#47)

Ok, cool, so I wasn't going crazy :-) Thanks.

@halter73 halter73 merged commit 1b1c7fa into modelcontextprotocol:main Mar 21, 2025
7 checks passed
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