Avoid intermediate strings in MCP transport serialization#1274
Avoid intermediate strings in MCP transport serialization#1274stephentoub merged 13 commits intomainfrom
Conversation
- StreamServerTransport.SendMessageAsync: Use SerializeToUtf8Bytes instead of Serialize + Encoding.UTF8.GetBytes - StreamClientSessionTransport.SendMessageAsync: Refactor to write UTF-8 bytes directly to the stream instead of going through TextWriter with a string - McpHttpClient (netstandard2.0 path): Use SerializeToUtf8Bytes + ByteArrayContent instead of Serialize + StringContent - ClientOAuthProvider: Use SerializeToUtf8Bytes + ByteArrayContent instead of Serialize + StringContent Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
The refactoring to bypass TextWriter and write directly to the underlying Stream changed the timing of pipe writes, causing the StdioClientTransportTests.CreateAsync_ValidProcessInvalidServer_StdErrCallbackInvoked test to fail on macOS and Ubuntu CI. Reverting to keep the original TextWriter-based write path while still using the deferred logging overload to avoid unnecessary serialization when trace logging is disabled. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Fixed in 523a0cb. The |
|
@copilot that timing explanation doesn't make sense. Timing issues like that should not result in test failure, and if they do, that's an issue to be fixed. |
…f8Bytes The previous CI failure was a pre-existing flaky test (StdErrCallbackInvoked), not caused by this change. The test's stderr callback mechanism (process.BeginErrorReadLine in StdioClientTransport) is independent of how stdin writes are performed. Changes: - Remove TextWriter-based constructor and StreamWriter wrapping - Store raw Stream for writes, use SerializeToUtf8Bytes + direct stream writes - Keep TextReader wrapping for reads (needed for ReadLineAsync) Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
You're right — the CI failure ( |
Use a static readonly MediaTypeHeaderValue instance shared between McpHttpClient and ClientOAuthProvider instead of creating a new instance on every request. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
cc: @ericstj |
Validate that both StreamServerTransport and StreamClientSessionTransport use \n (not \r\n) as the message delimiter on all platforms, and that both correctly accept \n-delimited messages on the read side. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Head branch was pushed to by a user without write access
tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs
Outdated
Show resolved
Hide resolved
Per review feedback, the LF read tests were redundant since we now always emit LF. Replace them with CRLF acceptance tests that validate backward compatibility with \r\n-delimited messages, skipped on non-Windows platforms. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
tests/ModelContextProtocol.Tests/Transport/StreamClientTransportTests.cs
Outdated
Show resolved
Hide resolved
Per review feedback, consolidate stream client transport tests into StdioClientTransportTests.cs for consistency with how StreamServerTransport tests live in StdioServerTransportTests.cs. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
tests/ModelContextProtocol.Tests/Transport/StdioClientTransportTests.cs
Outdated
Show resolved
Hide resolved
TextReader.ReadLineAsync handles \r\n on all platforms, so there's no reason to restrict these tests to Windows. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Several transport paths serialize JSON to a
string, then re-encode to UTF-8 bytes. This eliminates those intermediate allocations by going directly to UTF-8.StreamServerTransport.SendMessageAsync: ReplaceJsonSerializer.Serialize+Encoding.UTF8.GetByteswithJsonSerializer.SerializeToUtf8Bytesto write directly to the output streamStreamClientSessionTransport.SendMessageAsync: RemoveStreamWriterindirection — useSerializeToUtf8Bytesand write bytes directly to the underlyingStream(same pattern asStreamServerTransport)McpHttpClient.CreatePostBodyContent(netstandard2.0 path): ReplaceJsonSerializer.Serialize+new StringContent(...)withSerializeToUtf8Bytes+ByteArrayContentClientOAuthProvider: ReplaceJsonSerializer.Serialize+new StringContent(...)withSerializeToUtf8Bytes+ByteArrayContentMediaTypeHeaderValue: Add a shared staticMediaTypeHeaderValueinstance forapplication/json; charset=utf-8used by bothMcpHttpClientandClientOAuthProvider, avoiding per-request allocations\n(not\r\n) write delimiter for both stream transports, plus\r\nread acceptance tests on all platformsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.