Skip to content

Make supposedly unreachable code less reachable #178

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 5 commits into from
Apr 2, 2025
Merged

Conversation

halter73
Copy link
Contributor

@halter73 halter73 commented Apr 1, 2025

When I removed IServerTransport, I combined StartSession and InitializeSession, but I should have moved the call to StartSession into the McpServer constructor so you don't run into the following "unreachable" exception from McpServer.SendMessageAsync and SendRequestAsync called after McpServerFactory.Create but before McpServer.RunAsync.

 ModelContextProtocol.Tests.Server.McpServerTests.Can_SendMessage_Before_RunAsync
   Source: McpServerTests.cs line 588
   Duration: 81 ms

  Message: 
System.InvalidOperationException : This should be unreachable from public API! Call StartSession before sending messages.

  Stack Trace: 
McpJsonRpcEndpoint.GetSessionOrThrow() line 126
McpJsonRpcEndpoint.SendMessageAsync(IJsonRpcMessage message, CancellationToken cancellationToken) line 51
McpServerTests.Can_SendMessage_Before_RunAsync() line 597
McpServerTests.Can_SendMessage_Before_RunAsync() line 604
--- End of stack trace from previous location ---

  Standard Output: 
| [2025-04-01T15:47:33] ModelContextProtocol.Server.McpServer Information: Cleaning up endpoint Server (TestServer 1.0)
| [2025-04-01T15:47:33] ModelContextProtocol.Server.McpServer Information: Endpoint cleaned up for Server (TestServer 1.0)

It's unusual to send a message before handling any client requests like the initialization request, which is why this wasn't caught by our earlier tests, but it's possible that something like a log is fired off as fire-and-forget message, and there's no reason that should cause any issues like the exception fixed by this PR.

@halter73 halter73 changed the title Move WriteJsonRpcMessageToBuffer to method Make supposedly unreachable code less reachable Apr 1, 2025
@PederHP
Copy link
Collaborator

PederHP commented Apr 1, 2025

It's unusual to send a message before handling any client requests like the initialization request, which is why this wasn't caught by our earlier tests, but it's possible that the tools or prompts collection changing fired off a fire-and-forget message, and there's no reason that should cause any issues like the exception fixed by this PR.

The spec permits only one or two message types before initialization is complete (logging and one I can't remember). The logging one is odd because the spec shows elsewhere that logging messages shouldn't be sent until after the client makes a request to set logging level.

But if there's exceptions now more could happen, so even if it's nonsensical or not allowed the endpoints (still wish there was a better catchall for client and server) should be robust against it.

@halter73
Copy link
Contributor Author

halter73 commented Apr 1, 2025

I'm not taking a principled stance in favor of supporting sending messages using IMcpServer before receiving the initialize notification. We could prevent sending messages until initialization is complete similar to how we don't give you an IMcpClient until McpClient.ConnectAsync receives an InitializeResult, but we've never blocked on this before.

This PR is not that ambitious. It's just preventing the regression I caused yesterday by moving InitializeSession out of the McpServer constructor in #160 which now makes it possible to get an exception that "should be unreachable from public API!"

If we want to make it easier to delay sending messages until receiving the initialization notification, we can do that in a follow up PR, but I don't think it's too important considering it's unusual for the server to send messages before handling some sort of action initiated by the client.

@halter73 halter73 requested a review from PederHP April 1, 2025 17:44
Copy link

github-actions bot commented Apr 1, 2025

Code Coverage

Package Line Rate Branch Rate Complexity Health
ModelContextProtocol 77% 68% 10261
ModelContextProtocol.AspNetCore 89% 100% 15
TestServer 62% 39% 609
TestSseServer 91% 68% 255
Summary 77% (12334 / 16047) 67% (3666 / 5512) 11140

Minimum allowed line rate is 60%

@stephentoub stephentoub merged commit 674cb15 into main Apr 2, 2025
8 checks passed
@stephentoub stephentoub deleted the less-reachable branch April 2, 2025 02:30
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