Fix low priority issues (#7413)

Thanks @svetkereMS for bringing this up, driving, and testing.

This fixes two interconnected issues.
First, if a process starts at normal priority then changes to low priority, it stays at normal priority. That's good for Visual Studio, which should stay at normal priority, but we relied on passing priority from a parent process to children, which is no longer valid. This ensures that we set the priority of a process early enough that we get the desired priority in worker nodes as well.

Second, if we were already connected to normal priority worker nodes, we could keep using them. This "shuts down" (disconnects—they may keep running if nodeReuse is true) worker nodes when the priority changes between build submissions.

One non-issue (therefore not fixed) is connecting to task hosts that are low priority. Tasks host nodes currently do not store their priority or node reuse. Node reuse makes sense because it's automatically off always for task hosts, at least currently. Not storing low priority sounds problematic, but it's actually fine because we make a task host—the right priority for this build, since we just made it—and connect to it. If we make a new build with different priority, we disconnect from all nodes, including task hosts. Since nodeReuse is always false, the task host dies, and we cannot reconnect to it even though if it didn't immediately die, we could, erroneously.

On the other hand, we went a little further and didn't even specify that task hosts should take the priority assigned to them as a command line argument. That has been changed.

svetkereMS had a chance to test some of this. He raised a couple potential issues:

conhost.exe launches as normal priority. Maybe some custom task dlls or other (Mef?) extensions will do something between MSBuild start time and when its priority is adjusted.
Some vulnerability if MSBuild init code improperly accounts for timing
For (1), how is conhost.exe related to MSBuild? It sounds like a command prompt thing. I don't know what Mef is.
For (2), what vulnerability? Too many processes starting and connecting to task hosts with different priorities simultaneously? I could imagine that being a problem but don't think it's worth worrying about unless someone complains.

He also mentioned a potential optimization if the main node stays at normal priority. Rather than making a new set of nodes, the main node could change the priority of all its nodes to the desired priority. Then it can skip the handshake, and if it's still at normal priority, it may be able to both raise and lower the priority of its children. Since there would never be more than 2x the "right" number of nodes anyway, and I don't think people will be switching rapidly back and forth, I think maybe we should file that as an issue in the backlog and get to it if we have time but not worry about it right now.

Edit:
I changed "shuts down...worker nodes when the priority changes" to just changing their priority. This does not work on linux or mac. However, Visual Studio does not run on linux or mac, and VS is the only currently known customer that runs in normal priority but may change between using worker nodes at normal priority or low priority. This approach is substantially more efficient than starting new nodes for every switch, disconnecting and reconnecting, or even maintaining two separate pools for different builds.

documentation/specs/low-priority-switch.md

@@ -0,0 +1,35 @@
+
+# Low Priority Nodes in MSBuild and Visual Studio
+
+## Problem Summary
+
+When doing other work, it can be useful for builds (which often take a long time and consume a lot of resources) to happen in the background, allowing other work to happen in the interim. This is true for both command line builds and builds within Visual Studio.
+
+Visual Studio, on the other hand, should always run at normal priority. This ensures that users can continue to interact with its other features, most notably editing code and seeing intellisense and autocomplete pop up.
+
+## High Level Design
+
+### Requirements
+
+1. A long-lived process can execute a series of builds divided between Normal and BelowNormal priority.
+2. Transitions between a build at Normal priority and one at BelowNormal priority (and vice versa) are fairly efficient, at least on Windows but ideally on all operating systems.
+3. NodeReuse is still possible. That is, another process can (often) use nodes from the long-lived process if NodeReuse is true.
+4. Any reused nodes are at the priority they themselves specify. Normal priority nodes are actually at normal priority, and low priority nodes are actually at BelowNormal priority.
+5. All nodes are at the priority they should be when being used to build even if a normal priority process had connected to them as normal priority worker nodes, and they are now executing a low priority build.
+
+
+## Non-goals
+
+Perfect parity between windows and mac or linux. Windows permits processes to raise their own priority or that of another process, whereas other operating systems do not. This is very efficient, so we should use it. As we expect this feature to be used in Visual Studio, we anticipate it being less used on mac and linux, hence not being as high priority to make it just as efficient.
+
+## Details
+
+Each node (including worker nodes) initially takes its priority from its parent process. Since we now need the priority to align with what it is passed instead of its parent, attempt to adjust priority afterwards if necessary as part of node startup.
+
+BuildManager.cs remembers the priority of the previous build it had executed. If that was set to a value that differs from the priority of the current build:
+1. On windows or when decreasing the priority: lowers the priority of all connected nodes
+2. On linux and mac when increasing the priority: disconnects from all nodes.
+
+When a worker node disconnects from the entrypoint node, it should ensure that it is the priority that would be expected by nodes that successfully connect to it. That means that it should be normal priority if lowPriority is false and BelowNormal priority otherwise.
+
+For this reason, if we intend to reuse a node, we check its priority and adjust it to the expected value if possible. If it proves impossible to adjust to the correct priority, the node shuts down.

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -34,6 +34,7 @@ using ForwardingLoggerRecord = Microsoft.Build.Logging.ForwardingLoggerRecord;
 using LoggerDescription = Microsoft.Build.Logging.LoggerDescription;
 
 using Microsoft.NET.StringTools;
+using System.ComponentModel;
 
 #nullable disable
 
@@ -176,6 +177,11 @@ namespace Microsoft.Build.Execution
         /// </summary>
         private int _nextBuildSubmissionId;
 
+        /// <summary>
+        /// The last BuildParameters used for building.
+        /// </summary>
+        private bool? _previousLowPriority = null;
+
         /// <summary>
         /// Mapping of unnamed project instances to the file names assigned to them.
         /// </summary>
@@ -404,6 +410,15 @@ namespace Microsoft.Build.Execution
             _deferredBuildMessages = null;
         }
 
