When sharing the terminal with child nodes, wait for the children to terminate before exiting ourselves.

scripts/Deploy-MSBuild.ps1

@@ -48,7 +48,7 @@ Write-Host "Existing MSBuild assemblies backed up to $BackupFolder"
 if ($runtime -eq "Desktop") {
     $targetFramework = "net472"
 } else {
-    $targetFramework = "netcoreapp2.1"
+    $targetFramework = "net5.0"
 }
 
 $bootstrapBinDirectory = "artifacts\bin\MSBuild.Bootstrap\$configuration\$targetFramework"
@@ -74,11 +74,11 @@ $filesToCopyToBin = @(
     FileToCopy "$bootstrapBinDirectory\Microsoft.Managed.targets"
     FileToCopy "$bootstrapBinDirectory\Microsoft.Managed.Before.targets"
     FileToCopy "$bootstrapBinDirectory\Microsoft.Managed.After.targets"
-    FileToCopy "$bootstrapBinDirectory\Microsoft.Net.props"
-    FileToCopy "$bootstrapBinDirectory\Microsoft.NetFramework.CurrentVersion.props"
-    FileToCopy "$bootstrapBinDirectory\Microsoft.NetFramework.CurrentVersion.targets"
-    FileToCopy "$bootstrapBinDirectory\Microsoft.NetFramework.props"
-    FileToCopy "$bootstrapBinDirectory\Microsoft.NetFramework.targets"
+    FileToCopy "$bootstrapBinDirectory\Microsoft.NET.props"
+    FileToCopy "$bootstrapBinDirectory\Microsoft.NETFramework.CurrentVersion.props"
+    FileToCopy "$bootstrapBinDirectory\Microsoft.NETFramework.CurrentVersion.targets"
+    FileToCopy "$bootstrapBinDirectory\Microsoft.NETFramework.props"
+    FileToCopy "$bootstrapBinDirectory\Microsoft.NETFramework.targets"
     FileToCopy "$bootstrapBinDirectory\Microsoft.VisualBasic.CrossTargeting.targets"
     FileToCopy "$bootstrapBinDirectory\Microsoft.VisualBasic.CurrentVersion.targets"
     FileToCopy "$bootstrapBinDirectory\Microsoft.VisualBasic.targets"

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -629,7 +629,7 @@ void Callback(object state)
                         }
                     }
 
-                    ShutdownConnectedNodesAsync(true /* abort */);
+                    ShutdownConnectedNodes(true /* abort */);
                     CheckForActiveNodesAndCleanUpSubmissions();
                 }
             }
