Don't disable the MSBuild server for /mt builds when node reuse is off - #14161
Don't disable the MSBuild server for /mt builds when node reuse is off#14161AR-May wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts MSBuild’s server-engagement logic so that multithreaded (/mt) builds can still run through MSBuild Server (and therefore benefit from Server GC) even when node reuse is disabled (-nr:false / MSBUILDDISABLENODEREUSE=1). It introduces a “shut down after build” behavior to honor the no-reuse intent while still using the server for /mt.
Changes:
- Decouples “can run on server” from “may keep server resident”:
/mtcan use server even when node reuse is disabled, with a newshutdownServerAfterBuildflag. - Plumbs
shutdownServerAfterBuildthroughXMake→MSBuildClientAppand requests server shutdown after the build completes. - Adds a unit test verifying
/mt -nr:falseuses the server but does not reuse it across builds.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/MSBuild/XMake.cs | Computes shutdownServerAfterBuild and allows /mt to engage the server even when node reuse is disabled. |
| src/MSBuild/MSBuildClientApp.cs | Adds a shutdown-after-build option and triggers server shutdown post-build when requested. |
| src/MSBuild.UnitTests/MSBuildServer_Tests.cs | Adds test coverage for /mt server usage + post-build shutdown behavior when node reuse is disabled. |
| if (shutdownServerAfterBuild) | ||
| { | ||
| MSBuildClient.ShutdownServer(CancellationToken.None); | ||
| } |
There was a problem hiding this comment.
It should not hang. But this is really best effort rather than guarantied shutdown - there is two connections to the server between which another client can theoretically manage to take over the server. In this case shutdown will timeout and be skipped. I wanted to avoid changing the communication protocol, but it seems it is a better way to ensure the shutdown. I will open another PR.
JanProvaznik
left a comment
JanProvaznik
left a comment
There was a problem hiding this comment.
code logic ok, maintainability comments
| if (shutdownServerAfterBuild) | ||
| { | ||
| MSBuildClient.ShutdownServer(CancellationToken.None); | ||
| } |
|
|
||
| // Perform the single authoritative command-line parse for this process. It yields: | ||
| // - canRunServer: whether switches (help/version/binlog/nodereuse/...) permit the server; | ||
| // - canRunServer: whether switches (help/version/binlog/nodemode/...) permit the server; |
There was a problem hiding this comment.
nodemode switch does not make sense in this comment
| !ProcessNodeReuseSwitch(commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.NodeReuse])) | ||
| FileUtilities.IsBinaryLogFilename(projectFile); | ||
|
|
||
| // Node reuse being disabled normally disqualifies the server because the server is a persistent |
There was a problem hiding this comment.
I think this needs a full documentation reframing of the server process rather than having bunch of comments "this is a server but..."
OvesN
left a comment
OvesN
left a comment
There was a problem hiding this comment.
Just have one comment, otherwise LGTM
#14248) Fixes #14157 ### Context When node reuse is disabled (`-nr:false`), MSBuild refused to use the server node. For a multithreaded ( /mt ) build that's a problem: the server is the only way to get Server GC, which /mt builds depend on for performance reasons. ### Changes Made Decouple "may we use the server for this build?" from "may the server stay resident afterward?": - /mt now uses the server even when node reuse is disabled, purely to obtain Server GC. - To honor the no-reuse intent, a new `shutdownServerAfterBuild` flag tears the server down immediately after the build completes, so it doesn't persist and each build gets a fresh process. - Node reuse being off still disqualifies the server for non- /mt builds (unchanged behavior). ### Testing Added a unit test. ### Notes Previous attempt: #14161. It tries to shut down the server after the build by issuing a shutdown command. However, previous implementation does not guarantee that the server will not accept another client's request in the meantime and subsequently fail to shut down. Another option would be to re-parse the command line and identify cases where a server shutdown is required, but I would prefer the initial parsing to remain the single source of truth and avoid re-parsing. Client and server are the same handshake-gated MSBuild version, so there's no cross-version serialization concern over the communication protocol.
Fixes #14157
Context
When node reuse is disabled (
-nr:false), MSBuild refused to use the server node. For a multithreaded ( /mt ) build that's a problem: the server is the only way to get Server GC, which /mt builds depend on for performance reasons.Changes Made
Decouple "may we use the server for this build?" from "may the server stay resident afterward?":
shutdownServerAfterBuildflag tears the server down immediately after the build completes, so it doesn't persist and each build gets a fresh process.Testing
Added a unit test.