+        private void UpdatePriority(Process p, ProcessPriorityClass priority)
+        {
+            try
+            {
+                p.PriorityClass = priority;
+            }
+            catch (Win32Exception) { }
+        }
+
         /// <summary>
         /// Prepares the BuildManager to receive build requests.
         /// </summary>
@@ -411,6 +426,41 @@ namespace Microsoft.Build.Execution
         /// <exception cref="InvalidOperationException">Thrown if a build is already in progress.</exception>
         public void BeginBuild(BuildParameters parameters)
         {
+            if (_previousLowPriority != null)
+            {
+                if (parameters.LowPriority != _previousLowPriority)
+                {
+                    if (NativeMethodsShared.IsWindows || parameters.LowPriority)
+                    {
+                        ProcessPriorityClass priority = parameters.LowPriority ? ProcessPriorityClass.BelowNormal : ProcessPriorityClass.Normal;
+                        IEnumerable<Process> processes = _nodeManager?.GetProcesses();
+                        if (processes is not null)
+                        {
+                            foreach (Process p in processes)
+                            {
+                                UpdatePriority(p, priority);
+                            }
+                        }
+
+                        processes = _taskHostNodeManager?.GetProcesses();
+                        if (processes is not null)
+                        {
+                            foreach (Process p in processes)
+                            {
+                                UpdatePriority(p, priority);
+                            }
+                        }
+                    }
+                    else
+                    {
+                        _nodeManager?.ShutdownAllNodes();
+                        _taskHostNodeManager?.ShutdownAllNodes();
+                    }
+               }
+            }
+
+            _previousLowPriority = parameters.LowPriority;
+
             lock (_syncLock)
             {
                 AttachDebugger();

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

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System.Collections.Generic;
+using System.Diagnostics;
 using Microsoft.Build.Execution;
 
 #nullable disable
@@ -51,6 +53,8 @@ namespace Microsoft.Build.BackEnd
         /// The node manager contains state which is not supposed to persist between builds, make sure this is cleared.
         /// </summary>
         void ClearPerBuildState();
+
+        IEnumerable<Process> GetProcesses();
         #endregion
     }
 }

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

@@ -3,6 +3,10 @@
 
 #nullable disable
 
