Skip to content

Commit e39ece4

Browse files
authored
sidecar-taskhost-attempt-2 (#12145)
Fixes #12071 Second attempt at 12071. First one failed due to VS insertion Perf DDRITs.
1 parent 0f8b66d commit e39ece4

File tree

19 files changed

+432
-137
lines changed

19 files changed

+432
-137
lines changed

src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,7 @@ public void VerifyExplicitlyLaunchTaskHost()
544544
ITask createdTask = null;
545545
try
546546
{
547-
SetupTaskFactory(null, true /* want task host */);
547+
SetupTaskFactory(null, true /* want task host */, true);
548548

549549
createdTask = _taskFactory.CreateTaskInstance(ElementLocation.Create("MSBUILD"), null, new MockHost(), null,
550550
#if FEATURE_APPDOMAIN
@@ -578,7 +578,7 @@ public void VerifyExplicitlyLaunchTaskHostEvenIfParametersMatch1()
578578
taskParameters.Add(XMakeAttributes.runtime, XMakeAttributes.MSBuildRuntimeValues.any);
579579
taskParameters.Add(XMakeAttributes.architecture, XMakeAttributes.MSBuildArchitectureValues.any);
580580

581-
SetupTaskFactory(taskParameters, true /* want task host */);
581+
SetupTaskFactory(taskParameters, true /* want task host */, isTaskHostFactory: true);
582582

583583
createdTask = _taskFactory.CreateTaskInstance(ElementLocation.Create("MSBUILD"), null, new MockHost(), null,
584584
#if FEATURE_APPDOMAIN
@@ -608,7 +608,7 @@ public void VerifyExplicitlyLaunchTaskHostEvenIfParametersMatch2()
608608
ITask createdTask = null;
609609
try
610610
{
611-
SetupTaskFactory(null, true /* want task host */);
611+
SetupTaskFactory(null, true /* want task host */, true);
612612

613613
IDictionary<string, string> taskParameters = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
614614
taskParameters.Add(XMakeAttributes.runtime, XMakeAttributes.MSBuildRuntimeValues.any);
@@ -644,7 +644,7 @@ public void VerifySameFactoryCanGenerateDifferentTaskInstances()
644644
factoryParameters.Add(XMakeAttributes.runtime, XMakeAttributes.MSBuildRuntimeValues.any);
645645
factoryParameters.Add(XMakeAttributes.architecture, XMakeAttributes.MSBuildArchitectureValues.any);
646646

647-
SetupTaskFactory(factoryParameters, explicitlyLaunchTaskHost: false);
647+
SetupTaskFactory(factoryParameters, explicitlyLaunchTaskHost: false, isTaskHostFactory: false);
648648

649649
try
650650
{
@@ -695,14 +695,19 @@ public void VerifySameFactoryCanGenerateDifferentTaskInstances()
695695
/// Abstract out the creation of the new AssemblyTaskFactory with default task, and
696696
/// with some basic validation.
697697
/// </summary>
698-
private void SetupTaskFactory(IDictionary<string, string> factoryParameters, bool explicitlyLaunchTaskHost)
698+
private void SetupTaskFactory(IDictionary<string, string> factoryParameters, bool explicitlyLaunchTaskHost = false, bool isTaskHostFactory = false)
699699
{
700700
_taskFactory = new AssemblyTaskFactory();
701701
#if FEATURE_ASSEMBLY_LOCATION
702702
_loadInfo = AssemblyLoadInfo.Create(null, Assembly.GetAssembly(typeof(TaskToTestFactories)).Location);
703703
#else
704704
_loadInfo = AssemblyLoadInfo.Create(typeof(TaskToTestFactories).GetTypeInfo().Assembly.FullName, null);
705705
#endif
706+
if (explicitlyLaunchTaskHost)
707+
{
708+
factoryParameters ??= new Dictionary<string, string>();
709+
factoryParameters.Add(Internal.Constants.TaskHostExplicitlyRequested, "true");
710+
}
706711
_loadedType = _taskFactory.InitializeFactory(_loadInfo, "TaskToTestFactories", new Dictionary<string, TaskPropertyInfo>(), string.Empty, factoryParameters, explicitlyLaunchTaskHost, null, ElementLocation.Create("NONE"), String.Empty);
707712
Assert.True(_loadedType.Assembly.Equals(_loadInfo)); // "Expected the AssemblyLoadInfo to be equal"
708713
}
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System.Diagnostics;
5+
using Microsoft.Build.Framework;
6+
using Microsoft.Build.Utilities;
7+
#nullable disable
8+
9+
namespace Microsoft.Build.UnitTests
10+
{
11+
/// <summary>
12+
/// This task was created for https://github.com/dotnet/msbuild/issues/3141
13+
/// </summary>
14+
public class ProcessIdTaskSidecar : Task
15+
{
16+
[Output]
17+
public int Pid { get; set; }
18+
19+
/// <summary>
20+
/// Log the id for this process.
21+
/// </summary>
22+
/// <returns></returns>
23+
public override bool Execute()
24+
{
25+
Pid = Process.GetCurrentProcess().Id;
26+
return true;
27+
}
28+
}
29+
}

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

Lines changed: 84 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,37 +26,114 @@ public TaskHostFactory_Tests(ITestOutputHelper testOutputHelper)
2626
_output = testOutputHelper;
2727
}
2828

29-
[Fact]
30-
public void TaskNodesDieAfterBuild()
29+
[Theory]
30+
[InlineData(true, false)]
31+
[InlineData(false, true)]
32+
[InlineData(true, true)]
33+
public void TaskNodesDieAfterBuild(bool taskHostFactorySpecified, bool envVariableSpecified)
3134
{
3235
using (TestEnvironment env = TestEnvironment.Create())
3336
{
37+
38+
string taskFactory = taskHostFactorySpecified ? "TaskHostFactory" : "AssemblyTaskFactory";
3439
string pidTaskProject = $@"
3540
<Project>
36-
<UsingTask TaskName=""ProcessIdTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""TaskHostFactory"" />
41+
<UsingTask TaskName=""ProcessIdTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""{taskFactory}"" />
3742
<Target Name='AccessPID'>
3843
<ProcessIdTask>
3944
<Output PropertyName=""PID"" TaskParameter=""Pid"" />
4045
</ProcessIdTask>
4146
</Target>
4247
</Project>";
4348
TransientTestFile project = env.CreateFile("testProject.csproj", pidTaskProject);
49+
50+
if (envVariableSpecified)
51+
{
52+
env.SetEnvironmentVariable("MSBUILDFORCEALLTASKSOUTOFPROC", "1");
53+
}
4454
ProjectInstance projectInstance = new(project.Path);
55+
4556
projectInstance.Build().ShouldBeTrue();
4657
string processId = projectInstance.GetPropertyValue("PID");
4758
string.IsNullOrEmpty(processId).ShouldBeFalse();
4859
Int32.TryParse(processId, out int pid).ShouldBeTrue();
49-
Process.GetCurrentProcess().Id.ShouldNotBe<int>(pid);
50-
try
60+
Process.GetCurrentProcess().Id.ShouldNotBe(pid);
61+
62+
if (taskHostFactorySpecified)
5163
{
64+
try
65+
{
66+
Process taskHostNode = Process.GetProcessById(pid);
67+
taskHostNode.WaitForExit(3000).ShouldBeTrue($"The executed MSBuild Version: {projectInstance.GetProperty("MSBuildVersion")}");
68+
}
69+
// We expect the TaskHostNode to exit quickly. If it exits before Process.GetProcessById, it will throw an ArgumentException.
70+
catch (ArgumentException e)
71+
{
72+
e.Message.ShouldBe($"Process with an Id of {pid} is not running.");
73+
}
74+
}
75+
else
76+
{
77+
// This is the sidecar TaskHost case - it should persist after build is done. So we need to clean up and kill it ourselves.
5278
Process taskHostNode = Process.GetProcessById(pid);
53-
taskHostNode.WaitForExit(2000).ShouldBeTrue();
79+
taskHostNode.WaitForExit(3000).ShouldBeFalse($"The executed MSBuild Version: {projectInstance.GetProperty("MSBuildVersion")}");
80+
taskHostNode.Kill();
81+
}
82+
}
83+
}
84+
85+
[Fact]
86+
public void TransientandSidecarNodeCanCoexist()
87+
{
88+
using (TestEnvironment env = TestEnvironment.Create())
89+
{
90+
string pidTaskProject = $@"
91+
<Project>
92+
<UsingTask TaskName=""ProcessIdTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""TaskHostFactory"" />
93+
<UsingTask TaskName=""ProcessIdTaskSidecar"" AssemblyName=""Microsoft.Build.Engine.UnitTests"" TaskFactory=""AssemblyTaskFactory"" />
94+
95+
<Target Name='AccessPID'>
96+
<ProcessIdTask>
97+
<Output PropertyName=""PID"" TaskParameter=""Pid"" />
98+
</ProcessIdTask>
99+
<ProcessIdTaskSidecar>
100+
<Output PropertyName=""PID2"" TaskParameter=""Pid"" />
101+
</ProcessIdTaskSidecar>
102+
</Target>
103+
</Project>";
104+
105+
TransientTestFile project = env.CreateFile("testProject.csproj", pidTaskProject);
106+
107+
108+
env.SetEnvironmentVariable("MSBUILDFORCEALLTASKSOUTOFPROC", "1");
109+
ProjectInstance projectInstance = new(project.Path);
110+
111+
projectInstance.Build().ShouldBeTrue();
112+
string processId = projectInstance.GetPropertyValue("PID");
113+
string processIdSidecar = projectInstance.GetPropertyValue("PID2");
114+
processIdSidecar.ShouldNotBe(processId, "Each task should have it's own TaskHost node.");
115+
116+
string.IsNullOrEmpty(processId).ShouldBeFalse();
117+
Int32.TryParse(processId, out int pid).ShouldBeTrue();
118+
Int32.TryParse(processIdSidecar, out int pidSidecar).ShouldBeTrue();
119+
120+
Process.GetCurrentProcess().Id.ShouldNotBe(pid);
121+
122+
123+
try
124+
{
125+
Process taskHostNode1 = Process.GetProcessById(pid);
126+
taskHostNode1.WaitForExit(3000).ShouldBeTrue("The node should be dead since this is the transient case.");
54127
}
55-
// We expect the TaskHostNode to exit quickly. If it exits before Process.GetProcessById, it will throw an ArgumentException.
56128
catch (ArgumentException e)
57129
{
130+
// We expect the TaskHostNode to exit quickly. If it exits before Process.GetProcessById, it will throw an ArgumentException.
58131
e.Message.ShouldBe($"Process with an Id of {pid} is not running.");
59132
}
133+
// This is the sidecar TaskHost case - it should persist after build is done. So we need to clean up and kill it ourselves.
134+
Process taskHostNode2 = Process.GetProcessById(pidSidecar);
135+
taskHostNode2.WaitForExit(3000).ShouldBeFalse($"The node should be alive since it is the sidecar node.");
136+
taskHostNode2.Kill();
60137
}
61138
}
62139

src/Build/BackEnd/Client/MSBuildClient.cs

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -619,13 +619,16 @@ private bool TryConnectToServer(int timeoutMilliseconds)
619619
while (tryAgain && sw.ElapsedMilliseconds < timeoutMilliseconds)
620620
{
621621
tryAgain = false;
622-
try
622+
623+
624+
if (NodeProviderOutOfProcBase.TryConnectToPipeStream(
625+
_nodeStream, _pipeName, _handshake, Math.Max(1, timeoutMilliseconds - (int)sw.ElapsedMilliseconds), out HandshakeResult result))
623626
{
624-
NodeProviderOutOfProcBase.ConnectToPipeStream(_nodeStream, _pipeName, _handshake, Math.Max(1, timeoutMilliseconds - (int)sw.ElapsedMilliseconds));
627+
return true;
625628
}
626-
catch (Exception ex)
629+
else
627630
{
628-
if (ex is not TimeoutException && sw.ElapsedMilliseconds < timeoutMilliseconds)
631+
if (result.Status is not HandshakeStatus.Timeout && sw.ElapsedMilliseconds < timeoutMilliseconds)
629632
{
630633
CommunicationsUtilities.Trace("Retrying to connect to server after {0} ms", sw.ElapsedMilliseconds);
631634
// 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)
635638
}
636639
else
637640
{
638-
CommunicationsUtilities.Trace("Failed to connect to server: {0}", ex);
641+
CommunicationsUtilities.Trace("Failed to connect to server: {0}", result.ErrorMessage);
639642
_exitResult.MSBuildClientExitType = MSBuildClientExitType.UnableToConnect;
640643
return false;
641644
}
642645
}
643646
}
644647

645-
return true;
648+
return false;
646649
}
647650

648651
private void WritePacket(Stream nodeStream, INodePacket packet)

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

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -467,8 +467,16 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han
467467

468468
try
469469
{
470-
ConnectToPipeStream(nodeStream, pipeName, handshake, timeout);
471-
return nodeStream;
470+
if (TryConnectToPipeStream(nodeStream, pipeName, handshake, timeout, out HandshakeResult result))
471+
{
472+
return nodeStream;
473+
}
474+
else
475+
{
476+
CommunicationsUtilities.Trace("Failed to connect to pipe {0}. {1}", pipeName, result.ErrorMessage.TrimEnd());
477+
nodeStream?.Dispose();
478+
return null;
479+
}
472480
}
473481
catch (Exception e) when (!ExceptionHandling.IsCriticalException(e))
474482
{
@@ -492,7 +500,7 @@ private Stream TryConnectToProcess(int nodeProcessId, int timeout, Handshake han
492500
/// <remarks>
493501
/// Reused by MSBuild server client <see cref="Microsoft.Build.Experimental.MSBuildClient"/>.
494502
/// </remarks>
495-
internal static void ConnectToPipeStream(NamedPipeClientStream nodeStream, string pipeName, Handshake handshake, int timeout)
503+
internal static bool TryConnectToPipeStream(NamedPipeClientStream nodeStream, string pipeName, Handshake handshake, int timeout, out HandshakeResult result)
496504
{
497505
nodeStream.Connect(timeout);
498506

@@ -521,13 +529,24 @@ internal static void ConnectToPipeStream(NamedPipeClientStream nodeStream, strin
521529

522530
CommunicationsUtilities.Trace("Reading handshake from pipe {0}", pipeName);
523531

532+
if (
533+
534+
nodeStream.TryReadEndOfHandshakeSignal(true,
524535
#if NETCOREAPP2_1_OR_GREATER
525-
nodeStream.ReadEndOfHandshakeSignal(true, timeout);
526-
#else
527-
nodeStream.ReadEndOfHandshakeSignal(true);
536+
timeout,
528537
#endif
529-
// We got a connection.
530-
CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName);
538+
out HandshakeResult innerResult))
539+
{
540+
// We got a connection.
541+
CommunicationsUtilities.Trace("Successfully connected to pipe {0}...!", pipeName);
542+
result = HandshakeResult.Success(0);
543+
return true;
544+
}
545+
else
546+
{
547+
result = innerResult;
548+
return false;
549+
}
531550
}
532551

533552
/// <summary>

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,6 +647,8 @@ internal bool CreateNode(HandshakeOptions hostContext, INodePacketFactory factor
647647
// 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.
648648
string commandLineArgsPlaceholder = "{0} /nologo /nodemode:2 /nodereuse:{1} /low:{2} ";
649649

650+
bool enableNodeReuse = ComponentHost.BuildParameters.EnableNodeReuse && Handshake.IsHandshakeOptionEnabled(hostContext, HandshakeOptions.NodeReuse);
651+
650652
IList<NodeContext> nodeContexts;
651653
int nodeId = (int)hostContext;
652654

@@ -687,7 +689,7 @@ internal bool CreateNode(HandshakeOptions hostContext, INodePacketFactory factor
687689

688690
nodeContexts = GetNodes(
689691
msbuildLocation,
690-
string.Format(commandLineArgsPlaceholder, string.Empty, ComponentHost.BuildParameters.EnableNodeReuse, ComponentHost.BuildParameters.LowPriority),
692+
string.Format(commandLineArgsPlaceholder, string.Empty, enableNodeReuse, ComponentHost.BuildParameters.LowPriority),
691693
nodeId,
692694
this,
693695
new Handshake(hostContext),

src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,13 @@ internal class AssemblyTaskFactory : ITaskFactory2
7575
/// </summary>
7676
private TaskLoggingContext _taskLoggingContext;
7777

78+
/// <summary>
79+
/// If yes, then this is a task that will for any reason be run in a task host.
80+
/// This might be due to the task not supporting the new single threaded interface
81+
/// or due to the project requesting it to be run out of process.
82+
/// </summary>
83+
private bool _isTaskHostFactory;
84+
7885
#endregion
7986

8087
/// <summary>
@@ -259,7 +266,7 @@ internal LoadedType InitializeFactory(
259266
IDictionary<string, TaskPropertyInfo> taskParameters,
260267
string taskElementContents,
261268
IDictionary<string, string> taskFactoryIdentityParameters,
262-
bool taskHostFactoryExplicitlyRequested,
269+
bool taskHostExplicitlyRequested,
263270
TargetLoggingContext targetLoggingContext,
264271
ElementLocation elementLocation,
265272
string taskProjectFile)
@@ -272,7 +279,11 @@ internal LoadedType InitializeFactory(
272279
_factoryIdentityParameters = new Dictionary<string, string>(taskFactoryIdentityParameters, StringComparer.OrdinalIgnoreCase);
273280
}
274281

275-
_taskHostFactoryExplicitlyRequested = taskHostFactoryExplicitlyRequested;
282+
_taskHostFactoryExplicitlyRequested = taskHostExplicitlyRequested;
283+
284+
_isTaskHostFactory = (taskFactoryIdentityParameters != null
285+
&& taskFactoryIdentityParameters.TryGetValue(Constants.TaskHostExplicitlyRequested, out string isTaskHostFactory)
286+
&& isTaskHostFactory.Equals("true", StringComparison.OrdinalIgnoreCase));
276287

277288
try
278289
{
@@ -381,7 +392,8 @@ internal ITask CreateTaskInstance(
381392
taskLoggingContext,
382393
buildComponentHost,
383394
mergedParameters,
384-
_loadedType
395+
_loadedType,
396+
_isTaskHostFactory
385397
#if FEATURE_APPDOMAIN
386398
, appDomainSetup
387399
#endif

0 commit comments

Comments
 (0)