sidecar-taskhost-attempt-2

src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs

@@ -544,7 +544,7 @@ public void VerifyExplicitlyLaunchTaskHost()
             ITask createdTask = null;
             try
             {
-                SetupTaskFactory(null, true /* want task host */);
+                SetupTaskFactory(null, true /* want task host */, true);
 
                 createdTask = _taskFactory.CreateTaskInstance(ElementLocation.Create("MSBUILD"), null, new MockHost(), null,
 #if FEATURE_APPDOMAIN
@@ -578,7 +578,7 @@ public void VerifyExplicitlyLaunchTaskHostEvenIfParametersMatch1()
                 taskParameters.Add(XMakeAttributes.runtime, XMakeAttributes.MSBuildRuntimeValues.any);
                 taskParameters.Add(XMakeAttributes.architecture, XMakeAttributes.MSBuildArchitectureValues.any);
 
-                SetupTaskFactory(taskParameters, true /* want task host */);
+                SetupTaskFactory(taskParameters, true /* want task host */, isTaskHostFactory: true);
 
                 createdTask = _taskFactory.CreateTaskInstance(ElementLocation.Create("MSBUILD"), null, new MockHost(), null,
 #if FEATURE_APPDOMAIN
@@ -608,7 +608,7 @@ public void VerifyExplicitlyLaunchTaskHostEvenIfParametersMatch2()
             ITask createdTask = null;
             try
             {
-                SetupTaskFactory(null, true /* want task host */);
+                SetupTaskFactory(null, true /* want task host */, true);
 
                 IDictionary<string, string> taskParameters = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
                 taskParameters.Add(XMakeAttributes.runtime, XMakeAttributes.MSBuildRuntimeValues.any);
@@ -644,7 +644,7 @@ public void VerifySameFactoryCanGenerateDifferentTaskInstances()
             factoryParameters.Add(XMakeAttributes.runtime, XMakeAttributes.MSBuildRuntimeValues.any);
             factoryParameters.Add(XMakeAttributes.architecture, XMakeAttributes.MSBuildArchitectureValues.any);
 
-            SetupTaskFactory(factoryParameters, explicitlyLaunchTaskHost: false);
+            SetupTaskFactory(factoryParameters, explicitlyLaunchTaskHost: false, isTaskHostFactory: false);
 
             try
             {
@@ -695,14 +695,19 @@ public void VerifySameFactoryCanGenerateDifferentTaskInstances()
         /// Abstract out the creation of the new AssemblyTaskFactory with default task, and
         /// with some basic validation.
         /// </summary>
-        private void SetupTaskFactory(IDictionary<string, string> factoryParameters, bool explicitlyLaunchTaskHost)
+        private void SetupTaskFactory(IDictionary<string, string> factoryParameters, bool explicitlyLaunchTaskHost = false, bool isTaskHostFactory = false)
         {
             _taskFactory = new AssemblyTaskFactory();
 #if FEATURE_ASSEMBLY_LOCATION
             _loadInfo = AssemblyLoadInfo.Create(null, Assembly.GetAssembly(typeof(TaskToTestFactories)).Location);
 #else
             _loadInfo = AssemblyLoadInfo.Create(typeof(TaskToTestFactories).GetTypeInfo().Assembly.FullName, null);
 #endif
+            if (explicitlyLaunchTaskHost)
+            {
+                factoryParameters ??= new Dictionary<string, string>();
+                factoryParameters.Add(Internal.Constants.TaskHostExplicitlyRequested, "true");
+            }
             _loadedType = _taskFactory.InitializeFactory(_loadInfo, "TaskToTestFactories", new Dictionary<string, TaskPropertyInfo>(), string.Empty, factoryParameters, explicitlyLaunchTaskHost, null, ElementLocation.Create("NONE"), String.Empty);
             Assert.True(_loadedType.Assembly.Equals(_loadInfo)); // "Expected the AssemblyLoadInfo to be equal"
         }

src/Build.UnitTests/BackEnd/ProcessIdTaskSidecar.cs

@@ -0,0 +1,29 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Diagnostics;
+using Microsoft.Build.Framework;
+using Microsoft.Build.Utilities;
+#nullable disable
+
+namespace Microsoft.Build.UnitTests
+{
+    /// <summary>
+    /// This task was created for https://github.com/dotnet/msbuild/issues/3141
+    /// </summary>
+    public class ProcessIdTaskSidecar : Task
+    {
+        [Output]
+        public int Pid { get; set; }
+
+        /// <summary>
+        /// Log the id for this process.
+        /// </summary>
+        /// <returns></returns>
+        public override bool Execute()
+        {
+            Pid = Process.GetCurrentProcess().Id;
+            return true;
+        }
+    }
+}

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

@@ -26,37 +26,114 @@ public TaskHostFactory_Tests(ITestOutputHelper testOutputHelper)
             _output = testOutputHelper;
         }
 
-        [Fact]
-        public void TaskNodesDieAfterBuild()
+        [Theory]
+        [InlineData(true, false)]
+        [InlineData(false, true)]
+        [InlineData(true, true)]
+        public void TaskNodesDieAfterBuild(bool taskHostFactorySpecified, bool envVariableSpecified)
         {
             using (TestEnvironment env = TestEnvironment.Create())
             {
+
+                string taskFactory = taskHostFactorySpecified ? "TaskHostFactory" : "AssemblyTaskFactory";
                 string pidTaskProject = $@"
 <Project>
-    <UsingTask TaskName=""ProcessIdTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""TaskHostFactory"" />
+    <UsingTask TaskName=""ProcessIdTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""{taskFactory}"" />
     <Target Name='AccessPID'>
         <ProcessIdTask>
             <Output PropertyName=""PID"" TaskParameter=""Pid"" />
         </ProcessIdTask>
     </Target>
 </Project>";
                 TransientTestFile project = env.CreateFile("testProject.csproj", pidTaskProject);
+
+                if (envVariableSpecified)
+                {
+                    env.SetEnvironmentVariable("MSBUILDFORCEALLTASKSOUTOFPROC", "1");
+                }
                 ProjectInstance projectInstance = new(project.Path);
+
                 projectInstance.Build().ShouldBeTrue();
                 string processId = projectInstance.GetPropertyValue("PID");
                 string.IsNullOrEmpty(processId).ShouldBeFalse();
                 Int32.TryParse(processId, out int pid).ShouldBeTrue();
-                Process.GetCurrentProcess().Id.ShouldNotBe<int>(pid);
-                try
+                Process.GetCurrentProcess().Id.ShouldNotBe(pid);
+
+                if (taskHostFactorySpecified)
                 {
+                    try
+                    {
+                        Process taskHostNode = Process.GetProcessById(pid);
+                        taskHostNode.WaitForExit(3000).ShouldBeTrue($"The executed MSBuild Version: {projectInstance.GetProperty("MSBuildVersion")}");
+                    }
+                    // We expect the TaskHostNode to exit quickly. If it exits before Process.GetProcessById, it will throw an ArgumentException.
+                    catch (ArgumentException e)
+                    {
+                        e.Message.ShouldBe($"Process with an Id of {pid} is not running.");
+                    }
+                }
+                else
+                {
+                    // This is the sidecar TaskHost case - it should persist after build is done. So we need to clean up and kill it ourselves.
                     Process taskHostNode = Process.GetProcessById(pid);
-                    taskHostNode.WaitForExit(2000).ShouldBeTrue();
+                    taskHostNode.WaitForExit(3000).ShouldBeFalse($"The executed MSBuild Version: {projectInstance.GetProperty("MSBuildVersion")}");
+                    taskHostNode.Kill();
+                }
+            }
+        }
+
+        [Fact]
+        public void TransientandSidecarNodeCanCoexist()
+        {
+            using (TestEnvironment env = TestEnvironment.Create())
+            {
+                string pidTaskProject = $@"
+<Project>
+<UsingTask TaskName=""ProcessIdTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""TaskHostFactory"" />
+<UsingTask TaskName=""ProcessIdTaskSidecar"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""AssemblyTaskFactory"" />
+
+<Target Name='AccessPID'>
+    <ProcessIdTask>
+        <Output PropertyName=""PID"" TaskParameter=""Pid"" />
+    </ProcessIdTask>
+    <ProcessIdTaskSidecar>
+        <Output PropertyName=""PID2"" TaskParameter=""Pid"" />
+    </ProcessIdTaskSidecar>
+</Target>
+</Project>";
+
+                TransientTestFile project = env.CreateFile("testProject.csproj", pidTaskProject);
+
+
+                env.SetEnvironmentVariable("MSBUILDFORCEALLTASKSOUTOFPROC", "1");
+                ProjectInstance projectInstance = new(project.Path);
+
+                projectInstance.Build().ShouldBeTrue();
+                string processId = projectInstance.GetPropertyValue("PID");
+                string processIdSidecar = projectInstance.GetPropertyValue("PID2");
+                processIdSidecar.ShouldNotBe(processId, "Each task should have it's own TaskHost node.");
+
+                string.IsNullOrEmpty(processId).ShouldBeFalse();
+                Int32.TryParse(processId, out int pid).ShouldBeTrue();
+                Int32.TryParse(processIdSidecar, out int pidSidecar).ShouldBeTrue();
+
+                Process.GetCurrentProcess().Id.ShouldNotBe(pid);
+
+
+                try
+                {
+                    Process taskHostNode1 = Process.GetProcessById(pid);
+                    taskHostNode1.WaitForExit(3000).ShouldBeTrue("The node should be dead since this is the transient case.");
                 }
-                // We expect the TaskHostNode to exit quickly. If it exits before Process.GetProcessById, it will throw an ArgumentException.
                 catch (ArgumentException e)
                 {
+                    // We expect the TaskHostNode to exit quickly. If it exits before Process.GetProcessById, it will throw an ArgumentException.
                     e.Message.ShouldBe($"Process with an Id of {pid} is not running.");
                 }
+                // This is the sidecar TaskHost case - it should persist after build is done. So we need to clean up and kill it ourselves.
+                Process taskHostNode2 = Process.GetProcessById(pidSidecar);
+                taskHostNode2.WaitForExit(3000).ShouldBeFalse($"The node should be alive since it is the sidecar node.");
+                taskHostNode2.Kill();
             }
         }
 

src/Build/BackEnd/Client/MSBuildClient.cs

@@ -619,13 +619,16 @@ private bool TryConnectToServer(int timeoutMilliseconds)
             while (tryAgain && sw.ElapsedMilliseconds < timeoutMilliseconds)
             {
                 tryAgain = false;
-                try
+
+
+                if (NodeProviderOutOfProcBase.TryConnectToPipeStream(
+                    _nodeStream, _pipeName, _handshake, Math.Max(1, timeoutMilliseconds - (int)sw.ElapsedMilliseconds), out HandshakeResult result))
                 {
-                    NodeProviderOutOfProcBase.ConnectToPipeStream(_nodeStream, _pipeName, _handshake, Math.Max(1, timeoutMilliseconds - (int)sw.ElapsedMilliseconds));
+                    return true;
                 }
-                catch (Exception ex)
+                else
                 {
-                    if (ex is not TimeoutException && sw.ElapsedMilliseconds < timeoutMilliseconds)
+                    if (result.Status is not HandshakeStatus.Timeout && sw.ElapsedMilliseconds < timeoutMilliseconds)
                     {
                         CommunicationsUtilities.Trace("Retrying to connect to server after {0} ms", sw.ElapsedMilliseconds);
                         // This solves race condition for time in which server started but have not yet listen on pipe or
@@ -635,14 +638,14 @@ private bool TryConnectToServer(int timeoutMilliseconds)
                     }
                     else
                     {
-                        CommunicationsUtilities.Trace("Failed to connect to server: {0}", ex);
+                        CommunicationsUtilities.Trace("Failed to connect to server: {0}", result.ErrorMessage);
                         _exitResult.MSBuildClientExitType = MSBuildClientExitType.UnableToConnect;
                         return false;
                     }
                 }
             }
 
-            return true;
+            return false;
         }
 
         private void WritePacket(Stream nodeStream, INodePacket packet)

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

@@ -467,8 +467,16 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han
 
             try
             {
-                ConnectToPipeStream(nodeStream, pipeName, handshake, timeout);
-                return nodeStream;
+                if (TryConnectToPipeStream(nodeStream, pipeName, handshake, timeout, out HandshakeResult result))
+                {
+                    return nodeStream;
+                }
+                else
+                {
+                    CommunicationsUtilities.Trace("Failed to connect to pipe {0}. {1}", pipeName, result.ErrorMessage.TrimEnd());
+                    nodeStream?.Dispose();
+                    return null;
+                }
             }
             catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
             {
@@ -492,7 +500,7 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han
         /// <remarks>
         /// Reused by MSBuild server client <see cref="Microsoft.Build.Experimental.MSBuildClient"/>.
         /// </remarks>
-        internal static void ConnectToPipeStream(NamedPipeClientStream nodeStream, string pipeName, Handshake handshake, int timeout)
+        internal static bool TryConnectToPipeStream(NamedPipeClientStream nodeStream, string pipeName, Handshake handshake, int timeout, out HandshakeResult result)
         {
             nodeStream.Connect(timeout);
 
@@ -521,13 +529,24 @@ internal static void ConnectToPipeStream(NamedPipeClientStream nodeStream, strin
 
             CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName);
 
+            if (
+
+            nodeStream.TryReadEndOfHandshakeSignal(true,
 #if NETCOREAPP2_1_OR_GREATER
-            nodeStream.ReadEndOfHandshakeSignal(true, timeout);
-#else
-            nodeStream.ReadEndOfHandshakeSignal(true);
+            timeout,
 #endif
-            // We got a connection.
-            CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName);
+            out HandshakeResult innerResult))
+            {
+                // We got a connection.
+                CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName);
+                result = HandshakeResult.Success(0);
+                return true;
+            }
+            else
+            {
+                result = innerResult;
+                return false;
+            }
         }
 
         /// <summary>

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

@@ -647,6 +647,8 @@ internal bool CreateNode(HandshakeOptions hostContext, INodePacketFactory factor
             // if runtime host path is null it means we don't have MSBuild.dll path resolved and there is no need to include it in the command line arguments.
             string commandLineArgsPlaceholder = "{0} /nologo /nodemode:2 /nodereuse:{1} /low:{2} ";
 
+            bool enableNodeReuse = ComponentHost.BuildParameters.EnableNodeReuse && Handshake.IsHandshakeOptionEnabled(hostContext, HandshakeOptions.NodeReuse);
+
             IList<NodeContext> nodeContexts;
             int nodeId = (int)hostContext;
 
@@ -687,7 +689,7 @@ internal bool CreateNode(HandshakeOptions hostContext, INodePacketFactory factor
 
             nodeContexts = GetNodes(
                 msbuildLocation,
-                string.Format(commandLineArgsPlaceholder, string.Empty, ComponentHost.BuildParameters.EnableNodeReuse, ComponentHost.BuildParameters.LowPriority),
+                string.Format(commandLineArgsPlaceholder, string.Empty, enableNodeReuse, ComponentHost.BuildParameters.LowPriority),
                 nodeId,
                 this,
                 new Handshake(hostContext),

src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs

@@ -75,6 +75,13 @@ internal class AssemblyTaskFactory : ITaskFactory2
         /// </summary>
         private TaskLoggingContext _taskLoggingContext;
 
+        /// <summary>
+        /// If yes, then this is a task that will for any reason be run in a task host.
+        /// This might be due to the task not supporting the new single threaded interface
+        /// or due to the project requesting it to be run out of process.
+        /// </summary>
+        private bool _isTaskHostFactory;
+
         #endregion
 
         /// <summary>
@@ -259,7 +266,7 @@ internal LoadedType InitializeFactory(
                 IDictionary<string, TaskPropertyInfo> taskParameters,
                 string taskElementContents,
                 IDictionary<string, string> taskFactoryIdentityParameters,
-                bool taskHostFactoryExplicitlyRequested,
+                bool taskHostExplicitlyRequested,
                 TargetLoggingContext targetLoggingContext,
                 ElementLocation elementLocation,
                 string taskProjectFile)
@@ -272,7 +279,11 @@ internal LoadedType InitializeFactory(
                 _factoryIdentityParameters = new Dictionary<string, string>(taskFactoryIdentityParameters, StringComparer.OrdinalIgnoreCase);
             }
 
-            _taskHostFactoryExplicitlyRequested = taskHostFactoryExplicitlyRequested;
+            _taskHostFactoryExplicitlyRequested = taskHostExplicitlyRequested;
+
+            _isTaskHostFactory = (taskFactoryIdentityParameters != null
+                 && taskFactoryIdentityParameters.TryGetValue(Constants.TaskHostExplicitlyRequested, out string isTaskHostFactory)
+                 && isTaskHostFactory.Equals("true", StringComparison.OrdinalIgnoreCase));
 
             try
             {
@@ -381,7 +392,8 @@ internal ITask CreateTaskInstance(
                     taskLoggingContext,
                     buildComponentHost,
                     mergedParameters,
-                    _loadedType
+                    _loadedType,
+                    _isTaskHostFactory
 #if FEATURE_APPDOMAIN
                     , appDomainSetup
 #endif