Removed WMI due to performance#13610
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the Windows WMI-based implementation used to retrieve a process command line (to address significant performance regressions during node reuse scanning), and updates unit tests to reflect that command-line retrieval is not supported on Windows.
Changes:
- Removed the Windows WMI/COM-based command-line retrieval path from
ProcessExtensions.TryGetCommandLine. - Treated Windows as unsupported for command-line retrieval (returns
nullcommand line) and updated unit tests accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Framework/Utilities/ProcessExtensions.cs | Removes WMI-based Windows command-line retrieval to avoid the perf hit. |
| src/Utilities.UnitTests/ProcessExtensions_Tests.cs | Updates expectations on Windows to assert commandLine is null. |
| try | ||
| { | ||
| #if NET | ||
| if (NativeMethods.IsWindows) | ||
| { | ||
| commandLine = Windows.GetCommandLine(process.Id); | ||
| return true; | ||
| } | ||
| else if (NativeMethods.IsOSX || NativeMethods.IsBSD) | ||
| if (NativeMethods.IsOSX || NativeMethods.IsBSD) | ||
| { | ||
| commandLine = BSD.GetCommandLine(process.Id); | ||
| return true; |
There was a problem hiding this comment.
TryGetCommandLine now treats Windows/other platforms as unsupported (returns true with commandLine == null). However, the method's XML doc says it returns false when the platform doesn't support command-line retrieval, and the unsupported-OS branch comment says "return false" even though it returns true. Please align the docs/comments with the actual contract (or adjust the return value/contract consistently across callers).
| // Windows is treated as an unsupported OS for command-line retrieval; commandLine is null. | ||
| commandLine.ShouldBeNull(); |
There was a problem hiding this comment.
PR description says "Windows is treated as unsupported OS", but the actual code change only makes Windows unsupported for command-line retrieval (TryGetCommandLine returns null). Please clarify the PR description (or comments) to avoid implying MSBuild no longer supports Windows overall.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correctness & Edge Cases — one issue found
Comment/code mismatch (line 78): The comment says "return false" but the code returns true. This is not a runtime bug — the true + null return is correct for the callers — but the comment directly contradicts the code it describes.
Everything else checks out:
- Callers handle
(true, null)correctly. Both call sites inNodeProviderOutOfProcBase.csexplicitly guardif (commandLine is null)and include the process, which is the right fallback when command-line retrieval is unsupported. ExtractFromCommandLineis null-safe. It checksstring.IsNullOrWhiteSpaceup front, so even if a BSD/Linux helper returnednull, callers would not crash (though they'd getNodeMode? == nulland exclude the process — thenullguard in callers fires first anyway).- XML doc
<returns>is slightly misleading (says "true if successfully retrieved" buttruealso means "unsupported platform"), however this is a pre-existing issue — the .NET Framework path already returned(true, null)before this PR.
Generated by Expert Code Review (on open) for issue #13610 · ● 11.7M
Fixes #13550
Context
WMI is causing issues, so we need to remove it now and find a better solution.
Changes Made
WMI code was removed and Windows is treated as unsupported OS.
Testing
Manual testing with running MSBuild worker nodes.
Notes