Handle RequestCores returning 0 in TaskHostFactory tasks#125276
Handle RequestCores returning 0 in TaskHostFactory tasks#125276
Conversation
MSBuild PR dotnet/msbuild#13306 changed OutOfProcTaskHostNode.RequestCores from throwing NotImplementedException to returning 0 (and logging MSB5022) when callbacks aren't supported. This broke four tasks that only caught NotImplementedException: - EmccCompile: caused blazorwasm publish failure (dotnet/sdk#53300) - MonoAOTCompiler, ILStrip, EmitBundleBase: same latent bug The failure chain was: 1. RequestCores returns 0 (no exception thrown) 2. allowedParallelism becomes 0 3. Parallel.ForEach with MaxDegreeOfParallelism=0 throws 4. ReleaseCores(0) in finally block throws ArgumentOutOfRangeException Fix: track cores actually acquired separately. If RequestCores returns 0 or throws any exception, fall back to the original parallelism and skip ReleaseCores. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e2ac77e to
d58e6e4
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a regression in MSBuild out-of-proc task hosting where IBuildEngine9.RequestCores(...) can return 0 (instead of throwing), which previously led to invalid Parallel* configuration and invalid ReleaseCores(0) calls in several WASM-related tasks.
Changes:
- Track cores actually acquired via a new
coresAcquiredvariable instead of overwritingallowedParallelismunconditionally. - Only override
allowedParallelismwhenRequestCores(...)returns a positive value. - Only call
ReleaseCores(...)when a positive number of cores was acquired.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/tasks/WasmAppBuilder/EmccCompile.cs | Make RequestCores/ReleaseCores robust to RequestCores returning 0 and avoid releasing 0 cores. |
| src/tasks/MonoTargetsTasks/ILStrip/ILStrip.cs | Same robustness changes for ILStrip’s parallel execution. |
| src/tasks/MonoTargetsTasks/EmitBundleTask/EmitBundleBase.cs | Same robustness changes for EmitBundle’s parallel resource emission. |
| src/tasks/AotCompilerTask/MonoAOTCompiler.cs | Same robustness changes for MonoAOTCompiler’s parallel AOT compilation loop. |
| catch | ||
| { | ||
| // RequestCores may not be supported in all host environments |
There was a problem hiding this comment.
Avoid a bare catch here. It suppresses all exceptions from RequestCores without any trace, which can hide real problems. Prefer catch (Exception ex) (or a small set of expected exceptions) and log at Low importance before falling back.
| catch | |
| { | |
| // RequestCores may not be supported in all host environments | |
| catch (Exception ex) | |
| { | |
| // RequestCores may not be supported in all host environments | |
| Log.LogMessage( | |
| MessageImportance.Low, | |
| "RequestCores failed; falling back to default parallelism. Exception: {0}", | |
| ex); |
| catch | ||
| { | ||
| // RequestCores may not be supported in all host environments |
There was a problem hiding this comment.
Avoid a bare catch around RequestCores. Catching everything without logging can mask unexpected failures and complicate diagnosing build issues. Prefer catching Exception ex (or known exceptions) and log at Low importance before proceeding with the fallback parallelism.
| catch | |
| { | |
| // RequestCores may not be supported in all host environments | |
| catch (Exception ex) | |
| { | |
| // RequestCores may not be supported in all host environments | |
| Log.LogMessage( | |
| MessageImportance.Low, | |
| "RequestCores({0}) failed: {1}", | |
| allowedParallelism, | |
| ex.Message); |
| catch | ||
| { | ||
| // RequestCores may not be supported in all host environments |
There was a problem hiding this comment.
Avoid a bare catch here. Swallowing all exceptions from RequestCores without logging can hide real issues and make failures harder to debug. Prefer catch (Exception ex) (or known expected exceptions) and log at Low importance before falling back to the original parallelism.
| catch | |
| { | |
| // RequestCores may not be supported in all host environments | |
| catch (Exception ex) | |
| { | |
| // RequestCores may not be supported in all host environments | |
| Log?.LogMessage(MessageImportance.Low, $"RequestCores failed; falling back to default parallelism. Exception: {ex}"); |
| int allowedParallelism = DisableParallelAot ? 1 : Math.Min(_assembliesToCompile.Count, Environment.ProcessorCount); | ||
| int coresAcquired = 0; | ||
| IBuildEngine9? be9 = BuildEngine as IBuildEngine9; | ||
| try | ||
| { | ||
| if (be9 is not null) | ||
| allowedParallelism = be9.RequestCores(allowedParallelism); | ||
| } | ||
| catch (NotImplementedException) | ||
| if (be9 is not null) | ||
| { |
There was a problem hiding this comment.
This RequestCores/ReleaseCores + fallback pattern is now duplicated across several tasks (EmccCompile, ILStrip, EmitBundleBase, MonoAOTCompiler). Since these task projects already compile in src/tasks/Common/Utils.cs, consider extracting a small shared helper (e.g., a method that returns (allowedParallelism, coresAcquired) and handles the fallback) to reduce future drift and make it easier to adjust behavior consistently if MSBuild changes again.
| catch | ||
| { | ||
| // RequestCores may not be supported in all host environments |
There was a problem hiding this comment.
Avoid a bare catch here. This will swallow unexpected exceptions from RequestCores (e.g., MSBuild bugs / environment failures) without any logging, which can make build failures very hard to diagnose. Catch Exception ex (or a narrower set of expected exceptions) and consider logging at Low importance before falling back.
| catch | |
| { | |
| // RequestCores may not be supported in all host environments | |
| catch (Exception ex) | |
| { | |
| // RequestCores may not be supported in all host environments | |
| Log.LogMessage(MessageImportance.Low, $"RequestCores({allowedParallelism}) failed, falling back to default parallelism. Exception: {ex.Message}"); |
Summary
Fixes dotnet/sdk#53300 — publishing Blazor WebAssembly apps fails after installing
wasm-toolsworkload with SDK 11.0.100-preview.2.26154.117.Root Cause
MSBuild PR dotnet/msbuild#13306 (merged March 2) changed
OutOfProcTaskHostNode.RequestCoresfrom throwingNotImplementedExceptionto returning 0 and logging MSB5022 when callbacks aren't supported. Four tasks in this repo only caughtNotImplementedException, causing:RequestCoresreturns 0 → no exception thrown → catch block not triggeredallowedParallelismbecomes 0Parallel.ForEachwithMaxDegreeOfParallelism=0throwsArgumentOutOfRangeExceptionReleaseCores(0)in the finally block throwsArgumentOutOfRangeException('coresToRelease')(new input validation requires > 0)Fix
Track cores actually acquired in a separate
coresAcquiredvariable. IfRequestCoresreturns 0 or throws any exception, fall back to the original parallelism value and skipReleaseCores. This is resilient to any future changes in MSBuild's task host behavior.Affected Tasks
EmccCompile(triggered the user-facing regression)MonoAOTCompiler(same latent bug)ILStrip(same latent bug)EmitBundleBase(same latent bug)