+using System.Collections;
+using System.Collections.Generic;
+using System.Diagnostics;
+
 namespace Microsoft.Build.BackEnd
 {
     /// <summary>
@@ -83,6 +87,8 @@ namespace Microsoft.Build.BackEnd
         /// Shuts down all of the managed nodes.  This call will not return until all nodes are shut down.
         /// </summary>
         void ShutdownAllNodes();
+
+        IEnumerable<Process> GetProcesses();
         #endregion
     }
 }

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

@@ -24,6 +24,8 @@ namespace Microsoft.Build.BackEnd
 
         private readonly bool _lowPriority;
 
+        internal bool LowPriority { get { return _lowPriority; } }
+
         #endregion
 
         #region Constructors and Factories

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

@@ -5,6 +5,8 @@ using System;
 using System.Collections.Generic;
 using Microsoft.Build.Shared;
 using Microsoft.Build.Execution;
+using System.Diagnostics;
+using System.Linq;
 
 #nullable disable
 
@@ -347,5 +349,10 @@ namespace Microsoft.Build.BackEnd
             _nodeIdToProvider.Add(nodeId, nodeProvider);
             return nodeId;
         }
+
+        public IEnumerable<Process> GetProcesses()
+        {
+            return _outOfProcNodeProvider.GetProcesses();
+        }
     }
 }

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

@@ -2,6 +2,8 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Collections.Generic;
+using System.Diagnostics;
 using System.Globalization;
 using System.Threading;
 using Microsoft.Build.Internal;
@@ -439,6 +441,9 @@ namespace Microsoft.Build.BackEnd
             }
         }
 
+        // The process here is the same as in the main node.
+        public IEnumerable<Process> GetProcesses() => throw new NotImplementedException();
+
         #endregion
     }
 }

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

@@ -8,7 +8,7 @@ using Microsoft.Build.Shared;
 using Microsoft.Build.Exceptions;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Internal;
-using Microsoft.Build.Utilities;
+using System.Linq;
 
 #nullable disable
 
@@ -202,5 +202,10 @@ namespace Microsoft.Build.BackEnd
                 _nodeContexts.Remove(nodeId);
             }
         }
+
+        public IEnumerable<Process> GetProcesses()
+        {
+            return _nodeContexts.Values.Select(context => context.Process);
+        }
     }
 }

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

@@ -665,6 +665,8 @@ namespace Microsoft.Build.BackEnd
             /// </summary>
             private readonly Process _process;
 
+            internal Process Process { get { return _process; } }
+
             /// <summary>
             /// An array used to store the header byte for each packet when read.
             /// </summary>

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

@@ -9,6 +9,7 @@ using System.Threading;
 
 using Microsoft.Build.Shared;
 using Microsoft.Build.Internal;
+using System.Linq;
 
 #nullable disable
 
@@ -537,7 +538,7 @@ namespace Microsoft.Build.BackEnd
 
             // Start the new process.  We pass in a node mode with a node number of 2, to indicate that we
             // want to start up an MSBuild task host node.
-            string commandLineArgs = $" /nologo /nodemode:2 /nodereuse:{ComponentHost.BuildParameters.EnableNodeReuse} ";
+            string commandLineArgs = $" /nologo /nodemode:2 /nodereuse:{ComponentHost.BuildParameters.EnableNodeReuse} /low:{ComponentHost.BuildParameters.LowPriority} ";
 
             string msbuildLocation = GetMSBuildLocationFromHostContext(hostContext);
 
@@ -600,5 +601,10 @@ namespace Microsoft.Build.BackEnd
                 }
             }
         }
+
+        public IEnumerable<Process> GetProcesses()
+        {
+            return _nodeContexts.Values.Select(context => context.Process);
+        }
     }
 }

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

@@ -4,6 +4,8 @@
 using System;
 using Microsoft.Build.Shared;
 using Microsoft.Build.Execution;
+using System.Collections.Generic;
+using System.Diagnostics;
 
 #nullable disable
 