@@ -774,7 +774,7 @@ public void EndBuild()
             try
             {
                 _noActiveSubmissionsEvent.WaitOne();
-                ShutdownConnectedNodesAsync(false /* normal termination */);
+                ShutdownConnectedNodes(false /* normal termination */);
                 _noNodesActiveEvent.WaitOne();
 
                 // Wait for all of the actions in the work queue to drain.  Wait() could throw here if there was an unhandled exception
@@ -1937,7 +1937,7 @@ public void Dispose()
         /// Asks the nodeManager to tell the currently connected nodes to shut down and sets a flag preventing all non-shutdown-related packets from
         /// being processed.
         /// </summary>
-        private void ShutdownConnectedNodesAsync(bool abort)
+        private void ShutdownConnectedNodes(bool abort)
         {
             _shuttingDown = true;
 

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs

@@ -26,6 +26,8 @@
 using Microsoft.Build.Utilities;
 
 using BackendNativeMethods = Microsoft.Build.BackEnd.NativeMethods;
+using Task = System.Threading.Tasks.Task;
+using DotNetFrameworkArchitecture = Microsoft.Build.Shared.DotNetFrameworkArchitecture;
 
 namespace Microsoft.Build.BackEnd
 {
@@ -50,6 +52,11 @@ internal abstract class NodeProviderOutOfProcBase
         /// </summary>
         private const int TimeoutForNewNodeCreation = 30000;
 
+        /// <summary>
+        /// The amount of time to wait for an out-of-proc node to exit.
+        /// </summary>
+        private const int TimeoutForWaitForExit = 30000;
+
         /// <summary>
         /// The build component host.
         /// </summary>
@@ -96,9 +103,29 @@ protected void ShutdownConnectedNodes(List<NodeContext> contextsToShutDown, bool
             // Send the build completion message to the nodes, causing them to shutdown or reset.
             _processesToIgnore.Clear();
 
+            // We wait for child nodes to exit to avoid them changing the terminal
+            // after this process terminates.
+            bool waitForExit =  !enableReuse &&
+                                !Console.IsInputRedirected &&
+                                Traits.Instance.EscapeHatches.EnsureStdOutForChildNodesIsPrimaryStdout;
+
+            Task[] waitForExitTasks = waitForExit && contextsToShutDown.Count > 0 ? new Task[contextsToShutDown.Count] : null;
+            int i = 0;
             foreach (NodeContext nodeContext in contextsToShutDown)
             {
-                nodeContext?.SendData(new NodeBuildComplete(enableReuse));
+                if (nodeContext is null)
+                {
+                    continue;
+                }
+                nodeContext.SendData(new NodeBuildComplete(enableReuse));
+                if (waitForExit)
+                {
+                    waitForExitTasks[i++] = nodeContext.WaitForExitAsync();
+                }
+            }
+            if (waitForExitTasks != null)
+            {
+                Task.WaitAll(waitForExitTasks);
             }
         }
 
@@ -139,7 +166,7 @@ protected void ShutdownAllNodes(bool nodeReuse, NodeContextTerminateDelegate ter
                 {
                     // If we're able to connect to such a process, send a packet requesting its termination
                     CommunicationsUtilities.Trace("Shutting down node with pid = {0}", nodeProcess.Id);
-                    NodeContext nodeContext = new NodeContext(0, nodeProcess.Id, nodeStream, factory, terminateNode);
+                    NodeContext nodeContext = new NodeContext(0, nodeProcess, nodeStream, factory, terminateNode);
                     nodeContext.SendData(new NodeBuildComplete(false /* no node reuse */));
                     nodeStream.Dispose();
                 }
@@ -205,7 +232,7 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in
                     {
                         // Connection successful, use this node.
                         CommunicationsUtilities.Trace("Successfully connected to existed node {0} which is PID {1}", nodeId, nodeProcess.Id);
-                        return new NodeContext(nodeId, nodeProcess.Id, nodeStream, factory, terminateNode);
+                        return new NodeContext(nodeId, nodeProcess, nodeStream, factory, terminateNode);
                     }
                 }
             }
@@ -243,20 +270,20 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in
 #endif
 
                 // Create the node process
-                int msbuildProcessId = LaunchNode(msbuildLocation, commandLineArgs);
-                _processesToIgnore.Add(GetProcessesToIgnoreKey(hostHandshake, msbuildProcessId));
+                Process msbuildProcess = LaunchNode(msbuildLocation, commandLineArgs);
+                _processesToIgnore.Add(GetProcessesToIgnoreKey(hostHandshake, msbuildProcess.Id));
 
                 // Note, when running under IMAGEFILEEXECUTIONOPTIONS registry key to debug, the process ID
                 // gotten back from CreateProcess is that of the debugger, which causes this to try to connect
                 // to the debugger process. Instead, use MSBUILDDEBUGONSTART=1
 
                 // Now try to connect to it.
-                Stream nodeStream = TryConnectToProcess(msbuildProcessId, TimeoutForNewNodeCreation, hostHandshake);
+                Stream nodeStream = TryConnectToProcess(msbuildProcess.Id, TimeoutForNewNodeCreation, hostHandshake);
                 if (nodeStream != null)
                 {
                     // Connection successful, use this node.
-                    CommunicationsUtilities.Trace("Successfully connected to created node {0} which is PID {1}", nodeId, msbuildProcessId);
-                    return new NodeContext(nodeId, msbuildProcessId, nodeStream, factory, terminateNode);
+                    CommunicationsUtilities.Trace("Successfully connected to created node {0} which is PID {1}", nodeId, msbuildProcess.Id);
+                    return new NodeContext(nodeId, msbuildProcess, nodeStream, factory, terminateNode);
                 }
             }
 
@@ -392,7 +419,7 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han
         /// <summary>
         /// Creates a new MSBuild process
         /// </summary>
-        private int LaunchNode(string msbuildLocation, string commandLineArgs)
+        private Process LaunchNode(string msbuildLocation, string commandLineArgs)
         {
             // Should always have been set already.
             ErrorUtilities.VerifyThrowInternalLength(msbuildLocation, nameof(msbuildLocation));
@@ -491,7 +518,7 @@ private int LaunchNode(string msbuildLocation, string commandLineArgs)
                 }
 
                 CommunicationsUtilities.Trace("Successfully launched {1} node with PID {0}", process.Id, exeName);
-                return process.Id;
+                return process;
             }
             else
             {
@@ -549,7 +576,7 @@ out processInfo
                 }
 
                 CommunicationsUtilities.Trace("Successfully launched {1} node with PID {0}", childProcessId, exeName);
