-
Notifications
You must be signed in to change notification settings - Fork 342
Handle stdin EOF gracefully in MCP stdio mode (#3428) #3647
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,17 +61,20 @@ public McpStdioServer(McpToolRegistry toolRegistry, IServiceProvider serviceProv | |
| /// <returns>A task representing the asynchronous operation.</returns> | ||
| public async Task RunAsync(CancellationToken cancellationToken) | ||
| { | ||
| // Use UTF-8 WITHOUT BOM for stdin. Stdout is owned by McpStdoutWriter, | ||
| // which serializes all writes from McpStdioServer and the MCP logging | ||
| // pipeline so JSON-RPC frames cannot interleave at the byte level. | ||
| UTF8Encoding utf8NoBom = new(encoderShouldEmitUTF8Identifier: false); | ||
|
|
||
| using Stream stdin = Console.OpenStandardInput(); | ||
| using StreamReader reader = new(stdin, utf8NoBom); | ||
| // Read through Console.In so tests can inject stdin and the process | ||
| // still follows the configured console input encoding in stdio mode. | ||
| TextReader reader = Console.In; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we no longer need utf8NoBom for stdin? Why was it needed before and what changed? |
||
|
|
||
| while (!cancellationToken.IsCancellationRequested) | ||
| { | ||
| string? line = await reader.ReadLineAsync(cancellationToken); | ||
|
|
||
| // EOF (stdin pipe closed) is a normal shutdown signal for stdio mode. | ||
| if (line is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(line)) | ||
| { | ||
| continue; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| #nullable enable | ||
|
|
||
| using System; | ||
| using System.IO; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Azure.DataApiBuilder.Mcp.Core; | ||
| using Microsoft.Extensions.DependencyInjection; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
|
|
||
| namespace Azure.DataApiBuilder.Service.Tests.UnitTests | ||
| { | ||
| [TestClass] | ||
| public class McpStdioServerRunAsyncTests | ||
| { | ||
| [TestMethod] | ||
| public async Task RunAsync_EofOnStdin_ExitsGracefullyWithoutOutput() | ||
| { | ||
| (McpStdioServer server, StringWriter stdoutCapture) = CreateServerWithCapturedOutput(); | ||
| TextReader originalIn = Console.In; | ||
|
|
||
| try | ||
| { | ||
| // Empty input immediately yields EOF (ReadLineAsync returns null). | ||
| Console.SetIn(new StringReader(string.Empty)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Console.SetIn is process-global, if we ever parallelize this test it could break other tests that read Console.In. Can instead extract the read source, and then tests can pass a StringReader directly. |
||
|
|
||
| await server.RunAsync(CancellationToken.None); | ||
|
|
||
| Assert.AreEqual(string.Empty, stdoutCapture.ToString(), | ||
| "Server should exit cleanly on EOF without emitting protocol output."); | ||
| } | ||
| finally | ||
| { | ||
| Console.SetIn(originalIn); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task RunAsync_BlankLineThenShutdown_IgnoresBlankLineAndHandlesShutdown() | ||
| { | ||
| (McpStdioServer server, StringWriter stdoutCapture) = CreateServerWithCapturedOutput(); | ||
| TextReader originalIn = Console.In; | ||
|
|
||
| try | ||
| { | ||
| Console.SetIn(new StringReader(Environment.NewLine + | ||
| "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"shutdown\"}" + | ||
| Environment.NewLine)); | ||
|
|
||
| await server.RunAsync(CancellationToken.None); | ||
|
|
||
| string[] lines = stdoutCapture | ||
| .ToString() | ||
| .Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries); | ||
|
|
||
| Assert.AreEqual(1, lines.Length, | ||
| "Expected a single response line for shutdown request."); | ||
| StringAssert.Contains(lines[0], "\"id\":1"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: parsing this line as JSON and then asserting on jsonrpc/id/result.ok would align more closely with the other mcp tests while being a bit more robust. |
||
| StringAssert.Contains(lines[0], "\"ok\":true"); | ||
| } | ||
| finally | ||
| { | ||
| Console.SetIn(originalIn); | ||
| } | ||
| } | ||
|
|
||
| private static (McpStdioServer server, StringWriter stdoutCapture) CreateServerWithCapturedOutput() | ||
| { | ||
| StringWriter stdoutCapture = new(); | ||
| McpStdoutWriter stdoutWriter = new(stdoutCapture); | ||
|
|
||
| ServiceCollection services = new(); | ||
| services.AddSingleton(stdoutWriter); | ||
| services.AddSingleton<McpToolRegistry>(); | ||
| IServiceProvider serviceProvider = services.BuildServiceProvider(); | ||
|
|
||
| McpStdioServer server = new( | ||
| serviceProvider.GetRequiredService<McpToolRegistry>(), | ||
| serviceProvider); | ||
|
|
||
| return (server, stdoutCapture); | ||
| } | ||
| } | ||
| } | ||
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.
nit: leaks test concerns into production code. from a production standpoint, the rationale here I think is that we want to honor the configured Console.In ie:
// Read via Console.In so the loop honors the configured Console.InputEncoding
// (set to UTF-8 by Program.Main for stdio mode) and shares the SyncTextReader.