@@ -171,5 +173,10 @@ namespace Microsoft.Build.BackEnd
             ErrorUtilities.VerifyThrow(type == BuildComponentType.TaskHostNodeManager, "Cannot create component of type {0}", type);
             return new TaskHostNodeManager();
         }
+
+        IEnumerable<Process> INodeManager.GetProcesses()
+        {
+            return _outOfProcTaskHostNodeProvider.GetProcesses();
+        }
     }
 }

src/Build/BackEnd/Node/OutOfProcNode.cs

@@ -17,6 +17,7 @@ using Microsoft.Build.Internal;
 using Microsoft.Build.BackEnd.Components.Caching;
 using Microsoft.Build.BackEnd.SdkResolution;
 using SdkResult = Microsoft.Build.BackEnd.SdkResolution.SdkResult;
+using System.Diagnostics;
 
 #nullable disable
 
@@ -809,6 +810,32 @@ namespace Microsoft.Build.Execution
         private void HandleNodeBuildComplete(NodeBuildComplete buildComplete)
         {
             _shutdownReason = buildComplete.PrepareForReuse ? NodeEngineShutdownReason.BuildCompleteReuse : NodeEngineShutdownReason.BuildComplete;
+            if (_shutdownReason == NodeEngineShutdownReason.BuildCompleteReuse)
+            {
+                ProcessPriorityClass priorityClass = Process.GetCurrentProcess().PriorityClass;
+                if (priorityClass != ProcessPriorityClass.Normal && priorityClass != ProcessPriorityClass.BelowNormal)
+                {
+                    // This isn't a priority class known by MSBuild. We should avoid connecting to this node.
+                    _shutdownReason = NodeEngineShutdownReason.BuildComplete;
+                }
+                else
+                {
+                    bool lowPriority = priorityClass == ProcessPriorityClass.BelowNormal;
+                    if (_nodeEndpoint.LowPriority != lowPriority)
+                    {
+                        if (!lowPriority || NativeMethodsShared.IsWindows)
+                        {
+                            Process.GetCurrentProcess().PriorityClass = lowPriority ? ProcessPriorityClass.Normal : ProcessPriorityClass.BelowNormal;
+                        }
+                        else
+                        {
+                            // On *nix, we can't adjust the priority up, so to avoid using this node at the wrong priority, we should not be reused.
+                            _shutdownReason = NodeEngineShutdownReason.BuildComplete;
+                        }
+                    }
+                }
+            }
+            
             _shutdownEvent.Set();
         }
     }

src/MSBuild/XMake.cs

@@ -629,20 +629,6 @@ namespace Microsoft.Build.CommandLine
                         Environment.SetEnvironmentVariable("MSBUILDLOADALLFILESASWRITEABLE", "1");
                     }
 
-                    // Honor the low priority flag, we place our selves below normal priority and let sub processes inherit
-                    // that priority. Idle priority would prevent the build from proceeding as the user does normal actions.
-                    try
-                    {
-                        if (lowPriority && Process.GetCurrentProcess().PriorityClass != ProcessPriorityClass.Idle)
-                        {
-                            Process.GetCurrentProcess().PriorityClass = ProcessPriorityClass.BelowNormal;
-                        }
-                    }
-                    // We avoid increasing priority because that causes failures on mac/linux, but there is no good way to
-                    // verify that a particular priority is lower than "BelowNormal." If the error appears, ignore it and
-                    // leave priority where it was.
-                    catch (Win32Exception) { }
-
                     DateTime t1 = DateTime.Now;
 
                     // If the primary file passed to MSBuild is a .binlog file, play it back into passed loggers
@@ -2091,6 +2077,27 @@ namespace Microsoft.Build.CommandLine
                 DisplayCopyrightMessage();
             }
 