-                return childProcessId;
+                return Process.GetProcessById(childProcessId);
             }
         }
 
@@ -598,9 +625,9 @@ internal class NodeContext
             private int _nodeId;
 
             /// <summary>
-            /// The process id
+            /// The node process.
             /// </summary>
-            private int _processId;
+            private readonly Process _process;
 
             /// <summary>
             /// An array used to store the header byte for each packet when read.
@@ -613,14 +640,14 @@ internal class NodeContext
             private byte[] _smallReadBuffer;
 
             /// <summary>
-            /// Event indicating the node has terminated.
+            /// Delegate called when the context terminates.
             /// </summary>
-            private ManualResetEvent _nodeTerminated;
+            private NodeContextTerminateDelegate _terminateDelegate;
 
             /// <summary>
-            /// Delegate called when the context terminates.
+            /// Node was requested to terminate.
             /// </summary>
-            private NodeContextTerminateDelegate _terminateDelegate;
+            private bool _isExiting;
 
             /// <summary>
             /// Per node read buffers
@@ -630,18 +657,17 @@ internal class NodeContext
             /// <summary>
             /// Constructor.
             /// </summary>
-            public NodeContext(int nodeId, int processId,
+            public NodeContext(int nodeId, Process process,
                 Stream nodePipe,
                 INodePacketFactory factory, NodeContextTerminateDelegate terminateDelegate)
             {
                 _nodeId = nodeId;
-                _processId = processId;
+                _process = process;
                 _clientToServerStream = nodePipe;
                 _serverToClientStream = nodePipe;
                 _packetFactory = factory;
                 _headerByte = new byte[5]; // 1 for the packet type, 4 for the body length
                 _smallReadBuffer = new byte[1000]; // 1000 was just an average seen on one profile run.
-                _nodeTerminated = new ManualResetEvent(false);
                 _terminateDelegate = terminateDelegate;
                 _sharedReadBuffer = InterningBinaryReader.CreateSharedBuffer();
             }
@@ -761,7 +787,7 @@ public void SendData(INodePacket packet)
 #else
                             _serverToClientStream.WriteAsync(writeStreamBuffer, i, lengthToWrite);
 #endif
-                            return;
+                            break;
                         }
                         else
                         {
@@ -777,6 +803,7 @@ public void SendData(INodePacket packet)
 #endif
                         }
                     }
