Fix VSD cache lock ordering during unload - #133213
Conversation
Release the dispatch cache lock before deleting loader heaps, which acquire the executable allocator lock. Add a collectible AssemblyLoadContext regression test that exercises enough interface dispatch entries to reproduce the checked-build assertion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dafdc384-5b50-4f91-a024-00a1dafbc0d3
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes low-level CoreCLR VM locking behavior during unload/heap teardown, which merits final human review despite the targeted fix and added regression test.
Pull request overview
Fixes a CoreCLR VM lock ordering problem during collectible AssemblyLoadContext unload by ensuring the VSD cache lock is not held while freeing loader heaps that may take the executable allocator lock, and adds a regression test to exercise the scenario.
Changes:
- Restrict the lifetime of the VSD resolve-cache write lock in
VirtualCallStubManager::~VirtualCallStubManager()so it’s released before heap teardown. - Add a collectible-ALC regression test that grows the VSD cache entry heap and forces unload/GC to validate the lock ordering does not assert.
- Add the corresponding test project configuration under the CoreCLR regressions suite.
File summaries
| File | Description |
|---|---|
| src/coreclr/vm/virtualcallstub.cpp | Scopes the resolve-cache lock to the unlink loop so it’s released before deleting loader heaps. |
| src/tests/Regressions/coreclr/GitHub_132982/test132982.cs | New xUnit regression test that loads the assembly into a collectible ALC, exercises VSD cache growth, and forces unload/GC. |
| src/tests/Regressions/coreclr/GitHub_132982/test132982.csproj | New test project wiring (priority, isolation/unloadability flags, and compilation inputs). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
Allow the regression test to run in the merged test process and under GC stress. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dafdc384-5b50-4f91-a024-00a1dafbc0d3
There was a problem hiding this comment.
🔵 Needs a closer look
The new regression test project needs RequiresProcessIsolation to match UnloadabilityIncompatible, and the test should assert it runs in the intended collectible AssemblyLoadContext to keep coverage reliable.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/tests/Regressions/coreclr/GitHub_132982/test132982.cs:162
- To make the regression test more robust, assert that the reflection-loaded assembly actually ended up in the collectible
AssemblyLoadContextyou created, and fail with a clear error ifDispatcher.Runcan’t be found. Otherwise, a future resolution change could accidentally runDispatcher.Runfrom the default context and the test would no longer cover the intended unload path.
src/tests/Regressions/coreclr/GitHub_132982/test132982.csproj:8 - The project sets
UnloadabilityIncompatible, which (perdocs/workflow/testing/coreclr/requiresprocessisolation.md) requiresRequiresProcessIsolation=trueso the per-process compatibility skip can work when tests are merged into a shared runner. Without it, the test may still run underRunInUnloadableContextlegs and behave unpredictably.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
## Summary - release the virtual stub dispatch cache lock before deleting loader heaps that acquire the executable allocator lock - add a collectible `AssemblyLoadContext` regression test that grows the VSD cache entry heap and exercises the lock-order failure Fixes dotnet#132982 ## Testing - `build.cmd clr -rc checked` - `src\tests\build.cmd -Test Regressions\coreclr\GitHub_132982\test132982.csproj x64 checked -Priority 1` - confirmed the regression test triggers the `CrstStubDispatchCache` / `CrstExecutableAllocatorLock` level violation without the fix and passes with the fix > [!NOTE] > This pull request description was generated with GitHub Copilot. --------- Copilot-Session: dafdc384-5b50-4f91-a024-00a1dafbc0d3
…33213) (#134159) Backport of #133213 to release/11.0 ## Customer Impact - [ ] Customer reported - [x] Found internally The PR fixes a lock issue which occurs in the VirtualCallStubManager destructor when cleaning up an AssemblyLoadContext. It is a regression for .NET 11 that originates in #128868 where a new lock aquisition was added for `g_resolveCache`. ## Regression - [x] Yes - [ ] No ## Testing A regression test was added that successfully reproduced the issue, which is now passing with the fix. ## Risk Low. Copilot-Session: dafdc384-5b50-4f91-a024-00a1dafbc0d3
Summary
AssemblyLoadContextregression test that grows the VSD cache entry heap and exercises the lock-order failureFixes #132982
Testing
build.cmd clr -rc checkedsrc\tests\build.cmd -Test Regressions\coreclr\GitHub_132982\test132982.csproj x64 checked -Priority 1CrstStubDispatchCache/CrstExecutableAllocatorLocklevel violation without the fix and passes with the fixNote
This pull request description was generated with GitHub Copilot.