Do not respect NodeReuse for TaskHosts Fixes #3141 by Forgind · Pull Request #5144 · dotnet/msbuild

Forgind · 2020-02-24T19:47:19Z

They can lock the node, causing errors.

Resolves #3141.

They can lock the node, causing errors.

rainersigwald

Can you add a regression test? Maybe a test task that returns its PID, and validate that that PID is not alive at the end of the build?

rainersigwald · 2020-02-24T20:10:38Z

src/MSBuild/OutOfProcTaskHostNode.cs

            ErrorUtilities.VerifyThrow(!_isTaskExecuting, "We should never have a task in the process of executing when we receive NodeBuildComplete.");

-            _shutdownReason = buildComplete.PrepareForReuse ? NodeEngineShutdownReason.BuildCompleteReuse : NodeEngineShutdownReason.BuildComplete;
+            _shutdownReason = NodeEngineShutdownReason.BuildComplete;


Worth a comment that there's no reuse scenario here because ...

cdmihai · 2020-03-03T01:27:36Z

Wouldn't this slow down people whose builds call into out of proc task nodes many times? An escape hatch might be useful. I guess it depends which population is larger, the repos with frequently called out of proc tasks, or the repos that build their own tasks. Probably the later ...

Forgind · 2020-03-03T01:31:17Z

I could add an escape hatch. I also considered trying to detect if the relevant task was a custom task or MSBuild task, but it got a little too complicated.

rainersigwald · 2020-03-03T21:31:08Z

src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs

+    </Target>
+      </Project>";
+                TransientTestFile project = env.CreateFile("testProject.csproj", pidTaskProject);
+                MockLogger logger = new MockLogger();


Suggested change

MockLogger logger = new MockLogger();

MockLogger logger = new MockLogger(_output);

rainersigwald · 2020-03-04T22:43:45Z

src/Shared/Traits.cs

        public readonly bool UseSymlinkTimeInsteadOfTargetTime = Environment.GetEnvironmentVariable("MSBUILDUSESYMLINKTIMESTAMP") == "1";

+        /// <summary>
+        /// Allow node reuse on TaskHost nodes. This breaks if there are custom tasks.


Suggested change

/// Allow node reuse on TaskHost nodes. This breaks if there are custom tasks.

/// Allow node reuse of TaskHost nodes. This results in task assemblies locked past the build lifetime, but may improve performance.

rainersigwald · 2020-03-04T22:44:17Z

src/Tasks.UnitTests/PortableTasks_Tests.cs

 using System.IO;
 using System.Linq;
 using System.Text.RegularExpressions;
-using Microsoft.Build.UnitTests;


Move unrelated edit to a new PR or just revert.

rainersigwald · 2020-03-04T22:49:21Z

src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs


+        [Fact]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp, "https://github.com/microsoft/msbuild/issues/5158")]
+        public void TaskNodesDieAfterBuild()


Why is this test in this class? It's not about warnings-as-messages. Can you find a better home, like maybe a new TaskHostFactory_Tests?

rainersigwald · 2020-03-04T23:02:17Z

src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs

+                int start = logger.FullLog.IndexOf(introToPID) + introToPID.Length;
+                int length = logger.FullLog.IndexOf(afterPID) - start;
+                string pid = logger.FullLog.Substring(start, length);


Instead of grepping through the logging, can you change the test to return the PID of its process as a property, capture the property into a named property when you call it, and read the property out of the built project instance? That's a much cleaner way to do things (and a good exercise for you in using our API).

rainersigwald

Sorry, misclick on previous review.

rainersigwald · 2020-03-05T17:40:28Z

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

@@ -0,0 +1,38 @@
+using System;


copyright header please

rainersigwald · 2020-03-05T17:40:57Z

src/Build.UnitTests/BackEnd/ProcessIdTask.cs

    public class ProcessIdTask : Task
    {
+        [Output]
+        public Int32 pid { get; set; }


Suggested change

public Int32 pid { get; set; }

public int Pid { get; set; }

style nits: use the C# type where possible, and properties should start with a capital letter.

rainersigwald · 2020-03-05T17:44:56Z

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

+    public sealed class TaskHostFactory_Tests
+    {
+        [Fact]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp, "https://github.com/microsoft/msbuild/issues/5158")]


Does this work on Mono?

Does Mono support out-of-proc task hosts? I was going to find out with the automated tests.

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

This will indicate if we're using a TaskHostNode as expected in the new test.

Todo: investigate failure

rainersigwald

LGTM after test tweaks

rainersigwald · 2020-03-18T18:15:13Z

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

