-
Notifications
You must be signed in to change notification settings - Fork 1.5k
When sharing the terminal with child nodes, wait for the children to terminate before exiting ourselves. #6053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
6ed2049
412452e
812a0c9
c6e5870
d0091f4
cdefa6c
5d26c34
f2c3945
83524bf
2cc7253
8d4ecbc
f14d6a2
888068a
12d14e7
0ce5d23
f2722df
f863718
1b0177c
6f67a09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 +101,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 && | ||
|
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; | ||
|
tmds marked this conversation as resolved.
Outdated
|
||
| 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) | ||
|
tmds marked this conversation as resolved.
|
||
| { | ||
| waitForExitTasks[i++] = nodeContext.WaitForExitAsync(); | ||
|
tmds marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| if (waitForExitTasks != null) | ||
| { | ||
| Task.WaitAll(waitForExitTasks); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -613,14 +638,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 _closeSent; | ||
|
|
||
| /// <summary> | ||
| /// Per node read buffers | ||
|
|
@@ -641,7 +666,6 @@ public NodeContext(int nodeId, int processId, | |
| _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(); | ||
| } | ||
|
|
@@ -777,6 +801,7 @@ public void SendData(INodePacket packet) | |
| #endif | ||
| } | ||
| } | ||
| _closeSent = packet is NodeBuildComplete buildCompletePacket && !buildCompletePacket.PrepareForReuse; | ||
| } | ||
| catch (IOException e) | ||
| { | ||
|
|
@@ -792,8 +817,9 @@ public void SendData(INodePacket packet) | |
| /// <summary> | ||
| /// Closes the node's context, disconnecting it from the node. | ||
| /// </summary> | ||
| public void Close() | ||
| private void Close() | ||
| { | ||
| _processId = -1; | ||
| _clientToServerStream.Dispose(); | ||
| if (!object.ReferenceEquals(_clientToServerStream, _serverToClientStream)) | ||
| { | ||
|
|
@@ -802,6 +828,56 @@ public void Close() | |
| _terminateDelegate(_nodeId); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Waits for the child node process to exit. | ||
| /// </summary> | ||
| public async Task WaitForExitAsync() | ||
|
tmds marked this conversation as resolved.
Outdated
|
||
| { | ||
| int processId = _processId; | ||
| if (processId != -1) | ||
| { | ||
| Process childProcess; | ||
| try | ||
| { | ||
| childProcess = Process.GetProcessById(processId); | ||
| } | ||
| catch (System.ArgumentException) | ||
| { | ||
| // The process has terminated already. | ||
| return; | ||
| } | ||
|
|
||
| // Wait for the process to terminate. | ||
| CommunicationsUtilities.Trace("Waiting for node with pid = {0} to terminate", processId); | ||
|
|
||
| if (_closeSent) | ||
| { | ||
| // .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 = childProcess.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}", processId); | ||
| childProcess.Kill(); | ||
|
tmds marked this conversation as resolved.
Outdated
|
||
| childProcess.WaitForExit(); | ||
|
tmds marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| #if FEATURE_APM | ||
| /// <summary> | ||
| /// Completes the asynchronous packet write to the node. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.