When sharing the terminal with child nodes, wait for the children to terminate before exiting ourselves. #6053
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,10 @@ | |
| using Microsoft.Build.Utilities; | ||
|
|
||
| using BackendNativeMethods = Microsoft.Build.BackEnd.NativeMethods; | ||
| using Task = System.Threading.Tasks.Task; | ||
| using DotNetFrameworkArchitecture = Microsoft.Build.Shared.DotNetFrameworkArchitecture; | ||
| using Microsoft.Build.Framework; | ||
| using Microsoft.Build.BackEnd.Logging; | ||
|
|
||
| namespace Microsoft.Build.BackEnd | ||
| { | ||
|
|
@@ -50,6 +54,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; | ||
|
Contributor
There was a problem hiding this comment. Do you think this this is the right timeout? Node creation takes longer than exiting, I would have thought.
Member
Author
There was a problem hiding this comment. It depends if we expect it to be common for nodes to fail to shut down when asked to. |
||
|
|
||
| /// <summary> | ||
| /// The build component host. | ||
| /// </summary> | ||
|
|
@@ -96,9 +105,30 @@ 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 && | ||
|
tmds marked this conversation as resolved.
|
||
| Traits.Instance.EscapeHatches.EnsureStdOutForChildNodesIsPrimaryStdout; | ||
|
Forgind marked this conversation as resolved.
|
||
|
|
||
| Task[] waitForExitTasks = waitForExit && contextsToShutDown.Count > 0 ? new Task[contextsToShutDown.Count] : null; | ||
| int i = 0; | ||
| var loggingService = _componentHost.LoggingService; | ||
| foreach (NodeContext nodeContext in contextsToShutDown) | ||
| { | ||
| if (nodeContext is null) | ||
| { | ||
| continue; | ||
| } | ||
| nodeContext.SendData(new NodeBuildComplete(enableReuse)); | ||
| if (waitForExit) | ||
|
tmds marked this conversation as resolved.
|
||
| { | ||
| waitForExitTasks[i++] = nodeContext.WaitForExitAsync(loggingService); | ||
| } | ||
| } | ||
| if (waitForExitTasks != null) | ||
| { | ||
| Task.WaitAll(waitForExitTasks); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -139,7 +169,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, nodeStream, factory, terminateNode); | ||
| nodeContext.SendData(new NodeBuildComplete(false /* no node reuse */)); | ||
| nodeStream.Dispose(); | ||
| } | ||
|
|
@@ -205,7 +235,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, nodeStream, factory, terminateNode); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -243,20 +273,20 @@ protected NodeContext GetNode(string msbuildLocation, string commandLineArgs, in | |
| #endif | ||
|
|
||
| // Create the node process | ||
| 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(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, msbuildProcess.Id); | ||
| return new NodeContext(nodeId, msbuildProcess, nodeStream, factory, terminateNode); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -392,7 +422,7 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han | |
| /// <summary> | ||
| /// Creates a new MSBuild process | ||
| /// </summary> | ||
| private Process LaunchNode(string msbuildLocation, string commandLineArgs) | ||
| { | ||
| // Should always have been set already. | ||
| ErrorUtilities.VerifyThrowInternalLength(msbuildLocation, nameof(msbuildLocation)); | ||
|
|
@@ -491,7 +521,7 @@ private int LaunchNode(string msbuildLocation, string commandLineArgs) | |
| } | ||
|
|
||
| CommunicationsUtilities.Trace("Successfully launched {1} node with PID {0}", process.Id, exeName); | ||
| return process; | ||
| } | ||
| else | ||
| { | ||
|
|
@@ -549,7 +579,7 @@ out processInfo | |
| } | ||
|
|
||
| CommunicationsUtilities.Trace("Successfully launched {1} node with PID {0}", childProcessId, exeName); | ||
| return Process.GetProcessById(childProcessId); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -598,9 +628,9 @@ internal class NodeContext | |
| private int _nodeId; | ||
|
|
||
| /// <summary> | ||
| /// The node process. | ||
| /// </summary> | ||
| private readonly Process _process; | ||
|
|
||
| /// <summary> | ||
| /// An array used to store the header byte for each packet when read. | ||
|
|
@@ -613,14 +643,14 @@ internal class NodeContext | |
| private byte[] _smallReadBuffer; | ||
|
|
||
| /// <summary> | ||
| /// Delegate called when the context terminates. | ||
| /// </summary> | ||
| private NodeContextTerminateDelegate _terminateDelegate; | ||
|
|
||
| /// <summary> | ||
| /// Node was requested to terminate. | ||
| /// </summary> | ||
| private bool _isExiting; | ||
|
|
||
| /// <summary> | ||
| /// Per node read buffers | ||
|
|
@@ -630,18 +660,17 @@ internal class NodeContext | |
| /// <summary> | ||
| /// Constructor. | ||
| /// </summary> | ||
| public NodeContext(int nodeId, Process process, | ||
| Stream nodePipe, | ||
| INodePacketFactory factory, NodeContextTerminateDelegate terminateDelegate) | ||
| { | ||
| _nodeId = nodeId; | ||
| _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. | ||
| _terminateDelegate = terminateDelegate; | ||
| _sharedReadBuffer = InterningBinaryReader.CreateSharedBuffer(); | ||
| } | ||
|
|
@@ -761,7 +790,7 @@ public void SendData(INodePacket packet) | |
| #else | ||
| _serverToClientStream.WriteAsync(writeStreamBuffer, i, lengthToWrite); | ||
| #endif | ||
| break; | ||
|
Member
There was a problem hiding this comment. Just as an FYI, this will conflict with another PR: I'm happy to resolve conflicts if needed in either PR (depending on whichever one goes first).
Member
There was a problem hiding this comment. Also keep in mind @tmds that the logic in SendData here is as follows: for large packets, we break them up in 1 MB chunks, and we write all chunks synchronously, except for the last one. The last chunk is fire-and-forget (the WriteAsync starts the write and returns). For small packets (the vast majority), the write is async and we never wait for the write to finish. In fact we see cases where the next invocation comes in and issues another write before the previous write has finished. We're just getting lucky that the framework seems to be good at serializing this. In my PR #6023 I optimize memory allocations, which required to make all the writes synchronous (so we can reuse the byte array buffer that we pass to Write). To compensate for that, we made the entire SendData method fire-and-forget. I haven't look into this PR deeply enough to decide whether one needs to await writing packets to the socket connected to the node process before any shutdown/killing happens. Before my PR 6023 it would be a bit harder to await each async write before killing or shutting down. After my PR you can just await the
Member
Author
There was a problem hiding this comment. Right, we shouldn't set Let's look at how this needs to be changed after #6023 is merged. |
||
| } | ||
| else | ||
| { | ||
|
|
@@ -777,6 +806,7 @@ public void SendData(INodePacket packet) | |
| #endif | ||
| } | ||
| } | ||
| _isExiting = packet is NodeBuildComplete buildCompletePacket && !buildCompletePacket.PrepareForReuse; | ||
| } | ||
| catch (IOException e) | ||
| { | ||
|
|
@@ -792,7 +822,7 @@ public void SendData(INodePacket packet) | |
| /// <summary> | ||
| /// Closes the node's context, disconnecting it from the node. | ||
| /// </summary> | ||
| private void Close() | ||
| { | ||
| _clientToServerStream.Dispose(); | ||
| if (!object.ReferenceEquals(_clientToServerStream, _serverToClientStream)) | ||
|
|
@@ -802,6 +832,48 @@ public void Close() | |
| _terminateDelegate(_nodeId); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Waits for the child node process to exit. | ||
| /// </summary> | ||
| public async Task WaitForExitAsync(ILoggingService loggingService) | ||
| { | ||
| // 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. | ||
| loggingService?.LogWarningFromText( | ||
| BuildEventContext.Invalid, | ||
| null, | ||
| null, | ||
| null, | ||
| BuildEventFileInfo.Empty, | ||
| $"Killing node with pid = {_process.Id}"); | ||
| CommunicationsUtilities.Trace("Killing node with pid = {0}", _process.Id); | ||
|
tmds marked this conversation as resolved.
tmds marked this conversation as resolved.
tmds marked this conversation as resolved.
|
||
|
|
||
| _process.KillTree(); | ||
| } | ||
|
|
||
| #if FEATURE_APM | ||
| /// <summary> | ||
| /// Completes the asynchronous packet write to the node. | ||
|
|
@@ -823,17 +895,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, _process.Id, bytesRead, _headerByte.Length); | ||
| try | ||
| { | ||
| if (_process.HasExited) | ||
| { | ||
| CommunicationsUtilities.Trace(_nodeId, " Child Process {0} has exited.", _process.Id); | ||
| } | ||
| else | ||
| { | ||
| CommunicationsUtilities.Trace(_nodeId, " Child Process {0} is still running.", _process.Id); | ||
| } | ||
| } | ||
| catch (Exception e) when (!ExceptionHandling.IsCriticalException(e)) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.