-                Should.Throw<ArgumentException>(() => Process.GetProcessById(pid));
+                try {
+                    Process taskHostNode = Process.GetProcessById(pid);
+                    taskHostNode.WaitForExit(2000);


Assert that this reported that the process ended (and didn't time out).

That's caught by the Should.Throw immediately afterwards. If it times out, Process.GetProcessById won't throw, and the test will fail.

Not necessarily though, given PID reuse.

rainersigwald · 2020-03-18T18:15:51Z

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

+                catch (Exception e)
+                {
+                    e.ShouldBeOfType<ArgumentException>();
+                }


Why this catch?

We want taskHostNode to die. If it dies before this try statement, Process.GetProcessById will throw an ArgumentException, so this test succeeded if either the Should.Throw statement succeeds or the node dies fast, and Process.GetProcessById throws an ArgumentException.

Comment with that reasoning, catch only the exact exception, minimize the scope of the catch, and can you assert details of it?

This reverts commit fe3fd57.

Forgind · 2020-03-18T21:53:11Z

Checked with @rainersigwald, and this does not work on mono, so disabling the test seems reasonable.

@jaredpar

In some offline discussion, @jaredpar pointed out that the TaskHostFactory docs didn't specify that the TaskHost node wouldn't live on past the build. It has since 16.6 (dotnet/msbuild#5144), so document that.

Do not respect NodeRuse for TaskHosts

8dde05f

They can lock the node, causing errors.

rainersigwald requested changes Feb 24, 2020

View reviewed changes

rainersigwald changed the title ~~Do not respect NodeRuse for TaskHosts Fixes #3141~~ Do not respect NodeReuse for TaskHosts Fixes #3141 Feb 25, 2020

Add test

b309c02

rainersigwald added this to the MSBuild 16.6 Preview 2 milestone Mar 2, 2020

Forgind added 5 commits March 2, 2020 18:02

Uninlined task

ecc1bb8

Merge remote-tracking branch 'origin/master' into node-reuse

02ce065

Disable In Proc Nodes

5572063

Add escape hatch

b1b6a74

Use TaskHostFactory

3075bfc

rainersigwald reviewed Mar 3, 2020

View reviewed changes

Forgind mentioned this pull request Mar 4, 2020

Enable out of proc TaskHost on .NET Core #5158

Closed

Skipped test on core

95625e4

rainersigwald reviewed Mar 4, 2020

View reviewed changes

Comment change

89c85ce

rainersigwald approved these changes Mar 4, 2020

View reviewed changes

rainersigwald requested changes Mar 4, 2020

View reviewed changes

Took in comments

66239ee

rainersigwald requested changes Mar 5, 2020

View reviewed changes

Minor changes

d8e8a85

Forgind force-pushed the node-reuse branch from a0886e5 to d8e8a85 Compare March 5, 2020 18:21

Forgind added 3 commits March 5, 2020 15:34

Added two asserts

d396f03

Aggressively kill TaskHostNodes

5beb459

This will indicate if we're using a TaskHostNode as expected in the new test.

Sleep thread

b4ad58b

Forgind force-pushed the node-reuse branch from 6a9c549 to b4ad58b Compare March 17, 2020 15:44

Forgind added 3 commits March 17, 2020 08:55

Changed means of waiting

d84a4d2

Initialized testoutputhelper

01dd270

Stop when node dies

cecff4b

Forgind added 4 commits March 17, 2020 11:45

Reduced to one second

87de64e

Add more precise sleep

db9000f

Disable test on mono

b3639b6

Todo: investigate failure

Skip instead of fail

fe3fd57

rainersigwald approved these changes Mar 18, 2020

View reviewed changes

Forgind added 3 commits March 18, 2020 11:31

Revert "Skip instead of fail"

31dbf62

This reverts commit fe3fd57.

Change exception handling

be2439e

Separated brace onto its own line

8623c66

Forgind merged commit 3453bee into dotnet:master Mar 18, 2020

Forgind deleted the node-reuse branch March 18, 2020 21:53

benvillalobos mentioned this pull request Apr 9, 2020

Visual Studio reuses MSBuild process although MSBuild node reuse is disabled #5221

Closed

rainersigwald mentioned this pull request Jun 3, 2020

Custom task assembly locked by MSBuild after building in Visual Studio #2670

Closed

Forgind mentioned this pull request Jan 12, 2022

Taskhosts die within build #7260

Closed

rainersigwald mentioned this pull request Apr 13, 2023

MSBuild: specify TaskHostFactory is short-lived MicrosoftDocs/visualstudio-docs#9136

Merged

	MockLogger logger = new MockLogger();
	MockLogger logger = new MockLogger(_output);

	/// Allow node reuse on TaskHost nodes. This breaks if there are custom tasks.
	/// Allow node reuse of TaskHost nodes. This results in task assemblies locked past the build lifetime, but may improve performance.

Conversation

Forgind commented Feb 24, 2020

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai commented Mar 3, 2020

Uh oh!

Forgind commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rainersigwald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind commented Mar 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants