From acc7282c99f5d9ffa193682c1efd1a4989106f04 Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Tue, 17 Mar 2026 13:03:28 +0100 Subject: [PATCH 1/2] Avoid process output/error redirection completely for MTP on protocol version 1.1.0 --- src/Cli/dotnet/Commands/Test/CliConstants.cs | 4 +- .../Test/MTP/Terminal/TerminalTestReporter.cs | 2 +- .../Commands/Test/MTP/TestApplication.cs | 112 ++++++++++-------- .../Test/MTP/TestApplicationHandler.cs | 26 ++-- 4 files changed, 80 insertions(+), 64 deletions(-) diff --git a/src/Cli/dotnet/Commands/Test/CliConstants.cs b/src/Cli/dotnet/Commands/Test/CliConstants.cs index 9fb46e3a91ff..9e04272229b7 100644 --- a/src/Cli/dotnet/Commands/Test/CliConstants.cs +++ b/src/Cli/dotnet/Commands/Test/CliConstants.cs @@ -60,9 +60,9 @@ internal static class HandshakeMessagePropertyNames internal static class ProtocolConstants { /// - /// The protocol versions that are supported by the current SDK. Multiple versions can be present and be semicolon separated. + /// The protocol versions that are supported by the current SDK. Must be ordered from highest version to lowest version. /// - internal const string SupportedVersions = "1.0.0"; + internal static readonly string[] SupportedVersions = ["1.1.0", "1.0.0"]; } internal static class ProjectProperties diff --git a/src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs b/src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs index ca8ea35e2d57..d0dd95057842 100644 --- a/src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs +++ b/src/Cli/dotnet/Commands/Test/MTP/Terminal/TerminalTestReporter.cs @@ -753,7 +753,7 @@ internal void AssemblyRunCompleted(string executionId, }); } - internal void HandshakeFailure(string assemblyPath, string? targetFramework, int exitCode, string outputData, string errorData) + internal void HandshakeFailure(string assemblyPath, string? targetFramework, int exitCode, string? outputData, string? errorData) { if (_isHelp) { diff --git a/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs b/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs index d5d733a17044..eb248b30f7f4 100644 --- a/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs +++ b/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs @@ -54,51 +54,61 @@ public async Task RunAsync() using var process = Process.Start(processStartInfo)!; - // Reading from process stdout/stderr is done on separate threads to avoid blocking IO on the threadpool. - // Note: even with 'process.StandardOutput.ReadToEndAsync()' or 'process.BeginOutputReadLine()', we ended up with - // many TP threads just doing synchronous IO, slowing down the progress of the test run. - // We want to read requests coming through the pipe and sending responses back to the test app as fast as possible. - // We are using ConcurrentQueue to avoid thread-safety issues for the timeout case. - // In the timeout case, we leave stdOutTask and stdErrTask running, just we stop observing them. - var stdOutBuilder = new ConcurrentQueue(); - var stdErrBuilder = new ConcurrentQueue(); - - var stdOutTask = Task.Factory.StartNew(() => + int exitCode; + if (!_handler.ShouldRedirectOutputAndError) { - var stdOut = process.StandardOutput; - string? currentLine; - while ((currentLine = stdOut.ReadLine()) is not null) + await process.WaitForExitAsync(); + exitCode = process.ExitCode; + _handler.OnTestProcessExited(exitCode, null, null); + } + else + { + // Reading from process stdout/stderr is done on separate threads to avoid blocking IO on the threadpool. + // Note: even with 'process.StandardOutput.ReadToEndAsync()' or 'process.BeginOutputReadLine()', we ended up with + // many TP threads just doing synchronous IO, slowing down the progress of the test run. + // We want to read requests coming through the pipe and sending responses back to the test app as fast as possible. + // We are using ConcurrentQueue to avoid thread-safety issues for the timeout case. + // In the timeout case, we leave stdOutTask and stdErrTask running, just we stop observing them. + var stdOutBuilder = new ConcurrentQueue(); + var stdErrBuilder = new ConcurrentQueue(); + + var stdOutTask = Task.Factory.StartNew(() => { - stdOutBuilder.Enqueue(currentLine); - } - }, TaskCreationOptions.LongRunning); + var stdOut = process.StandardOutput; + string? currentLine; + while ((currentLine = stdOut.ReadLine()) is not null) + { + stdOutBuilder.Enqueue(currentLine); + } + }, TaskCreationOptions.LongRunning); - var stdErrTask = Task.Factory.StartNew(() => - { - var stdErr = process.StandardError; - string? currentLine; - while ((currentLine = stdErr.ReadLine()) is not null) + var stdErrTask = Task.Factory.StartNew(() => { - stdErrBuilder.Enqueue(currentLine); - } - }, TaskCreationOptions.LongRunning); + var stdErr = process.StandardError; + string? currentLine; + while ((currentLine = stdErr.ReadLine()) is not null) + { + stdErrBuilder.Enqueue(currentLine); + } + }, TaskCreationOptions.LongRunning); - // WaitForExitAsync only waits for process exit (and doesn't wait for output) for our usage here. - // If we use BeginOutputReadLine/BeginErrorReadLine, it will also wait for output which can deadlock. - await process.WaitForExitAsync(); + // WaitForExitAsync only waits for process exit (and doesn't wait for output) for our usage here. + // If we use BeginOutputReadLine/BeginErrorReadLine, it will also wait for output which can deadlock. + await process.WaitForExitAsync(); - // At this point, process already exited. Allow for 5 seconds to consume stdout/stderr. - // We might not be able to consume all the output if the test app has exited but left a child process alive. - try - { - await Task.WhenAll(stdOutTask, stdErrTask).WaitAsync(TimeSpan.FromSeconds(5)); - } - catch (TimeoutException) - { - } + // At this point, process already exited. Allow for 5 seconds to consume stdout/stderr. + // We might not be able to consume all the output if the test app has exited but left a child process alive. + try + { + await Task.WhenAll(stdOutTask, stdErrTask).WaitAsync(TimeSpan.FromSeconds(5)); + } + catch (TimeoutException) + { + } - var exitCode = process.ExitCode; - _handler.OnTestProcessExited(exitCode, string.Join(Environment.NewLine, stdOutBuilder), string.Join(Environment.NewLine, stdErrBuilder)); + exitCode = process.ExitCode; + _handler.OnTestProcessExited(exitCode, string.Join(Environment.NewLine, stdOutBuilder), string.Join(Environment.NewLine, stdErrBuilder)); + } // This condition is to prevent considering the test app as successful when we didn't receive test session end. // We don't produce the exception if the exit code is already non-zero to avoid surfacing this exception when there is already a known failure. @@ -121,6 +131,7 @@ public async Task RunAsync() private ProcessStartInfo CreateProcessStartInfo() { + var shouldRedirect = _handler.ShouldRedirectOutputAndError; var processStartInfo = new ProcessStartInfo { // We should get correct RunProperties right away. @@ -128,8 +139,8 @@ private ProcessStartInfo CreateProcessStartInfo() // for providing the dotnet muxer as RunCommand, and `exec "path/to/dll"` as RunArguments. FileName = Module.RunProperties.Command, Arguments = GetArguments(), - RedirectStandardOutput = true, - RedirectStandardError = true, + RedirectStandardOutput = shouldRedirect, + RedirectStandardError = shouldRedirect, // False is already the default on .NET Core, but prefer to be explicit. UseShellExecute = false, }; @@ -254,7 +265,7 @@ private Task OnRequest(NamedPipeServer server, IRequest request) case HandshakeMessage handshakeMessage: _handshakes.Add(server, handshakeMessage); string negotiatedVersion = GetSupportedProtocolVersion(handshakeMessage); - OnHandshakeMessage(handshakeMessage, negotiatedVersion.Length > 0); + OnHandshakeMessage(handshakeMessage, negotiatedVersion); return Task.FromResult((IResponse)CreateHandshakeMessage(negotiatedVersion)); case CommandLineOptionMessages commandLineOptionMessages: @@ -317,14 +328,15 @@ private static string GetSupportedProtocolVersion(HandshakeMessage handshakeMess return string.Empty; } - // NOTE: Today, ProtocolConstants.Version is only 1.0.0 (i.e, SDK supports only a single version). - // Whenever we support multiple versions in SDK, we should do intersection - // between protocolVersions given by MTP, and the versions supported by SDK. - // Then we return the "highest" version from the intersection. - // The current logic **assumes** that ProtocolConstants.SupportedVersions is a single version. - if (protocolVersions.Split(";").Contains(ProtocolConstants.SupportedVersions)) + // ProtocolConstant.SupportedVersions is the SDK supported versions, ordered from highest version to lowest version. + // So, we take the first highest version that is also supported by MTP. + var protocolVersionsByMTP = protocolVersions.Split(";"); + foreach (var sdkSupportedVersion in ProtocolConstants.SupportedVersions) { - return ProtocolConstants.SupportedVersions; + if (protocolVersionsByMTP.Contains(sdkSupportedVersion)) + { + return sdkSupportedVersion; + } } // The version given by MTP is not supported by SDK. @@ -341,8 +353,8 @@ private static HandshakeMessage CreateHandshakeMessage(string version) => { HandshakeMessagePropertyNames.SupportedProtocolVersions, version } }); - public void OnHandshakeMessage(HandshakeMessage handshakeMessage, bool gotSupportedVersion) - => _handler.OnHandshakeReceived(handshakeMessage, gotSupportedVersion); + public void OnHandshakeMessage(HandshakeMessage handshakeMessage, string negotiatedVersion) + => _handler.OnHandshakeReceived(handshakeMessage, negotiatedVersion); private void OnCommandLineOptionMessages(CommandLineOptionMessages commandLineOptionMessages) { diff --git a/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs b/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs index 9735d0ab72b2..0c69856899f1 100644 --- a/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs +++ b/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs @@ -14,7 +14,7 @@ internal sealed class TestApplicationHandler private readonly Lock _lock = new(); private readonly Dictionary _testSessionEventCountPerSessionUid = new(); - private (string? TargetFramework, string? Architecture, string ExecutionId)? _handshakeInfo; + private (string? TargetFramework, string? Architecture, string ExecutionId, string NegotiatedVersion)? _handshakeInfo; private bool _receivedTestHostHandshake; public TestApplicationHandler(TerminalTestReporter output, TestModule module, TestOptions options) @@ -24,11 +24,14 @@ public TestApplicationHandler(TerminalTestReporter output, TestModule module, Te _options = options; } - internal void OnHandshakeReceived(HandshakeMessage handshakeMessage, bool gotSupportedVersion) + // Only 1.0.0 specifically should redirect output/err. + internal bool ShouldRedirectOutputAndError => !_handshakeInfo.HasValue || _handshakeInfo.Value.NegotiatedVersion == "1.0.0"; + + internal void OnHandshakeReceived(HandshakeMessage handshakeMessage, string negotiatedVersion) { LogHandshake(handshakeMessage); - if (!gotSupportedVersion) + if (negotiatedVersion.Length == 0) { _output.HandshakeFailure( _module.TargetPath, @@ -37,7 +40,7 @@ internal void OnHandshakeReceived(HandshakeMessage handshakeMessage, bool gotSup string.Format( CliCommandStrings.DotnetTestIncompatibleHandshakeVersion, handshakeMessage.Properties[HandshakeMessagePropertyNames.SupportedProtocolVersions], - ProtocolConstants.SupportedVersions), + string.Join(';', ProtocolConstants.SupportedVersions)), string.Empty); // Protocol version is not supported. @@ -48,7 +51,7 @@ internal void OnHandshakeReceived(HandshakeMessage handshakeMessage, bool gotSup var executionId = handshakeMessage.Properties[HandshakeMessagePropertyNames.ExecutionId]; var arch = handshakeMessage.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower(); var tfm = TargetFrameworkParser.GetShortTargetFramework(handshakeMessage.Properties[HandshakeMessagePropertyNames.Framework]); - var currentHandshakeInfo = (tfm, arch, executionId); + var currentHandshakeInfo = (tfm, arch, executionId, negotiatedVersion); if (!_handshakeInfo.HasValue) { @@ -137,6 +140,7 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage) } var handshakeInfo = _handshakeInfo.Value; + var shouldRedirect = ShouldRedirectOutputAndError; foreach (var testResult in testResultMessage.SuccessfulTestMessages) { _output.TestCompleted(_module.TargetPath, handshakeInfo.TargetFramework, handshakeInfo.Architecture, handshakeInfo.ExecutionId, @@ -149,8 +153,8 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage) exceptions: null, expected: null, actual: null, - standardOutput: testResult.StandardOutput, - errorOutput: testResult.ErrorOutput); + standardOutput: shouldRedirect ? testResult.StandardOutput : null, + errorOutput: shouldRedirect ? testResult.ErrorOutput : null); } foreach (var testResult in testResultMessage.FailedTestMessages) @@ -164,8 +168,8 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage) exceptions: [.. testResult.Exceptions!.Select(fe => new Terminal.FlatException(fe.ErrorMessage, fe.ErrorType, fe.StackTrace))], expected: null, actual: null, - standardOutput: testResult.StandardOutput, - errorOutput: testResult.ErrorOutput); + standardOutput: shouldRedirect ? testResult.StandardOutput : null, + errorOutput: shouldRedirect ? testResult.ErrorOutput : null); } } @@ -263,7 +267,7 @@ internal bool HasMismatchingTestSessionEventCount() return false; } - internal void OnTestProcessExited(int exitCode, string outputData, string errorData) + internal void OnTestProcessExited(int exitCode, string? outputData, string? errorData) { if (_receivedTestHostHandshake && _handshakeInfo.HasValue) { @@ -378,7 +382,7 @@ private static void LogFileArtifacts(FileArtifactMessages fileArtifactMessages) Logger.LogTrace(logMessageBuilder, static logMessageBuilder => logMessageBuilder.ToString()); } - private static void LogTestProcessExit(int exitCode, string outputData, string errorData) + private static void LogTestProcessExit(int exitCode, string? outputData, string? errorData) { if (!Logger.TraceEnabled) { From 6086399a8d5975ee79b65f36de0b051b6824e1ec Mon Sep 17 00:00:00 2001 From: Youssef1313 Date: Wed, 18 Mar 2026 13:59:59 +0100 Subject: [PATCH 2/2] Try different approach --- .../Commands/Test/MTP/TestApplication.cs | 93 ++++++++++--------- .../Test/MTP/TestApplicationHandler.cs | 15 +-- 2 files changed, 57 insertions(+), 51 deletions(-) diff --git a/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs b/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs index eb248b30f7f4..b7df12f3e915 100644 --- a/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs +++ b/src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs @@ -55,60 +55,66 @@ public async Task RunAsync() using var process = Process.Start(processStartInfo)!; int exitCode; - if (!_handler.ShouldRedirectOutputAndError) - { - await process.WaitForExitAsync(); - exitCode = process.ExitCode; - _handler.OnTestProcessExited(exitCode, null, null); - } - else + + // Reading from process stdout/stderr is done on separate threads to avoid blocking IO on the threadpool. + // Note: even with 'process.StandardOutput.ReadToEndAsync()' or 'process.BeginOutputReadLine()', we ended up with + // many TP threads just doing synchronous IO, slowing down the progress of the test run. + // We want to read requests coming through the pipe and sending responses back to the test app as fast as possible. + // We are using ConcurrentQueue to avoid thread-safety issues for the timeout case. + // In the timeout case, we leave stdOutTask and stdErrTask running, just we stop observing them. + var stdOutBuilder = new ConcurrentQueue(); + var stdErrBuilder = new ConcurrentQueue(); + + var stdOutTask = Task.Factory.StartNew(() => { - // Reading from process stdout/stderr is done on separate threads to avoid blocking IO on the threadpool. - // Note: even with 'process.StandardOutput.ReadToEndAsync()' or 'process.BeginOutputReadLine()', we ended up with - // many TP threads just doing synchronous IO, slowing down the progress of the test run. - // We want to read requests coming through the pipe and sending responses back to the test app as fast as possible. - // We are using ConcurrentQueue to avoid thread-safety issues for the timeout case. - // In the timeout case, we leave stdOutTask and stdErrTask running, just we stop observing them. - var stdOutBuilder = new ConcurrentQueue(); - var stdErrBuilder = new ConcurrentQueue(); - - var stdOutTask = Task.Factory.StartNew(() => + var stdOut = process.StandardOutput; + string? currentLine; + while ((currentLine = stdOut.ReadLine()) is not null) { - var stdOut = process.StandardOutput; - string? currentLine; - while ((currentLine = stdOut.ReadLine()) is not null) + if (_handler.ShouldHideOutputAndError) { stdOutBuilder.Enqueue(currentLine); } - }, TaskCreationOptions.LongRunning); + else + { + Console.WriteLine(currentLine); + } + } + }, TaskCreationOptions.LongRunning); - var stdErrTask = Task.Factory.StartNew(() => + var stdErrTask = Task.Factory.StartNew(() => + { + var stdErr = process.StandardError; + string? currentLine; + while ((currentLine = stdErr.ReadLine()) is not null) { - var stdErr = process.StandardError; - string? currentLine; - while ((currentLine = stdErr.ReadLine()) is not null) + if (_handler.ShouldHideOutputAndError) { stdErrBuilder.Enqueue(currentLine); } - }, TaskCreationOptions.LongRunning); - - // WaitForExitAsync only waits for process exit (and doesn't wait for output) for our usage here. - // If we use BeginOutputReadLine/BeginErrorReadLine, it will also wait for output which can deadlock. - await process.WaitForExitAsync(); - - // At this point, process already exited. Allow for 5 seconds to consume stdout/stderr. - // We might not be able to consume all the output if the test app has exited but left a child process alive. - try - { - await Task.WhenAll(stdOutTask, stdErrTask).WaitAsync(TimeSpan.FromSeconds(5)); - } - catch (TimeoutException) - { + else + { + Console.WriteLine(currentLine); + } } + }, TaskCreationOptions.LongRunning); + + // WaitForExitAsync only waits for process exit (and doesn't wait for output) for our usage here. + // If we use BeginOutputReadLine/BeginErrorReadLine, it will also wait for output which can deadlock. + await process.WaitForExitAsync(); - exitCode = process.ExitCode; - _handler.OnTestProcessExited(exitCode, string.Join(Environment.NewLine, stdOutBuilder), string.Join(Environment.NewLine, stdErrBuilder)); + // At this point, process already exited. Allow for 5 seconds to consume stdout/stderr. + // We might not be able to consume all the output if the test app has exited but left a child process alive. + try + { + await Task.WhenAll(stdOutTask, stdErrTask).WaitAsync(TimeSpan.FromSeconds(5)); } + catch (TimeoutException) + { + } + + exitCode = process.ExitCode; + _handler.OnTestProcessExited(exitCode, string.Join(Environment.NewLine, stdOutBuilder), string.Join(Environment.NewLine, stdErrBuilder)); // This condition is to prevent considering the test app as successful when we didn't receive test session end. // We don't produce the exception if the exit code is already non-zero to avoid surfacing this exception when there is already a known failure. @@ -131,7 +137,6 @@ public async Task RunAsync() private ProcessStartInfo CreateProcessStartInfo() { - var shouldRedirect = _handler.ShouldRedirectOutputAndError; var processStartInfo = new ProcessStartInfo { // We should get correct RunProperties right away. @@ -139,8 +144,8 @@ private ProcessStartInfo CreateProcessStartInfo() // for providing the dotnet muxer as RunCommand, and `exec "path/to/dll"` as RunArguments. FileName = Module.RunProperties.Command, Arguments = GetArguments(), - RedirectStandardOutput = shouldRedirect, - RedirectStandardError = shouldRedirect, + RedirectStandardOutput = true, + RedirectStandardError = true, // False is already the default on .NET Core, but prefer to be explicit. UseShellExecute = false, }; diff --git a/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs b/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs index 0c69856899f1..c35dae3166c1 100644 --- a/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs +++ b/src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs @@ -24,8 +24,9 @@ public TestApplicationHandler(TerminalTestReporter output, TestModule module, Te _options = options; } - // Only 1.0.0 specifically should redirect output/err. - internal bool ShouldRedirectOutputAndError => !_handshakeInfo.HasValue || _handshakeInfo.Value.NegotiatedVersion == "1.0.0"; + // Only 1.0.0 hides the output. + // Otherwise, TerminalTestReporter of the test app will interfere with the output of the test command. + internal bool ShouldHideOutputAndError => _handshakeInfo.HasValue && _handshakeInfo.Value.NegotiatedVersion == "1.0.0"; internal void OnHandshakeReceived(HandshakeMessage handshakeMessage, string negotiatedVersion) { @@ -140,7 +141,7 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage) } var handshakeInfo = _handshakeInfo.Value; - var shouldRedirect = ShouldRedirectOutputAndError; + var shouldHide = ShouldHideOutputAndError; foreach (var testResult in testResultMessage.SuccessfulTestMessages) { _output.TestCompleted(_module.TargetPath, handshakeInfo.TargetFramework, handshakeInfo.Architecture, handshakeInfo.ExecutionId, @@ -153,8 +154,8 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage) exceptions: null, expected: null, actual: null, - standardOutput: shouldRedirect ? testResult.StandardOutput : null, - errorOutput: shouldRedirect ? testResult.ErrorOutput : null); + standardOutput: shouldHide ? testResult.StandardOutput : null, + errorOutput: shouldHide ? testResult.ErrorOutput : null); } foreach (var testResult in testResultMessage.FailedTestMessages) @@ -168,8 +169,8 @@ internal void OnTestResultsReceived(TestResultMessages testResultMessage) exceptions: [.. testResult.Exceptions!.Select(fe => new Terminal.FlatException(fe.ErrorMessage, fe.ErrorType, fe.StackTrace))], expected: null, actual: null, - standardOutput: shouldRedirect ? testResult.StandardOutput : null, - errorOutput: shouldRedirect ? testResult.ErrorOutput : null); + standardOutput: shouldHide ? testResult.StandardOutput : null, + errorOutput: shouldHide ? testResult.ErrorOutput : null); } }