Skip to content

Modify the wpf to connect to the mcp server using the stdio method #694

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions src/ModelContextProtocol.Core/Client/StdioClientTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -156,20 +156,21 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
// up the encoding from Console.InputEncoding. As such, when not targeting .NET Core,
// we temporarily change Console.InputEncoding to no-BOM UTF-8 around the Process.Start
// call, to ensure it picks up the correct encoding.
#if NET
processStarted = process.Start();
#else
Comment on lines -159 to -161
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the simple #if NET (.NET Core) case which never sets Console.InputEncoding, so it cannot be the cause of your issues.


Encoding originalInputEncoding = Console.InputEncoding;
try
if (HasConsole())
{
Console.InputEncoding = StreamClientSessionTransport.NoBomUtf8Encoding;
processStarted = process.Start();
}
finally
{
Console.InputEncoding = originalInputEncoding;
try
{
Console.InputEncoding = StreamClientSessionTransport.NoBomUtf8Encoding;
}
finally
{
Console.InputEncoding = originalInputEncoding;
}
}
#endif

processStarted = process.Start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need start the process before resetting back to the originalInputEncoding for it to matter.


if (!processStarted)
{
Expand Down Expand Up @@ -199,7 +200,13 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
throw new IOException("Failed to connect transport.", ex);
}
}
[DllImport("kernel32.dll")]
private static extern IntPtr GetConsoleWindow();

private static bool HasConsole()
{
try { return GetConsoleWindow() != IntPtr.Zero; } catch { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very suspicious this is the best way to determine whether the Console.InputEncoding setter is going to throw a NotSupportedException.

If we're going to catch arbitrary exceptions anyway, it would be better to catch the NotSupportedException from the setter although I'm not sure that's the best approach either. I think #73 added this logic. I'm curious if @willibrandon or @stephentoub have alternatives.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to catch arbitrary exceptions anyway, it would be better to catch the NotSupportedException from the setter

Yeah, I'd rather just catch/eat the exception from the actual operation being performed than have a separate IsXx method that does the exact same thing and that's then used as a guard. We're about to launch a whole process; I'm not particularly concerned at the overhead of an exception.

That said, I'm unclear about the originally stated problem. I just created a WPF app, set both Console.OutputEncoding and Console.InputEncoding, and it worked fine, no exceptions. The code for these properties in Console also just bails without exception for what I would have expected in these cases:
https://github.com/dotnet/runtime/blob/edb570cd9e75036b6c61b201a0a9a9f2301db276/src/libraries/System.Console/src/System/ConsolePal.Windows.cs#L135-L138

Can you share a repro of the issue?

}
internal static void DisposeProcess(
Process? process, bool processRunning, TimeSpan shutdownTimeout, string endpointName)
{
Expand Down