Fix managed cache publication races#1472
Open
leculver wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens ClrMD’s managed caches against publication races by making several lazy-initialization and singleton publication paths thread-safe without requiring additional DAC serialization.
Changes:
- Deduplicates concurrent
ClrTypecreation by using an atomic publish-or-discard pattern inClrTypeFactory.GetOrCreateType. - Makes several lazy caches race-safe (GCDesc publication, basic type array publication/slot initialization, module type maps).
- Avoids equality paths that can trigger expensive/racy DAC-backed lookups by preferring
ReferenceEqualsfor heap singletons.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Diagnostics.Runtime/Implementation/ClrTypeFactory.cs | Deduplicates racing type creation and hardens basic-type/module fallback publication. |
| src/Microsoft.Diagnostics.Runtime/Implementation/ClrDacType.cs | Publishes GCDesc safely and switches singleton comparisons to reference equality. |
| src/Microsoft.Diagnostics.Runtime/ClrType.cs | Adjusts ClrType.Equals to avoid DAC-backed work for MT==0 types (but currently introduces a regression; see comment). |
| src/Microsoft.Diagnostics.Runtime/ClrObject.cs | Uses ReferenceEquals for ErrorType checks to avoid dispatching into ClrType.Equals. |
| src/Microsoft.Diagnostics.Runtime/ClrModule.cs | Adds lock + volatile publication for typedef/typeref maps to avoid racy ??= initialization. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 2
Comment on lines
+522
to
+523
| if (MethodTable == 0 || other.MethodTable == 0) | ||
| return false; |
Comment on lines
148
to
+157
| lock (_types) | ||
| { | ||
| if (_types.TryGetValue(mt, out ClrType? winner)) | ||
| { | ||
| ApplyDeferredComponentType(winner, obj); | ||
| return winner; | ||
| } | ||
|
|
||
| _types[mt] = result; | ||
| } |
Make ClrMD-owned managed caches safe under concurrent inspection. Avoid torn struct publication for ClrDacType.GCDesc, deduplicate ClrTypeFactory races, publish singleton/error-module state atomically, and use reference equality for singleton type comparisons so equality checks do not accidentally dispatch into DAC-backed type/name work. This change is intentionally separate from DAC serialization; it fixes managed cache publication and identity semantics.
leculver
force-pushed
the
split/c-managed-cache-publication
branch
from
15b8c1a to
1fc3a1d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes ClrMD-owned managed cache publication races that do not require DAC serialization.
This PR keeps the change focused on managed object/cache safety:
ClrDacType.GCDescClrTypeFactorytype creationPart of #420.
Validation
dotnet build src/Microsoft.Diagnostics.Runtime/Microsoft.Diagnostics.Runtime.csproj --framework net10.0-- passed with 0 warnings and 0 errors.lockfree-mmf-threadsafe; all non-stress paths matched the original combined branch.