Non-blocking MSBuildClientPacketPump.#13818
Open
teo-tsirpanis wants to merge 8 commits into
Open
Conversation
Fixes test failures. There are already binding redirects in the app.config.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors the MSBuild client/server communication to use async APIs and System.Threading.Channels instead of thread + wait-handle based packet pumping.
Changes:
- Introduces async execution paths (
ExecuteAsync, async packet send/receive) and sync wrappers where needed. - Replaces
ConcurrentQueue/WaitHandle-based packet pumping with aChannel<INodePacket>-based async loop. - Adds
System.Threading.Channelspackage reference to support the new channel-based implementation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/MSBuild/MSBuildClientApp.cs | Switches client app execution to async-based implementation. |
| src/Build/Microsoft.Build.csproj | Adds System.Threading.Channels dependency for the new packet pump. |
| src/Build/BackEnd/Client/MSBuildClientPacketPump.cs | Replaces thread/event packet pump with channel + async task and async disposal. |
| src/Build/BackEnd/Client/MSBuildClient.cs | Plumbs async send/receive and consumes the channel-based packet pump. |
| /// <returns>A value of type <see cref="MSBuildApp.ExitType"/> that indicates whether the build succeeded, | ||
| /// or the manner in which it failed.</returns> | ||
| public static MSBuildApp.ExitType Execute(string[] commandLineArgs, string msbuildLocation, CancellationToken cancellationToken) | ||
| public static async Task<MSBuildApp.ExitType> ExecuteAsync(string[] commandLineArgs, string msbuildLocation, CancellationToken cancellationToken) |
Contributor
Author
There was a problem hiding this comment.
This API is not public.
Comment on lines
+85
to
+86
| public void RegisterPacketHandler(NodePacketType packetType, NodePacketFactoryMethod factory) => | ||
| _packetDeserializationMethods.Add(packetType, factory); |
Contributor
Author
There was a problem hiding this comment.
On second thought, removed the packet handler registering, in favor of accepting a deserialization callback in the constructor.
Comment on lines
+208
to
+212
| _shutdownTokenSource.Cancel(); | ||
| if (_packetPumpTask is not null) | ||
| { | ||
| await _packetPumpTask.ConfigureAwait(false); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Completed _receivedPacketsChannel. There's not much reason to dispose the shutdown token source, since we don't use it for timeouts.
| result.AsyncWaitHandle | ||
| while (true) | ||
| { | ||
| // Client recieved a packet header. Read the rest of it. |
Comment on lines
+343
to
+344
| // After the cancelation, we want to wait to server gracefuly finish the build. | ||
| // We have to replace the cancelation token, because the thrown OCE would cause to repeatedly hit this branch of code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
As part of removing
FEATURE_APM, I noticed that MSBuild uses old-style async I/O in various places, often leading to more complex code and threads being blocked. MSBuild Server's client is an easy first step towards modernizing MSBuild's async code.Changes Made
MSBuildClientPacketPumpwas updated to useasync/awaitand other modern APIs.System.Threading.Channels, instead of event wait handles.CancellationToken, instead of an event wait handle.MSBuildClientwas updated to use asynchronous I/O to send packets and retrieve them from the packet pump'sChannelReader.ExecuteAsyncpublic API; the existingExecutemethod is a sync-over-async wrapper overExecuteAsync; calling it is not recommended for performance reasons.Testing
Existing tests pass locally, modulo some failures likely due to my machine's language being set to Greek.
Notes