Skip to content

Remove APM use outside of TaskHost node provider / endpoint #11800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ccastanedaucf
Copy link
Contributor

Switches preprocessor directives to allow .NET Framework to use TPL-based concurrency over the old APM model. Since MSBuildClientPacketPump and NodeProviderOutOfProcBase never run on the TaskHost, this allows removing a chunk of (now) dead code.

(3rd part of re-attempting #11546)

@Copilot Copilot AI review requested due to automatic review settings May 7, 2025 03:03
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes legacy APM code paths from non‐TaskHost builds in favor of TPL‐based concurrency while retaining APM only when running on a TaskHost. Key changes include adjusting preprocessor directives from FEATURE_APM/NET451_OR_GREATER to TASKHOST, removing large blocks of obsolete APM code, and updating asynchronous read logic for consistency.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/Shared/NodeEndpointOutOfProcBase.cs Updated preprocessor directives and logic to use TPL-based concurrency when not in TaskHost, and switch to APM only in TaskHost scenarios.
src/Shared/CommunicationsUtilities.cs Modified async read method conditions to align with the new TASKHOST flag and .NET conditional compilation patterns.
src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs Removed legacy APM handling methods and integrated minimal logging for NETFRAMEWORK in the packet read loop.
src/Build/BackEnd/Client/MSBuildClientPacketPump.cs Eliminated FEATURE_APM branches in favor of TPL-based reads for non-TaskHost configurations.
Comments suppressed due to low confidence (3)

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

  • [nitpick] The removal of the FEATURE_APM branch here is appropriate; please confirm that all TaskHost-specific scenarios are covered and legacy APM code is no longer expected in other execution contexts.
public void BeginAsyncPacketRead() { ... }

src/Build/BackEnd/Client/MSBuildClientPacketPump.cs:204

  • [nitpick] Ensure that the removal of FEATURE_APM branches here is coordinated with the overall TASKHOST strategy so that TPL-based concurrency is seamlessly adopted in non-TaskHost builds.
Task<int> readTask = CommunicationsUtilities.ReadAsync(localStream, headerByte, headerByte.Length).AsTask();

src/Shared/CommunicationsUtilities.cs:589

  • [nitpick] Verify that switching the condition to '!TASKHOST' correctly excludes TaskHost builds from using the TPL-based ReadAsync logic, aligning with the overall design.
#if !TASKHOST

@@ -21,7 +21,7 @@
using System.Security.Principal;

#endif
Copy link
Preview

Copilot AI May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider adding an inline comment here to clarify that '!TASKHOST' triggers TPL-based concurrency and that APM logic is reserved for TaskHost builds.

Suggested change
#endif
#endif
// The '!TASKHOST' directive triggers TPL-based concurrency by including System.Threading.Tasks.
// APM (Asynchronous Programming Model) logic is reserved for TaskHost builds.

Copilot uses AI. Check for mistakes.

@@ -368,7 +368,7 @@ private void PacketPumpProc()
try
{
// Wait for a connection
Copy link
Preview

Copilot AI May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Ensure that TASKHOST is defined only for TaskHost scenarios since it now controls the use of legacy APM code in this section.

Suggested change
// Wait for a connection
// Wait for a connection
// Ensure TASKHOST is defined only for TaskHost scenarios, as it controls the use of legacy APM code.

Copilot uses AI. Check for mistakes.

@SimaTian
Copy link
Member

SimaTian commented May 7, 2025

Looks good to me. Removing ancient way of doing stuff in favor of the more modern one.
Just to be extra safe (since we had some issues before) I will run our battery of perf tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants