Fix dotnet test hang when the test app process exits but a child process of it remains alive

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

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Concurrent;
 using System.Diagnostics;
 using System.Globalization;
 using System.IO.Pipes;
@@ -56,14 +57,47 @@ public async Task<int> RunAsync()
             // 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.
-            var stdOutTask = Task.Factory.StartNew(static standardOutput => ((StreamReader)standardOutput!).ReadToEnd(), process.StandardOutput, TaskCreationOptions.LongRunning);
-            var stdErrTask = Task.Factory.StartNew(static standardError => ((StreamReader)standardError!).ReadToEnd(), process.StandardError, TaskCreationOptions.LongRunning);
+            // 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 outputAndError = await Task.WhenAll(stdOutTask, stdErrTask);
+            var stdOutTask = Task.Factory.StartNew(() =>
+            {
+                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)
+                {
+                    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)
+            {
+            }
+
             var exitCode = process.ExitCode;
-            _handler.OnTestProcessExited(exitCode, outputAndError[0], outputAndError[1]);
+            _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.

test/Microsoft.NET.TestFramework/Commands/SdkCommandSpec.cs

@@ -19,6 +19,8 @@ public class SdkCommandSpec
 
         public bool RedirectStandardInput { get; set; }
 
+        public bool DisableOutputAndErrorRedirection { get; set; }
+
         private string EscapeArgs()
         {
             //  Note: this doesn't handle invoking .cmd files via "cmd /c" on Windows, which probably won't be necessary here

test/Microsoft.NET.TestFramework/Commands/TestCommand.cs

@@ -22,6 +22,8 @@ public abstract class TestCommand
 
         public bool RedirectStandardInput { get; set; }
 
+        public bool DisableOutputAndErrorRedirection { get; set; }
+
         //  These only work via Execute(), not when using GetProcessStartInfo()
         public Action<string>? CommandOutputHandler { get; set; }
         public Action<Process>? ProcessStartedHandler { get; set; }
@@ -47,6 +49,12 @@ public TestCommand WithWorkingDirectory(string workingDirectory)
             return this;
         }
 
+        public TestCommand WithDisableOutputAndErrorRedirection()
+        {
+            DisableOutputAndErrorRedirection = true;
+            return this;
+        }
+
         public TestCommand WithStandardInput(string stdin)
         {
             Debug.Assert(ProcessStartedHandler == null);
@@ -107,6 +115,7 @@ private SdkCommandSpec CreateCommandSpec(IEnumerable<string> args)
             }
 
             commandSpec.RedirectStandardInput = RedirectStandardInput;
+            commandSpec.DisableOutputAndErrorRedirection = DisableOutputAndErrorRedirection;
 
             return commandSpec;
         }
@@ -147,24 +156,27 @@ public virtual CommandResult Execute(IEnumerable<string> args)
             var spec = CreateCommandSpec(args);
 
             var command = spec
-                .ToCommand(_doNotEscapeArguments)
-                .CaptureStdOut()
-                .CaptureStdErr();
+                .ToCommand(_doNotEscapeArguments);
 
-            command.OnOutputLine(line =>
+            if (!spec.DisableOutputAndErrorRedirection)
             {
-                Log.WriteLine($"》{line}");
-                CommandOutputHandler?.Invoke(line);
-            });
-
-            command.OnErrorLine(line =>
-            {
-                Log.WriteLine($"❌{line}");
-            });
-
-            if (StandardOutputEncoding is not null)
-            {
-                command.StandardOutputEncoding(StandardOutputEncoding);
+                command
+                    .CaptureStdOut()
+                    .CaptureStdErr()
+                    .OnOutputLine(line =>
+                     {
+                         Log.WriteLine($"》{line}");
+                         CommandOutputHandler?.Invoke(line);
+                     })
+                    .OnErrorLine(line =>
+                    {
+                        Log.WriteLine($"❌{line}");
+                    });
+
+                if (StandardOutputEncoding is not null)
+                {
+                    command.StandardOutputEncoding(StandardOutputEncoding);
+                }
             }
 
             string fileToShow = Path.GetFileNameWithoutExtension(spec.FileName!).Equals("dotnet", StringComparison.OrdinalIgnoreCase) ?
@@ -173,7 +185,7 @@ public virtual CommandResult Execute(IEnumerable<string> args)
             var display = $"{fileToShow} {string.Join(" ", spec.Arguments)}";
 
             Log.WriteLine($"Executing '{display}':");