+
+            // Idle priority would prevent the build from proceeding as the user does normal actions.
+            // This switch is processed early to capture both the command line case (main node should
+            // also be low priority) and the Visual Studio case in which the main node starts and stays
+            // at normal priority (not through XMake.cs) but worker nodes still need to honor this switch.
+            if (commandLineSwitches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.LowPriority))
+            {
+                lowPriority = ProcessBooleanSwitch(commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.LowPriority], defaultValue: true, resourceName: "InvalidLowPriorityValue");
+            }
+            try
+            {
+                if (lowPriority && Process.GetCurrentProcess().PriorityClass != ProcessPriorityClass.Idle)
+                {
+                    Process.GetCurrentProcess().PriorityClass = ProcessPriorityClass.BelowNormal;
+                }
+            }
+            // We avoid increasing priority because that causes failures on mac/linux, but there is no good way to
+            // verify that a particular priority is lower than "BelowNormal." If the error appears, ignore it and
+            // leave priority where it was.
+            catch (Win32Exception) { }
+
             // if help switch is set (regardless of switch errors), show the help message and ignore the other switches
             if (commandLineSwitches[CommandLineSwitches.ParameterlessSwitch.Help])
             {
@@ -2098,7 +2105,7 @@ namespace Microsoft.Build.CommandLine
             }
             else if (commandLineSwitches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.NodeMode))
             {
-                StartLocalNode(commandLineSwitches);
+                StartLocalNode(commandLineSwitches, lowPriority);
             }
             else
             {
@@ -2236,11 +2243,6 @@ namespace Microsoft.Build.CommandLine
                         graphBuild = ProcessGraphBuildSwitch(commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.GraphBuild]);
                     }
 
-                    if (commandLineSwitches.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.LowPriority))
-                    {
-                        lowPriority = ProcessBooleanSwitch(commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.LowPriority], defaultValue: true, resourceName: "InvalidLowPriorityValue");
-                    }
-
                     inputResultsCaches = ProcessInputResultsCaches(commandLineSwitches);
 
                     outputResultsCache = ProcessOutputResultsCache(commandLineSwitches);
@@ -2580,7 +2582,7 @@ namespace Microsoft.Build.CommandLine
         /// Uses the input from thinNodeMode switch to start a local node server
         /// </summary>
         /// <param name="commandLineSwitches"></param>
-        private static void StartLocalNode(CommandLineSwitches commandLineSwitches)
+        private static void StartLocalNode(CommandLineSwitches commandLineSwitches, bool lowpriority)
         {
             string[] input = commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.NodeMode];
             int nodeModeNumber = 0;
@@ -2608,22 +2610,22 @@ namespace Microsoft.Build.CommandLine
             {
                 Exception nodeException = null;
                 NodeEngineShutdownReason shutdownReason = NodeEngineShutdownReason.Error;
+
                 // normal OOP node case
                 if (nodeModeNumber == 1)
                 {
-                    OutOfProcNode node = new OutOfProcNode();
-
                     // If FEATURE_NODE_REUSE is OFF, just validates that the switch is OK, and always returns False
                     bool nodeReuse = ProcessNodeReuseSwitch(commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.NodeReuse]);
-                    string[] lowPriorityInput = commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.LowPriority];
-                    bool lowpriority = lowPriorityInput.Length > 0 && lowPriorityInput[0].Equals("true");
-
+                    OutOfProcNode node = new OutOfProcNode();
                     shutdownReason = node.Run(nodeReuse, lowpriority, out nodeException);
 
                     FileUtilities.ClearCacheDirectory();
                 }
                 else if (nodeModeNumber == 2)
                 {
+                    // TaskHost nodes don't need to worry about node reuse or low priority. Node reuse is always off, and TaskHosts
+                    // receive a connection immediately after being launched and shut down as soon as their work is over, so
+                    // whatever our priority is is correct.
                     OutOfProcTaskHostNode node = new OutOfProcTaskHostNode();
                     shutdownReason = node.Run(out nodeException);
                 }

src/MSBuildTaskHost/OutOfProcTaskHost.cs

@@ -68,7 +68,7 @@ namespace Microsoft.Build.CommandLine
         [MTAThread]
         public static int Main()
         {
-            int exitCode = (Execute() == ExitType.Success ? 0 : 1);
+            int exitCode = Execute() == ExitType.Success ? 0 : 1;
             return exitCode;
         }