Avoid process output/error redirection completely for MTP on protocol version 1.1.0

Author: Youssef1313

src/Cli/dotnet/Commands/Test/CliConstants.cs

@@ -60,9 +60,9 @@ internal static class HandshakeMessagePropertyNames
 internal static class ProtocolConstants
 {
     /// <summary>
-    /// 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.
     /// </summary>
-    internal const string SupportedVersions = "1.0.0";
+    internal const string[] SupportedVersions = ["1.1.0", "1.0.0"];
 }
 
 internal static class ProjectProperties

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)
         {

src/Cli/dotnet/Commands/Test/MTP/TestApplication.cs

@@ -54,51 +54,61 @@ public async Task<int> 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<string>();
-            var stdErrBuilder = new ConcurrentQueue<string>();
-
-            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<string>();
+                var stdErrBuilder = new ConcurrentQueue<string>();
+
+                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,15 +131,16 @@ public async Task<int> RunAsync()
 
     private ProcessStartInfo CreateProcessStartInfo()
     {
+        var shouldRedirect = _handler.ShouldRedirectOutputAndError;
         var processStartInfo = new ProcessStartInfo
         {
             // We should get correct RunProperties right away.
             // For the case of dotnet test --test-modules path/to/dll, the TestModulesFilterHandler is responsible
             // 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<IResponse> 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)
     {

src/Cli/dotnet/Commands/Test/MTP/TestApplicationHandler.cs

@@ -14,7 +14,7 @@ internal sealed class TestApplicationHandler
     private readonly Lock _lock = new();
     private readonly Dictionary<string, (int TestSessionStartCount, int TestSessionEndCount)> _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)
         {