Creating support for in memory transport

[colombod]

sketch design for in memory transport.
This is useful for creating tests for mcp servers and running mcp in process

[stephentoub]

The tests at https://github.com/modelcontextprotocol/csharp-sdk/blob/main/tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs already use an in-memory transport. It creates two pipes, one for each direction. It then uses the StdioServerTransport, instantiated with a stream for each of those as stdin/stdout:

csharp-sdk/tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs

Line 27 in 4b70bc7

sc.AddSingleton<IServerTransport>(new StdioServerTransport("TestServer", _clientToServerPipe.Reader.AsStream(), _serverToClientPipe.Writer.AsStream()));

and then for the client, it just uses a little bit of boilerplate distilled from the StdioClientTransport:
https://github.com/modelcontextprotocol/csharp-sdk/blob/main/tests/ModelContextProtocol.Tests/Transport/StreamClientTransport.cs

If we're going to add a replacement, we should dedup.

Though there's benefit to having all of these tests exercising StdioServerTransport rather than exercising only test code. Do we intend to ship an in-memory transport in the actual library?

[colombod]

The tests at https://github.com/modelcontextprotocol/csharp-sdk/blob/main/tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs already use an in-memory transport. It creates two pipes, one for each direction. It then uses the StdioServerTransport, instantiated with a stream for each of those as stdin/stdout:

csharp-sdk/tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs

Line 27 in 4b70bc7

sc.AddSingleton<IServerTransport>(new StdioServerTransport("TestServer", _clientToServerPipe.Reader.AsStream(), _serverToClientPipe.Writer.AsStream()));

and then for the client, it just uses a little bit of boilerplate distilled from the StdioClientTransport:
https://github.com/modelcontextprotocol/csharp-sdk/blob/main/tests/ModelContextProtocol.Tests/Transport/StreamClientTransport.cs
If we're going to add a replacement, we should dedup.

Though there's benefit to having all of these tests exercising StdioServerTransport rather than exercising only test code. Do we intend to ship an in-memory transport in the actual library?

Having the in memory was my final goal, could we reuse this code you already have instead of continue working on this pr?

[colombod]

@stephentoub , @eiriktsarpalis @halter73 this is more where i'd like to go, is there a better way to build this gesture? Maybe to use the core of the Stdtio classes ?

[halter73]

is there a better way to build this gesture? Maybe to use the core of the Stdtio classes ?

I agree with @stephentoub that we should be using System.IO.Pipelines for our in-memory test transports. That's also what we do for Kestrel tests. It would require serializing and deserializing the JSON-RPC messages unlike the InMemoryTransport in this PR, but I think that's a good thing. It gives us more real test coverage, and JSON serialization and deserialization is still extremely fast relative to launching processes or even connecting sockets.

I think using "the core of the Stdtio classes" could be alright, but the Stdio classes don't do much of interest to an in-memory transport aside from serialization, deserialization, and logging. The process management stuff is irrelevant. The logging would be nice to have, but we could probably add it to the StreamClientTransport. Is there an issue with using StreamClientTransport and StdioServerTransport with pipes like McpServerBuilderExtensionsToolsTests does?

[colombod]

is there a better way to build this gesture? Maybe to use the core of the Stdtio classes ?

I agree with @stephentoub that we should be using System.IO.Pipelines for our in-memory test transports. That's also what we do for Kestrel tests. It would require serializing and deserializing the JSON-RPC messages unlike the InMemoryTransport in this PR, but I think that's a good thing. It gives us more real test coverage, and JSON serialization and deserialization is still extremely fast relative to launching processes or even connecting sockets.

I think using "the core of the Stdtio classes" could be alright, but the Stdio classes don't do much of interest to an in-memory transport aside from serialization, deserialization, and logging. The process management stuff is irrelevant. The logging would be nice to have, but we could probably add it to the StreamClientTransport. Is there an issue with using StreamClientTransport and StdioServerTransport with pipes like McpServerBuilderExtensionsToolsTests does?

But why serialising if is in memory? Not sure I see the benefit of adding that when we do not need it, there is test coverage for serialization and not sure it is the right thing to do to mask it with a dependency here, I'd rather do not do additional work when the logic doesn't need it and have the relevant test coverage with the required tests if needed, not rely on some sort of integration tests side effect.

[stephentoub]

is there a better way to build this gesture? Maybe to use the core of the Stdtio classes ?

