diff --git a/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs b/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs index fa60eee0889..f7a6ec573fd 100644 --- a/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs +++ b/src/Build.UnitTests/BackEnd/TaskRouter_IntegrationTests.cs @@ -3,11 +3,8 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Reflection; -using System.Text.RegularExpressions; -using Microsoft.Build.BackEnd; using Microsoft.Build.Execution; using Microsoft.Build.Framework; using Microsoft.Build.Shared; @@ -384,257 +381,6 @@ public void ExplicitTaskHostFactory_OverridesRoutingLogic() logger.FullLog.ShouldContain("TaskWithAttribute executed"); } - [Fact] - public void RequiresTransientTaskHost_ReturnsTrueForRestoreTask() - { - TaskRouter.RequiresTransientTaskHost(typeof(NuGet.Build.Tasks.RestoreTask)).ShouldBeTrue(); - } - - [Fact] - public void RequiresTransientTaskHost_ReturnsFalseForRegularTask() - { - TaskRouter.RequiresTransientTaskHost(typeof(NonEnlightenedTestTask)).ShouldBeFalse(); - TaskRouter.RequiresTransientTaskHost(typeof(AttributeTestTask)).ShouldBeFalse(); - } - - [Fact] - public void RequiresTransientTaskHost_RoutedToTaskHost_InMultiThreadedMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskMT.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = true, - Loggers = [logger], - DisableInProcNode = false, - EnableNodeReuse = false, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result = buildManager.Build(buildParameters, buildRequestData); - - result.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - logger.FullLog.ShouldContain("RestoreTask executed"); - } - - [Fact] - public void RequiresTransientTaskHost_RoutedToTaskHost_InServerMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskServer.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = false, - Loggers = [logger], - DisableInProcNode = false, - EnableNodeReuse = false, - - // Simulate running under the MSBuild Server. In production this flag is - // set process-wide by OutOfProcServerNode via BuildParameters.MarkProcessAsLongLivedHost. - IsLongLivedHost = true, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result = buildManager.Build(buildParameters, buildRequestData); - - result.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - logger.FullLog.ShouldContain("RestoreTask executed"); - } - - [Fact] - public void RequiresTransientTaskHost_GetsFreshProcess_OnEachInvocation_InMultiThreadedMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskFreshProcess.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = true, - Loggers = [logger], - DisableInProcNode = false, - - // Reuse must stay ON so we test the workaround, not natural process death. - // With reuse off the test cannot distinguish "transient TaskHost (workaround)" - // from "sidecar TaskHost that happened to die between builds". - EnableNodeReuse = true, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - // Two separate Build cycles. The workaround forces nodeReuse=false on the spawned - // TaskHost so it dies at EndBuild, giving the next Build a fresh process. - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result1 = buildManager.Build(buildParameters, buildRequestData); - BuildResult result2 = buildManager.Build(buildParameters, buildRequestData); - - result1.OverallResult.ShouldBe(BuildResultCode.Success); - result2.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - - int[] pids = ExtractReportedPids(logger.FullLog); - - pids.Length.ShouldBe(2, $"Expected two RestoreTask invocations to log a PID. Log:{Environment.NewLine}{logger.FullLog}"); - pids[0].ShouldNotBe(pids[1], "Each build must spawn a fresh TaskHost so NuGet static state cannot leak across builds."); - pids.ShouldNotContain(Process.GetCurrentProcess().Id, "TaskHost should be out-of-process from the test runner."); - } - - [Fact] - public void RequiresTransientTaskHost_GetsFreshProcess_OnEachInvocation_InServerMode() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskFreshProcessServer.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = false, - Loggers = [logger], - DisableInProcNode = false, - - // Reuse must stay ON so we test the workaround, not natural process death. - EnableNodeReuse = true, - - // Simulate running under the MSBuild Server. - IsLongLivedHost = true, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - // Two separate Build cycles. The workaround forces nodeReuse=false on the spawned - // TaskHost so it dies at EndBuild, giving the next Build a fresh process. - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result1 = buildManager.Build(buildParameters, buildRequestData); - BuildResult result2 = buildManager.Build(buildParameters, buildRequestData); - - result1.OverallResult.ShouldBe(BuildResultCode.Success); - result2.OverallResult.ShouldBe(BuildResultCode.Success); - TaskRouterTestHelper.AssertTaskUsedTaskHost(logger, "RestoreTask"); - - int[] pids = ExtractReportedPids(logger.FullLog); - - pids.Length.ShouldBe(2, $"Expected two RestoreTask invocations to log a PID. Log:{Environment.NewLine}{logger.FullLog}"); - pids[0].ShouldNotBe(pids[1], "Each build must spawn a fresh TaskHost so NuGet static state cannot leak across builds."); - pids.ShouldNotContain(Process.GetCurrentProcess().Id, "TaskHost should be out-of-process from the test runner."); - } - - private static int[] ExtractReportedPids(string log) - { - var pids = new List(); - foreach (Match m in Regex.Matches(log, @"RestoreTask executed in PID=(\d+)")) - { - pids.Add(int.Parse(m.Groups[1].Value)); - } - - return pids.ToArray(); - } - - [Fact] - public void RequiresTransientTaskHost_RunsInProcess_WhenNoMTOrServer() - { - string projectContent = $@" - - - - - - -"; - - string projectFile = Path.Combine(_testProjectsDir, "RestoreTaskNoMT.proj"); - File.WriteAllText(projectFile, projectContent); - - var logger = new MockLogger(_output); - var buildParameters = new BuildParameters - { - MultiThreaded = false, - Loggers = [logger], - DisableInProcNode = false, - EnableNodeReuse = false, - }; - - var buildRequestData = new BuildRequestData( - projectFile, - new Dictionary(), - null, - ["TestTarget"], - null); - - BuildManager buildManager = BuildManager.DefaultBuildManager; - BuildResult result = buildManager.Build(buildParameters, buildRequestData); - - result.OverallResult.ShouldBe(BuildResultCode.Success); - - TaskRouterTestHelper.AssertTaskRanInProcess(logger, "RestoreTask"); - logger.FullLog.ShouldContain("RestoreTask executed"); - } - private string CreateTestProject(string taskName, string taskClass) { return $@" @@ -735,25 +481,6 @@ public override bool Execute() #endregion } -// Test task in the NuGet.Build.Tasks namespace to simulate the real RestoreTask for routing tests. -namespace NuGet.Build.Tasks -{ - /// - /// Simulates the NuGet RestoreTask for testing task routing workaround. - /// Has the same full name (NuGet.Build.Tasks.RestoreTask) that TaskRouter checks. - /// - public class RestoreTask : Task - { - public override bool Execute() - { - // Include the OS PID so tests can verify each invocation runs in a fresh - // TaskHost process - Log.LogMessage(MessageImportance.High, $"RestoreTask executed in PID={Process.GetCurrentProcess().Id}"); - return true; - } - } -} - // Custom attribute definition in Microsoft.Build.Framework namespace to match what TaskRouter expects // TaskRouter looks for attributes with FullName = "Microsoft.Build.Framework.MSBuildMultiThreadableTaskAttribute" // Since the real attribute is internal in Framework, we define our own test version here diff --git a/src/Build/BackEnd/BuildManager/BuildParameters.cs b/src/Build/BackEnd/BuildManager/BuildParameters.cs index 9565e8a884b..c0c0fcde2ce 100644 --- a/src/Build/BackEnd/BuildManager/BuildParameters.cs +++ b/src/Build/BackEnd/BuildManager/BuildParameters.cs @@ -64,12 +64,6 @@ public class BuildParameters : ITranslatable /// private static string s_startupDirectory = Environment.CurrentDirectory; - /// - /// Process-wide flag indicating that the engine is hosted in a long-lived process - /// (e.g., the MSBuild Server) that persists across multiple build invocations. - /// - private static bool s_isLongLivedHost; - /// /// Indicates whether we should warn when a property is uninitialized when it is used. /// @@ -334,7 +328,6 @@ internal BuildParameters(BuildParameters other, bool resetEnvironment = false) Question = other.Question; IsBuildCheckEnabled = other.IsBuildCheckEnabled; IsTelemetryEnabled = other.IsTelemetryEnabled; - IsLongLivedHost = other.IsLongLivedHost; ProjectCacheDescriptor = other.ProjectCacheDescriptor; _enableTargetOutputLogging = other.EnableTargetOutputLogging; } @@ -806,25 +799,6 @@ internal IToolsetProvider ToolsetProvider /// internal bool IsOutOfProc { get; set; } - /// - /// True when the engine is hosted in a long-lived process (e.g., MSBuild Server) - /// that persists across multiple build invocations. Used to opt tasks whose static - /// singleton state would leak across invocations out of sidecar TaskHost reuse. - /// See https://github.com/dotnet/msbuild/issues/13315. - /// - internal bool IsLongLivedHost - { - get => _isLongLivedHost; - set => _isLongLivedHost = value; - } - - private bool _isLongLivedHost = s_isLongLivedHost; - - /// - /// Marks the current process as a long-lived host. - /// - internal static void MarkProcessAsLongLivedHost() => s_isLongLivedHost = true; - /// public ProjectLoadSettings ProjectLoadSettings { @@ -1004,7 +978,6 @@ void ITranslatable.Translate(ITranslator translator) translator.Translate(ref _reportFileAccesses); translator.Translate(ref _enableTargetOutputLogging); translator.Translate(ref _multiThreaded); - translator.Translate(ref _isLongLivedHost); // ProjectRootElementCache is not transmitted. // ResetCaches is not transmitted. diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs index 7831afe7f54..b0814b13fe1 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskRouter.cs @@ -83,37 +83,6 @@ private static bool HasMultiThreadableTaskAttribute(Type taskType) }); } - /// - /// Full name of a task whose static singleton state makes it unsafe to run in a - /// long-lived sidecar TaskHost (which persists across invocations). Such tasks must - /// instead run in an explicit (transient) TaskHost that terminates after execution, - /// ensuring static state is cleaned up. - /// This is a temporary workaround until the task authors fix their static state issues. - /// See https://github.com/dotnet/msbuild/issues/13315 - /// - private const string TaskRequiringTransientTaskHostFullName = "NuGet.Build.Tasks.RestoreTask"; - - /// - /// Determines if a task must be routed to an explicit (transient) TaskHost rather than - /// a reusable sidecar, because its static singleton state would leak across invocations. - /// Such tasks should run in a TaskHost that terminates after execution so all static - /// state is cleaned up. - /// - /// The type of the task to evaluate. - /// True if the task requires a transient TaskHost; false otherwise. - public static bool RequiresTransientTaskHost(Type taskType) - { - ArgumentNullException.ThrowIfNull(taskType); - - string? fullName = taskType.FullName; - if (fullName is null) - { - return false; - } - - return string.Equals(fullName, TaskRequiringTransientTaskHostFullName, StringComparison.Ordinal); - } - /// /// Clears the thread-safety cache. Used primarily for testing. /// diff --git a/src/Build/BackEnd/Node/OutOfProcServerNode.cs b/src/Build/BackEnd/Node/OutOfProcServerNode.cs index 0c8ae60bd10..01708c2a34c 100644 --- a/src/Build/BackEnd/Node/OutOfProcServerNode.cs +++ b/src/Build/BackEnd/Node/OutOfProcServerNode.cs @@ -108,13 +108,6 @@ public NodeEngineShutdownReason Run(out Exception? shutdownException) return NodeEngineShutdownReason.Error; } - // Mark the process as a long-lived host so per-build BuildParameters instances - // inherit IsLongLivedHost = true. This drives the workaround that routes tasks - // whose static state would leak across invocations (e.g., NuGet RestoreTask) to - // a transient TaskHost instead of a reusable sidecar. - // See https://github.com/dotnet/msbuild/issues/13315. - BuildParameters.MarkProcessAsLongLivedHost(); - while (true) { NodeEngineShutdownReason shutdownReason = RunInternal(out shutdownException, handshake); @@ -388,7 +381,6 @@ private void HandleServerNodeBuildCommand(ServerNodeBuildCommand command) Directory.SetCurrentDirectory(command.StartupDirectory); CommunicationsUtilities.SetEnvironment(command.BuildProcessEnvironment); - Traits.UpdateFromEnvironment(); Thread.CurrentThread.CurrentCulture = command.Culture; diff --git a/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs b/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs index 9ad3591fbd0..b64c63ee083 100644 --- a/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs +++ b/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs @@ -343,23 +343,6 @@ internal ITask CreateTaskInstance( } } - // Workaround for tasks whose static singleton state would leak across invocations - // (e.g., NuGet RestoreTask). In MT mode or when MSBuild server is active, these tasks - // must run in a transient (non-sidecar) TaskHost so static state is cleaned up after - // each invocation. See https://github.com/dotnet/msbuild/issues/13315 - bool forceTransientTaskHost = false; - if (_loadedType?.Type != null && TaskRouter.RequiresTransientTaskHost(_loadedType.Type)) - { - bool isMultiThreaded = buildComponentHost?.BuildParameters?.MultiThreaded == true; - bool isLongLivedHost = buildComponentHost?.BuildParameters?.IsLongLivedHost == true; - - if (isMultiThreaded || isLongLivedHost) - { - useTaskFactory = true; - forceTransientTaskHost = true; - } - } - taskLoggingContext?.TargetLoggingContext?.ProjectLoggingContext?.ProjectTelemetry?.AddTaskExecution(GetType().FullName, isTaskHost: useTaskFactory); if (useTaskFactory) @@ -374,9 +357,7 @@ internal ITask CreateTaskInstance( // If the task host factory is explicitly requested, do not act as a sidecar task host. // This is important as customers use task host factories for short lived tasks to release // potential locks. - // Also disable sidecar for tasks that require a transient TaskHost so their - // static state is cleaned up between invocations. - bool useSidecarTaskHost = !forceTransientTaskHost && !(_factoryIdentityParameters.TaskHostFactoryExplicitlyRequested ?? false); + bool useSidecarTaskHost = !(_factoryIdentityParameters.TaskHostFactoryExplicitlyRequested ?? false); TaskHostTask task = new( taskLocation,