-            var result = ((Command)command).Execute(ProcessStartedHandler);
+            var result = command.Execute(ProcessStartedHandler);
             Log.WriteLine($"Command '{display}' exited with exit code {result.ExitCode}.");
 
             if (Environment.GetEnvironmentVariable("HELIX_WORKITEM_UPLOAD_ROOT") is string uploadRoot)

test/TestAssets/TestProjects/MTPChildProcessHangTest/MTPChildProcessHangTest.csproj

@@ -0,0 +1,17 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), testAsset.props))\testAsset.props" />
+
+	<PropertyGroup>
+		<TargetFramework>$(CurrentTargetFramework)</TargetFramework>
+		<OutputType>Exe</OutputType>
+
+		<ImplicitUsings>enable</ImplicitUsings>
+		<Nullable>enable</Nullable>
+
+		<ManagePackageVersionsCentrally>false</ManagePackageVersionsCentrally>
+	</PropertyGroup>
+
+	<ItemGroup>
+		<PackageReference Include="Microsoft.Testing.Platform" Version="$(MicrosoftTestingPlatformVersion)" />
+	</ItemGroup>
+</Project>

test/TestAssets/TestProjects/MTPChildProcessHangTest/Program.cs

@@ -0,0 +1,47 @@
+using System.Diagnostics;
+using Microsoft.Testing.Platform.Builder;
+using Microsoft.Testing.Platform.Capabilities.TestFramework;
+using Microsoft.Testing.Platform.Extensions.TestFramework;
+
+if (args.Length == 1 && args[0] == "hang")
+{
+    var @event = new ManualResetEvent(false);
+    @event.WaitOne();
+    return 0;
+}
+
+var builder = await TestApplication.CreateBuilderAsync(args);
+builder.RegisterTestFramework(_ => new TestFrameworkCapabilities(), (_, _) => new MyTestFramework());
+using var testApp = await builder.BuildAsync();
+return await testApp.RunAsync();
+
+internal class MyTestFramework : ITestFramework
+{
+    public string Uid => nameof(MyTestFramework);
+    public string Version => "1.0.0";
+    public string DisplayName => nameof(MyTestFramework);
+    public string Description => DisplayName;
+    public Task<CloseTestSessionResult> CloseTestSessionAsync(CloseTestSessionContext context)
+    {
+        return Task.FromResult(new CloseTestSessionResult() { IsSuccess = true });
+    }
+    public Task<CreateTestSessionResult> CreateTestSessionAsync(CreateTestSessionContext context)
+        => Task.FromResult(new CreateTestSessionResult() { IsSuccess = true });
+    public Task ExecuteRequestAsync(ExecuteRequestContext context)
+    {
+        var fileName = Process.GetCurrentProcess().MainModule.FileName;
+        var p = Process.Start(new ProcessStartInfo(fileName, "hang")
+        {
+            RedirectStandardOutput = true,
+            RedirectStandardError = true,
+        });
+        p.BeginOutputReadLine();
+        p.BeginErrorReadLine();
+        p.ErrorDataReceived += (sender, e) => { };
+        p.OutputDataReceived += (sender, e) => { };
+        context.Complete();
+        return Task.CompletedTask;
+    }
+    public Task<bool> IsEnabledAsync()
+        => Task.FromResult(true);
+}

test/TestAssets/TestProjects/MTPChildProcessHangTest/global.json

@@ -0,0 +1,5 @@
+{
+    "test": {
+        "runner": "Microsoft.Testing.Platform"
+    }
+}

test/dotnet.Tests/CommandTests/Test/GivenDotnetTestBuildsAndRunsTests.cs

@@ -588,5 +588,23 @@ public void RunTestProjectWithEnvVariable(string configuration)
 
             result.ExitCode.Should().Be(ExitCodes.AtLeastOneTestFailed);
         }
+
+        [InlineData(TestingConstants.Debug)]
+        [InlineData(TestingConstants.Release)]
+        [Theory]
+        public void DotnetTest_MTPChildProcessHangTestProject_ShouldNotHang(string configuration)
+        {
+            var testInstance = TestAssetsManager.CopyTestAsset("MTPChildProcessHangTest", Guid.NewGuid().ToString())
+                .WithSource();
+
+            var result = new DotnetTestCommand(Log, disableNewOutput: false)
+                .WithWorkingDirectory(testInstance.Path)
+                // We need to disable output and error redirection so that the test infra doesn't
+                // hang because of a hanging child process that keeps out/err open.
+                .WithDisableOutputAndErrorRedirection()
+                .Execute("-c", configuration);
+
+            result.ExitCode.Should().Be(ExitCodes.ZeroTests);
+        }
     }
 }