+                    _isExiting = packet is NodeBuildComplete buildCompletePacket && !buildCompletePacket.PrepareForReuse;
                 }
                 catch (IOException e)
                 {
@@ -792,7 +819,7 @@ public void SendData(INodePacket packet)
             /// <summary>
             /// Closes the node's context, disconnecting it from the node.
             /// </summary>
-            public void Close()
+            private void Close()
             {
                 _clientToServerStream.Dispose();
                 if (!object.ReferenceEquals(_clientToServerStream, _serverToClientStream))
@@ -802,6 +829,40 @@ public void Close()
                 _terminateDelegate(_nodeId);
             }
 
+            /// <summary>
+            /// Waits for the child node process to exit.
+            /// </summary>
+            public async Task WaitForExitAsync()
+            {
+                // Wait for the process to exit.
+                if (_isExiting)
+                {
+                    CommunicationsUtilities.Trace("Waiting for node with pid = {0} to exit", _process.Id);
+
+                    // .NET 5 introduces a real WaitForExitAsyc.
+                    // This is a poor man's implementation that uses polling.
+                    int timeout = TimeoutForWaitForExit;
+                    int delay = 5;
+                    while (timeout > 0)
+                    {
+                        bool exited = _process.WaitForExit(milliseconds: 0);
+                        if (exited)
+                        {
+                            return;
+                        }
+                        timeout -= delay;
+                        await Task.Delay(delay).ConfigureAwait(false);
+
+                        // Double delay up to 500ms.
+                        delay = Math.Min(delay * 2, 500);
+                    }
+                }
+
+                // Kill the child and do a blocking wait.
+                CommunicationsUtilities.Trace("Killing node with pid = {0}", _process.Id);
+                _process.KillTree(timeout: -1);
+            }
+
 #if FEATURE_APM
             /// <summary>
             /// Completes the asynchronous packet write to the node.
@@ -823,17 +884,16 @@ private bool ProcessHeaderBytesRead(int bytesRead)
             {
                 if (bytesRead != _headerByte.Length)
                 {
-                    CommunicationsUtilities.Trace(_nodeId, "COMMUNICATIONS ERROR (HRC) Node: {0} Process: {1} Bytes Read: {2} Expected: {3}", _nodeId, _processId, bytesRead, _headerByte.Length);
+                    CommunicationsUtilities.Trace(_nodeId, "COMMUNICATIONS ERROR (HRC) Node: {0} Process: {1} Bytes Read: {2} Expected: {3}", _nodeId, _process.Id, bytesRead, _headerByte.Length);
                     try
                     {
-                        Process childProcess = Process.GetProcessById(_processId);
-                        if (childProcess?.HasExited != false)
+                        if (_process.HasExited)
                         {
-                            CommunicationsUtilities.Trace(_nodeId, "   Child Process {0} has exited.", _processId);
+                            CommunicationsUtilities.Trace(_nodeId, "   Child Process {0} has exited.", _process.Id);
                         }
                         else
                         {
-                            CommunicationsUtilities.Trace(_nodeId, "   Child Process {0} is still running.", _processId);
+                            CommunicationsUtilities.Trace(_nodeId, "   Child Process {0} is still running.", _process.Id);
                         }
                     }
                     catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))

src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs

@@ -24,6 +24,7 @@
 using Microsoft.Build.Utilities;
 
 using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;
+using Task = System.Threading.Tasks.Task;
 
 namespace Microsoft.Build.BackEnd
 {

src/Build/Microsoft.Build.csproj

@@ -128,6 +128,7 @@
     <Compile Include="..\Shared\NodeBuildComplete.cs" />
     <Compile Include="..\Shared\NodeShutdown.cs" />
     <Compile Include="..\Shared\NamedPipeUtil.cs" />
+    <Compile Include="..\Shared\ProcessExtensions.cs" />
     <Compile Include="..\Shared\TaskLoggingHelper.cs">
       <Link>BackEnd\Components\RequestBuilder\IntrinsicTasks\TaskLoggingHelper.cs</Link>
       <ExcludeFromStyleCop>True</ExcludeFromStyleCop>

src/Utilities/ProcessExtensions.cs → src/Shared/ProcessExtensions.cs

@@ -7,7 +7,7 @@
 using System.IO;
 using Microsoft.Build.Shared;
 
-namespace Microsoft.Build.Utilities
+namespace Microsoft.Build.Shared
 {
     internal static class ProcessExtensions
     {
@@ -77,10 +77,21 @@ private static void GetAllChildIdsUnix(int parentId, ISet<int> children)
 
         private static void KillProcessUnix(int processId)
         {
-            RunProcessAndWaitForExit(
-                "kill",
-                $"-TERM {processId}",
-                out string _);
+            try
+            {
+                using Process process = Process.GetProcessById(processId);
+                process.Kill();
+            }
+            catch (ArgumentException)
+            {
+                // Process already terminated.
+                return;
+            }
+            catch (InvalidOperationException)
+            {
+                // Process already terminated.
+                return;
+            }
         }
 
         private static int RunProcessAndWaitForExit(string fileName, string arguments, out string stdout)
@@ -102,8 +113,13 @@ private static int RunProcessAndWaitForExit(string fileName, string arguments, o
             }
             else
             {
-                process.Kill();
-
+                try
+                {
+                    process.Kill();
+                }
+                catch (InvalidOperationException)
+                { }
+
                 // Kill is asynchronous so we should still wait a little
                 //
                 process.WaitForExit((int) TimeSpan.FromSeconds(1).TotalMilliseconds);