MSBuild server #7634

AR-May · 2022-05-18T15:23:18Z

Context

Adding an opt-in feature for executing the build in MSBuild server. With this feature turned on, MSBuild sends the build request for execution to the MSBuild server node. This approach avoids to execute targets and tasks into a short-living process from CLI tools like .NET SDK and MSBuild.exe.

Changes Made

This PR implements:

a new node that accepts build request from client. This approach avoids spawning a new MSBuild process for every build from CLI tools like .NET SDK.
a new MSBuild client classes able to communicate with MSBuild server node via the named pipe.
unit tests for the new code path (TODO).

…eHandshake.

Fixes #7374, #7373 Context MSBuild client is a new code path that is triggered with opt-in env variable. It sends the build request for execution to the MSBuild server node. This approach avoids to do execute targets and tasks into a short-living process from CLI tools like .NET SDK and MSBuild.exe. Changes Made This PR implements a new MSBuild client classes able to communicate with MSBuild server node via the named pipe. Testing Manually tested. Automatic tests will be added in another PR. Co-authored-by: Forgind <Forgind@users.noreply.github.com> Co-authored-by: Roman Konecny <rokonecn@microsoft.com>

* Some instrumentation * Add more details to ETW * Use class-wide variables

donJoseLuis

my review's timebox expired. Please take a look at my comments, I only managed to review 5/28 files.

src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs

src/Build/BackEnd/Client/MSBuildClientExitResult.cs

src/Build/BackEnd/Client/MSBuildClientExitType.cs

src/Build/BackEnd/Client/MSBuildClientPacketPump.cs

src/Framework/MSBuildEventSource.cs

* Added cancelation feature

Forgind

Just need to review OutOfProcServerNode

src/Build/BackEnd/Client/MSBuildClientPacketPump.cs

src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs

src/Build/BackEnd/Node/ConsoleOutput.cs

src/Build/BackEnd/Client/MSBuildClient.cs

* Fix control sequence emission * Some cleanup

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

