Read std err before waiting to avoid deadlock (#4875)

* Read std err before waiting to avoid deadlock

* Document that we will wait forever

Author: andreasohlund

src/ServiceControlInstaller.Engine.UnitTests/Setup/SetupInstanceTests.cs

@@ -9,33 +9,22 @@
 public class SetupInstanceTests
 {
     [Test]
-    public void Should_not_throw_on_0_exit_code() => Assert.DoesNotThrow(() => InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", "", Timeout.Infinite));
+    public void Should_not_throw_on_0_exit_code() => Assert.DoesNotThrow(() => InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", ""));
 
     [Test]
     public void Should_capture_and_rethrow_failures()
     {
-        var ex = Assert.Throws<Exception>(() => InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", "fail", Timeout.Infinite));
+        var ex = Assert.Throws<Exception>(() => InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", "fail"));
 
         Assert.That(ex.Message, Does.Contain("Fake exception"));
     }
 
     [Test]
     public void Should_capture_and_rethrow_non_zero_exit_codes()
     {
-        var ex = Assert.Throws<Exception>(() => InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", "non-zero-exit-code", Timeout.Infinite));
+        var ex = Assert.Throws<Exception>(() => InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", "non-zero-exit-code"));
 
         Assert.That(ex.Message, Does.Contain("returned a non-zero exit code: 3"));
         Assert.That(ex.Message, Does.Contain("Fake non zero exit code message"));
     }
-
-    [Test]
-    public void Should_not_kill_the_process_if_wait_time_is_execeeded()
-    {
-        var process = InstanceSetup.Run(TestContext.CurrentContext.WorkDirectory, "SetupProcessFake.exe", "test", "delay", 10);
-
-        Assert.That(process.HasExited, Is.False);
-
-        process.Kill();
-        process.WaitForExit();
-    }
 }

src/ServiceControlInstaller.Engine/Setup/InstanceSetup.cs

@@ -35,12 +35,10 @@ static void Run(string installPath, string exeName, string instanceName, bool sk
             args += " --skip-queue-creation";
         }
 
-        // we will wait "forever" since that is the most safe action right not. We will leave it up to the setup code in the instances to make sure it won't run forever.
-        // If/when provide better UI experience we might revisit this and potentially find a way for the installer to control the timeout.
-        Run(installPath, exeName, instanceName, args, Timeout.Infinite);
+        Run(installPath, exeName, instanceName, args);
     }
 
-    internal static Process Run(string installPath, string exeName, string instanceName, string args, int timeout)
+    internal static void Run(string installPath, string exeName, string instanceName, string args)
     {
         var processStartupInfo = new ProcessStartInfo
         {
@@ -56,15 +54,20 @@ internal static Process Run(string installPath, string exeName, string instanceN
 
         var p = Process.Start(processStartupInfo) ?? throw new Exception($"Attempt to launch {exeName} failed.");
 
-        p.WaitForExit(timeout);
+        // Reading std err needs to happen before waiting to avoid the risk of a deadlock.
+        // See https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.process.standardoutput?view=net-9.0&redirectedfrom=MSDN#remarks for more details.
+        // Note that this will wait forever, should we want to avoid that the async ProcessStartInfo.ErrorDataReceived API needs to be used.
+        var error = p.StandardError.ReadToEnd();
+
+        // we will wait "forever" since that is the most safe action right not. We will leave it up to the setup code in the instances to make sure it won't run forever.
+        // If/when provide better UI experience we might revisit this and potentially find a way for the installer to control the timeout.
+        p.WaitForExit();
 
         if (!p.HasExited || p.ExitCode == 0)
         {
-            return p;
+            return;
         }
 
-        var error = p.StandardError.ReadToEnd();
-
         throw new Exception($"{exeName} returned a non-zero exit code: {p.ExitCode}. This typically indicates a configuration error. The error output was:\r\n {error}");
     }
 }

src/SetupProcessFake/Program.cs

@@ -10,11 +10,4 @@
     return 3;
 }
 
-if (args.Any(a => a == "delay"))
-{
-#pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task
-    await Task.Delay(TimeSpan.FromSeconds(5));
-#pragma warning restore CA2007 // Consider calling ConfigureAwait on the awaited task
-}
-
 return 0;