Make Razor tag helper resolution safe for concurrent mixed inputs - #85167
Conversation
Classic rzc processes .cshtml and .razor inputs in parallel through a shared RazorProjectEngine. A phase-wide resolver field lets one document overwrite another document's file-kind strategy, producing component IR for a legacy code target. Keep resolver selection in the per-execution resolution context and pass it through fallback unwrapping. Add a mixed MVC/component concurrency test and document that phase instances cannot own per-document mutable state. Fixes dotnet#85132 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d07d252c-d5c2-43ae-ae7c-e56c4624e953
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The new concurrency regression test uses a very high-iteration Parallel.For loop that is likely to be unnecessarily expensive and may increase CI flakiness/timeouts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR removes per-document mutable state from DefaultTagHelperResolutionPhase by keeping resolver selection local to each ExecuteCore invocation and flowing it through the resolution context and unwrap path, making tag helper resolution safe when a single RazorProjectEngine processes mixed legacy (.cshtml) and component (.razor) inputs concurrently.
Changes:
- Eliminates the shared
_resolverfield and threads the chosenTagHelperResolverthroughResolutionContextandUnwrapAllElements. - Updates resolution/unwrap call sites to use the per-execution resolver rather than shared instance state.
- Adds a regression test exercising concurrent mixed file kinds, and documents the Razor engine concurrency constraint in repo instructions.
File summaries
| File | Description |
|---|---|
| src/Razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultTagHelperResolutionPhase.cs | Moves resolver selection to an execution-local variable and passes it through context/unwrap to avoid cross-thread contamination. |
| src/Razor/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/DefaultTagHelperResolutionPhaseTest.cs | Adds a concurrent mixed-input regression test to validate correct per-file-kind output under shared-engine processing. |
| .github/instructions/Razor.instructions.md | Documents that phases/passes are shared across concurrent document processing and must not store per-document state in mutable fields. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
Match classic rzc's four-way parallelism and run 2,000 mixed document iterations. This preserves a wide overlap window for the timing-sensitive race while keeping the test cheaper on constrained agents. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d07d252c-d5c2-43ae-ae7c-e56c4624e953
There was a problem hiding this comment.
🟢 Approval recommended
The changes eliminate shared mutable state in the phase (addressing the concurrency hazard) and include targeted regression coverage, with only a minor readability nit noted.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultTagHelperResolutionPhase.cs:59
- The root call to
UnwrapAllElementspassesdocumentNodeboth as the node and as the optionaldocumentNodeparameter. Since the method already setsdocumentNodewhennodeis aDocumentIntermediateNode, the third argument is redundant and slightly obscures intent.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Thank you, looks like this is good to go? This would be helpful to have as it transitively delays progress for dotnet/sdk#56126 |
Classic
rzcprocesses.cshtmland.razorinputs concurrently through oneRazorProjectEngine.DefaultTagHelperResolutionPhasekept its per-document resolver in a shared field, allowing a component execution to overwrite a legacy execution and produce component IR for a legacy code target. Mixed projects can then fail withThis writer does not support components.Resolver selection now stays within each phase execution and flows through the existing resolution context and fallback-unwrapping path. The existing phase ordering and markup-split behavior remain unchanged.
Coverage uses the Razor 5.0 / MVC 3.0 host shape to process legacy and component documents concurrently through one engine and verifies that each retains its file-kind-specific output.
Fixes #85132
Microsoft Reviewers: Open in CodeFlow