…he (#7655) * Solving memory leak by reusing BuildManager and ProjectRoolElementCache * Do not clear project root element cache if in auto reload.

…nt code path. (#7668)

* Add giant test for MSBuild Server * Add comm traces * Remove test that uses MSBUILDNOINPROCNODE flag: it checks the wrong behavior. * Add comments about WaitForExit and set a timeout for the process execution. Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com> Co-authored-by: Roman Konecny <rokonecn@microsoft.com>

…build into feature/msbuild-server

* Added cancellation support for client. * Added cancellation support for client. * Fixing wrong merge * Removed "Cancelled" exit type * Resolving comments

* Handle race condition * Clean running server nodes in tests.

src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs

…d.Experimental" namespace. (#7688)

…build into feature/msbuild-server

Fixes #7658 Context See #7658 Changes Made MSBuild Server clients detects: ConsoleBufferWidth, AcceptAnsiColorCodes, ConsoleIsScreen, ConsoleBackgroundColor of current console and sent it to Server in ServerNodeBuildCommand. Server overrides ConsoleConfigueation so our loggers can get target console configuration. Testing Manual Notes There are no expected functional changes for NON Server and hence also VS scenarios. Co-authored-by: Forgind <Forgind@users.noreply.github.com> Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>

…build into feature/msbuild-server

* Send command line as string[] in dotnet builds.

* Fixed some commented issues

* Update doc * Fixed typo * Fixed another typo * Update documentation/MSBuild-Server.md Co-authored-by: Forgind <Forgind@users.noreply.github.com> * Update documentation/MSBuild-Server.md Co-authored-by: Forgind <Forgind@users.noreply.github.com> * Update documentation/MSBuild-Server.md Co-authored-by: Forgind <Forgind@users.noreply.github.com> * Update documentation/MSBuild-Server.md Co-authored-by: Forgind <Forgind@users.noreply.github.com> * Update documentation/MSBuild-Server.md Co-authored-by: Forgind <Forgind@users.noreply.github.com> * Update documentation/MSBuild-Server.md Co-authored-by: Forgind <Forgind@users.noreply.github.com> * Resolving comments Co-authored-by: Forgind <Forgind@users.noreply.github.com>

…build into feature/msbuild-server # Conflicts: # src/Build/BackEnd/Client/MSBuildClient.cs

…/msbuild-server # Conflicts: # src/Build/PublicAPI/netstandard/PublicAPI.Unshipped.txt

* Reduces allocations in case the tracing is not enabled * Resolving comment

rainersigwald · 2022-07-12T16:16:13Z

Notes from call with @rokonec going over the headlines on this:

Looked at MSBuild node stuff and some VBCSCompiler as-a-service stuff
- Mostly already reviewed
Some commands that get serialized
more complex stuff around console capture + serialization
- batched or 100ms timer
- could maybe test per-line/write to console
Commit about memory leak (don't dispose BuildManager in server mode)
Check ProjectRootElementCache if autoreload
complex stuff: console properties/redirection status has to be passed in build request, set where possible or stored in a static object instead of Console object itself <-- could be surprising if a logger checks this stuff
PrepareConsoleColor to use VT100 most of the time
(self) think about compat with new console logger project
(self) we should turn this on in the second-stage build

src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs

src/Build/BackEnd/BuildManager/BuildManager.cs

Forgind

🚢

Forgind · 2022-07-13T16:18:16Z

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

+                    processStartInfo.RedirectStandardInput = true;
+                    processStartInfo.RedirectStandardOutput = true;
+                    processStartInfo.RedirectStandardError = true;
+                    processStartInfo.CreateNoWindow = (creationFlags | BackendNativeMethods.CREATENOWINDOW) == BackendNativeMethods.CREATENOWINDOW;


Suggested change

processStartInfo.CreateNoWindow = (creationFlags | BackendNativeMethods.CREATENOWINDOW) == BackendNativeMethods.CREATENOWINDOW;

processStartInfo.CreateNoWindow = (creationFlags & BackendNativeMethods.CREATENOWINDOW) == BackendNativeMethods.CREATENOWINDOW;

?

Forgind · 2022-07-13T16:23:56Z

src/Build/BackEnd/Node/ServerNodeBuildCommand.cs

+    /// </summary>
+    internal sealed class ServerNodeBuildCommand : INodePacket
+    {
+        private string _commandLine = default!;


I'd prefer just disabling nullable here if that's viable, but it isn't critical.

Forgind · 2022-07-13T16:33:15Z

src/MSBuild/XMake.cs

+            {
+                Console.CancelKeyPress += Console_CancelKeyPress;
+
+                DebuggerLaunchCheck();


I'm unsure if it's better to break into the client or just wait to break into the server...it's useful to be able to debug anything that might be going wrong, but server was supposed to be mostly invisible, and that'd be visible.

Forgind · 2022-07-13T17:02:36Z

src/Build/BackEnd/Client/MSBuildClient.cs

+
+            // Now write in the actual packet length
+            memoryStream.Position = 1;
+            _binaryWriter.Write(packetStreamLength - 5);


Where did 5 come from?

this code was inspired by NodeEndpointOutOfProcBase.RunReadLoop and has meaning of size of header (byte: packet type, int32: sizeInBytes).

rainersigwald · 2022-07-13T19:16:29Z

src/Shared/UnitTests/TestEnvironment.cs


+    public class TransientTestProcess : TransientTestState
+    {
+        private readonly int _processId;


Nit: process IDs can be reused, so it'd be better to keep a Process object (which internally keeps an OS handle open so that the PID can't be reused until the Process object is cleaned up).

rainersigwald · 2022-07-13T19:21:02Z

src/MSBuild.UnitTests/MSBuildServer_Tests.cs

+    public class SleepingTask : Microsoft.Build.Utilities.Task
+    {
+        public int SleepTime { get; set; }
+
+        /// <summary>
+        /// Sleep for SleepTime milliseconds.
+        /// </summary>
+        /// <returns>Success on success.</returns>
+        public override bool Execute()
+        {
+            Thread.Sleep(SleepTime);
+            return !Log.HasLoggedErrors;
+        }
+    }
+
+    public class ProcessIdTask : Microsoft.Build.Utilities.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;
+        }
+    }


I think we have both of these in some test assembly somewhere, but they're tiny so reimplementing is probably ok.

rainersigwald · 2022-07-13T19:29:42Z

src/Build/BackEnd/Client/MSBuildClient.cs

+            {
+                try
+                {
+                    IntPtr stdOut = NativeMethodsShared.GetStdHandle(NativeMethodsShared.STD_OUTPUT_HANDLE);


TIL this is a handle you're not supposed to dispose: https://docs.microsoft.com/en-us/windows/console/getstdhandle#handle-disposal

MichalPavlik and others added 6 commits April 21, 2022 14:24

[WIP-FEATURE] MSBuild server node (#7489)

b0a788d

Removed IHanshake interface and removed duplicate code from ServerNod…

3c24e9a

…eHandshake.

Change server mutex name generationt to support posix

cdacf78

Fix appending FORCECONSOLECOLOR log parametr

e0a7bc8

Server instrumentation (#7602)

f97f022

* Some instrumentation * Add more details to ETW * Use class-wide variables

AR-May added the Area: Performance label May 18, 2022

donJoseLuis suggested changes May 18, 2022

View reviewed changes

AR-May commented May 19, 2022

View reviewed changes

src/Framework/MSBuildEventSource.cs Show resolved Hide resolved

Added cancelation feature (#7638)

5c2eead

* Added cancelation feature

Forgind suggested changes May 25, 2022

View reviewed changes

Forgind and others added 12 commits May 30, 2022 16:17

Fix control sequence emission (#7630)

5ade626

* Fix control sequence emission * Some cleanup

Solving memory leak by reusing BuildManager and ProjectRoolElementCache

8080b4c

Do not clear project root element cache if in auto reload.

e41cf8a

Reduce if

447225c

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Solving memory leak by reusing BuildManager and ProjectRoolElementCac…

836f6ef

…he (#7655) * Solving memory leak by reusing BuildManager and ProjectRoolElementCache * Do not clear project root element cache if in auto reload.

Put msbuild server feature under ChangeWave 17.4. (#7661)

88f571c

Add support for MSBUILDDEBUGONSTART env. var. for msbuild server clie…

88f3410

…nt code path. (#7668)

Fix msbuild server process launch. (#7673)

b4a3482

Merge branch 'feature/msbuild-server' of https://github.com/dotnet/ms…

c2691b1

…build into feature/msbuild-server

Added cancelation support for client. (#7659)

90d6f32

* Added cancellation support for client. * Added cancellation support for client. * Fixing wrong merge * Removed "Cancelled" exit type * Resolving comments

Fix graceful disconnection (#7701)

4b42b69

* Handle race condition * Clean running server nodes in tests.

AR-May commented Jun 16, 2022

View reviewed changes

src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs Outdated Show resolved Hide resolved

MichalPavlik and others added 2 commits June 16, 2022 19:14

Moving public types not intended to use externally to "Microsoft.Buil…

39a56d0

…d.Experimental" namespace. (#7688)

Merge branch 'feature/msbuild-server' of https://github.com/dotnet/ms…

60179f8

…build into feature/msbuild-server

AR-May mentioned this pull request Jun 20, 2022

MSBuild server V2 dotnet/sdk#26104

Merged

rokonec and others added 3 commits June 20, 2022 19:04

Merge branch 'feature/msbuild-server' of https://github.com/dotnet/ms…

c994838

…build into feature/msbuild-server

Fix arguments passing to MSBuild Server (#7723)

dec0fbc

* Send command line as string[] in dotnet builds.

MichalPavlik and others added 3 commits June 30, 2022 10:06

Fixed some commented issues (#7759)

200fbb9

* Fixed some commented issues

Merge branch 'main' into feature/msbuild-server

a55907e

benvillalobos added the Area: Engine Issues impacting the core execution of targets and tasks. label Jul 5, 2022

rokonec and others added 10 commits July 11, 2022 16:41

Added argument null checking

7a293b2

Delete dead commented code

c9a1e61

Delete dead code

072b447

prefix mutexes name by msbuild

265f3b7

Reuse ConnectToPipeStream from NodeProviderOutOfProcBase

2aa65ba

Merge branch 'feature/msbuild-server' of https://github.com/dotnet/ms…

ca8f983

…build into feature/msbuild-server # Conflicts: # src/Build/BackEnd/Client/MSBuildClient.cs

Merge branch 'main' of https://github.com/dotnet/msbuild into feature…

e45470a

…/msbuild-server # Conflicts: # src/Build/PublicAPI/netstandard/PublicAPI.Unshipped.txt

Unshipped api fix

504f720

Reduces allocations in case the tracing is not enabled (#7814)

c625fc1

* Reduces allocations in case the tracing is not enabled * Resolving comment

Fix msbuild client exit type for connection errors. (#7816)

6d00e2b

rokonec added 4 commits July 12, 2022 20:42

Emmit BuildTelemetry event (#7778)

26b43b0

Increase console refresh to 25Hz

60abe1f

Do not recover from TrySendBuildCommand failure

ae46347

Move *ConcoleConfiguration classes to its own files

9c2f0f3

rokonec approved these changes Jul 13, 2022

View reviewed changes

src/Build.UnitTests/BackEnd/RedirectConsoleWriter_Tests.cs Outdated Show resolved Hide resolved

src/Build/BackEnd/BuildManager/BuildManager.cs Show resolved Hide resolved

src/Build/BackEnd/BuildManager/BuildManager.cs Outdated Show resolved Hide resolved

rokonec added 3 commits July 13, 2022 15:44

Change test timing

aa17a3f

Add comment for graph build telemetry project

b87a5be

Fix English in comment

568e61f

rokonec requested review from Forgind and donJoseLuis July 13, 2022 13:48

donJoseLuis approved these changes Jul 13, 2022

View reviewed changes

Forgind approved these changes Jul 13, 2022

View reviewed changes

rainersigwald reviewed Jul 13, 2022

View reviewed changes

Forgind merged commit 1887d29 into main Jul 13, 2022

Forgind deleted the feature/msbuild-server branch July 13, 2022 19:37

	processStartInfo.CreateNoWindow = (creationFlags \| BackendNativeMethods.CREATENOWINDOW) == BackendNativeMethods.CREATENOWINDOW;
	processStartInfo.CreateNoWindow = (creationFlags & BackendNativeMethods.CREATENOWINDOW) == BackendNativeMethods.CREATENOWINDOW;

MSBuild server #7634

MSBuild server #7634

Uh oh!

Conversation

AR-May commented May 18, 2022 • edited by baronfel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Uh oh!

donJoseLuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Forgind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rainersigwald commented Jul 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Forgind 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

AR-May commented May 18, 2022 •

edited by baronfel

Loading