Harden lock-free MMF dump reader#1470
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 opt-in lock-free, memory-mapped dump reader to support concurrent memory reads and direct-span access more safely, while tightening lifetime/cleanup behavior and validating dump-controlled segment ranges.
Changes:
- Updates the lock-free MMF reader to support concurrent read/direct-span access via a striped per-thread segment cache, plus safer segment range validation and overlap handling.
- Strengthens disposal and construction-failure cleanup semantics (idempotent dispose, fail-closed after dispose, release MMF pointer/resources on failure).
- Updates public/test contract and adds stress/regression tests for concurrent reads/direct spans, invalid segments, overlap behavior, and dispose semantics.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Diagnostics.Runtime/DataTargetOptions.cs | Updates option documentation to reflect concurrent read/direct-span safety and dispose quiescence requirements. |
| src/Microsoft.Diagnostics.Runtime/DataReaders/LockFreeMmfDataReader.cs | Implements concurrency hardening, safer segment table construction/validation, overlap handling, and disposal/cleanup improvements. |
| src/Microsoft.Diagnostics.Runtime.Tests/src/MemoryReaderTests.cs | Adds concurrency and regression tests for lock-free MMF reader behavior and cleanup contracts. |
| src/Microsoft.Diagnostics.Runtime.Tests/src/Attributes/DataReaderKind.cs | Updates enum documentation to match the revised thread-safety/dispose contract. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 1
Comment on lines
+209
to
+210
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| private void ThrowIfDisposed() => ObjectDisposedException.ThrowIf(_disposed != 0, this); |
Make the existing opt-in lock-free memory-mapped dump reader safe for concurrent read/direct-span use. This adds overflow-safe segment bounds validation, filters invalid file ranges, handles overlapping virtual-address segments without losing valid memory, fails closed after disposal, makes Dispose idempotent for concurrent callers, and cleans up acquired MMF resources on constructor failure. Update the public/test contract and add focused MemoryReaderTests coverage for malformed segments, overlapping segments, use-after-dispose, constructor cleanup, and concurrent Dispose.
leculver
force-pushed
the
split/a-mmf-reader-hardening
branch
from
64b97f1 to
3c2565e
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
Hardens the existing opt-in lock-free memory-mapped dump reader for concurrent read/direct-span use.
This PR keeps the existing reader architecture, but closes correctness and lifetime gaps found during review:
Disposeidempotent for concurrent callers with an atomic disposed transitionPart 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.