I agree with @stephentoub that we should be using System.IO.Pipelines for our in-memory test transports. That's also what we do for Kestrel tests. It would require serializing and deserializing the JSON-RPC messages unlike the InMemoryTransport in this PR, but I think that's a good thing. It gives us more real test coverage, and JSON serialization and deserialization is still extremely fast relative to launching processes or even connecting sockets.
I think using "the core of the Stdtio classes" could be alright, but the Stdio classes don't do much of interest to an in-memory transport aside from serialization, deserialization, and logging. The process management stuff is irrelevant. The logging would be nice to have, but we could probably add it to the StreamClientTransport. Is there an issue with using StreamClientTransport and StdioServerTransport with pipes like McpServerBuilderExtensionsToolsTests does?

But why serialising if is in memory? Not sure I see the benefit of adding that when we do not need it, there is test coverage for serialization and not sure it is the right thing to do to mask it with a dependency here, I'd rather do not do additional work when the logic doesn't need it and have the relevant test coverage with the required tests if needed, not rely on some sort of integration tests side effect.

I think it needs to serialize. If it doesn't, it's creating a variety of possible issues, e.g.

• Sharing objects between client and server, which then introduces the possibility of unexpected state corruption and concurrency problems where they shouldn't exist.
• Unexpected differences between transports. The transport layer shouldn't affect application-level behavior, but with serialization/deserialization happening for some transports and not for others, it means that any intricacies of serialization where data doesn't roundtrip with 100% fidelity will be visible.

[stephentoub]

I think the right answer here is effectively to:

1. Productize https://github.com/modelcontextprotocol/csharp-sdk/blob/main/tests/ModelContextProtocol.Tests/Transport/StreamClientTransport.cs, i.e. making it more robust, giving whatever better name we want, including it in the library rather than tests. (We might be able to layer it as a base class for StdioClientTransport, where StdioClientTransport adds on top of it the Process.Start logic.)
2. Create a helper method that produces a pair of that StreamClientTransport + StdioServerTransport types appropriately connected up.

We'll be able to leverage that in our tests, but others will also be able to utilize it for testing, and anyone that wants to use it in a production capacity could as well.

[colombod]

2. StreamClientTransport + StdioServerTransport

Do you mean StreamClientTransport + StreamServerTransport ?

[stephentoub]

2. StreamClientTransport + StdioServerTransport

Do you mean StreamClientTransport + StreamServerTransport ?

I meant StdioServerTransport, but it's logically the same thing. StdioServerTransport accepts two Streams, one representing stdin and one representing stdout. If you don't specify them, you're just getting Console.In/Out, but if you specify them, there's actually nothing "stdin"/"stdout" specific about StdioServerTransport.

[colombod]

2. StreamClientTransport + StdioServerTransport

Do you mean StreamClientTransport + StreamServerTransport ?

I meant StdioServerTransport, but it's logically the same thing. StdioServerTransport accepts two Streams, one representing stdin and one representing stdout. If you don't specify them, you're just getting Console.In/Out, but if you specify them, there's actually nothing "stdin"/"stdout" specific about StdioServerTransport.

But a StreamServerTransport requires stream and StdioServerTransport has an opinionated default behaviour on the streams represented by Console.In/ Out and setting encoding. That is why I was asking. The name StdioServerTransport gives me a clear idea of what it does and sets my expectations, the fact that can also be used as StreamServerTransport with in memory streams happens if I read carefully the documentation.

Now that you mention it then it make sense to have the StreamClientTransport and StdioServerTransport since the only difference in behaviour will happen wen calling the With*Transport gestures and that pushed the transports in the di container of the builder.

[stephentoub]

2. StreamClientTransport + StdioServerTransport

Do you mean StreamClientTransport + StreamServerTransport ?

I meant StdioServerTransport, but it's logically the same thing. StdioServerTransport accepts two Streams, one representing stdin and one representing stdout. If you don't specify them, you're just getting Console.In/Out, but if you specify them, there's actually nothing "stdin"/"stdout" specific about StdioServerTransport.

But a StreamServerTransport requires stream and StdioServerTransport has an opinionated default behaviour on the streams represented by Console.In/ Out and setting encoding. That is why I was asking. The name StdioServerTransport gives me a clear idea of what it does and sets my expectations, the fact that can also be used as StreamServerTransport with in memory streams happens if I read carefully the documentation.

Are you suggesting there be a:

public class StreamServerTransport : StdioServerTransport
{
    public StreamServerTransport(string serverName, Stream stdinStream, Stream stdoutStream, ILoggerFactory? loggerFactory = null) :
        base(serverName, stdinStream, stdoutStream, loggerFactory)
    {
    }
}

(or possibly inverted, with StdioServerTransport : StreamServerTransport) purely for discoverability reasons? Or are you suggesting something else?

[colombod]

StreamServerTransport

I am suggesting the other way around

public class StdioServerTransport: StreamServerTransport 
{
    public StdioServerTransport(string serverName,  ILoggerFactory? loggerFactory = null) :
        base(serverName, Console.In, Console.Out, loggerFactory)
    {
    }
}