-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix missing Dispose override in SizeOptIListSelectIterator and SizeOptIListWhereIterator #122729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add Dispose() method to properly dispose the enumerator when UseSizeOptimizedLinq is enabled. This fixes the bug where custom enumerator's Dispose was not called when the source implements IList<T>. Fixes #115239 Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-linq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a regression in .NET 10 where SizeOptIListSelectIterator (used when UseSizeOptimizedLinq=true) failed to dispose custom enumerators, breaking user code that relies on disposal for cleanup or invariant tracking.
Key Changes:
- Added missing
Dispose()override toSizeOptIListSelectIteratormatching the existing pattern inIListSelectIterator - Added comprehensive tests to verify enumerator disposal in both completion and explicit disposal scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Linq/src/System/Linq/Select.SizeOpt.cs | Added Dispose() override to properly clean up _enumerator reference |
| src/libraries/System.Linq/tests/SelectTests.cs | Added two tests and a helper class to validate enumerator disposal behavior |
|
SizeOptIListWhereIterator is also missing a Dispose override. It's a bit inconvenient that the PR is not linked to the issue. |
|
@copilot, please search for any other missing dispose overrides. |
Found and fixed another missing Dispose override in Where.SizeOpt.cs for SizeOptIListWhereIterator. Added tests for both Select and Where disposal behavior with IList sources. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Searched all iterator classes with All other iterators with enumerator fields ( |
|
Consider creating a |
| Assert.Equal(1, source.DisposeCalls); | ||
| } | ||
|
|
||
| private sealed class DisposeTrackingList<T> : IList<T>, IReadOnlyList<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, move this to the shared EnumerableTests file and remove the duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved DisposeTrackingList to EnumerableTests.cs and removed duplicates from both test files in c3487f5.
|
|
||
| public override void Dispose() | ||
| { | ||
| if (_enumerator is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, please restructure these as:
if (_enumerator is {} e)
{
_enumerator = null;
e.Dispose()
}
base.Dispose();There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restructured both Dispose methods (in Select.SizeOpt.cs and Where.SizeOpt.cs) to use the requested pattern in c3487f5.
- Restructured Dispose() methods to use `if (_enumerator is {} e)` pattern
- Moved DisposeTrackingList class to shared EnumerableTests.cs file
- Removed duplicate DisposeTrackingList from SelectTests.cs and WhereTests.cs
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
main PR
Description
Two iterator classes (used when
UseSizeOptimizedLinq=true) hold_enumeratorreferences but did not overrideDispose()to clean them up. Custom enumerators never hadDisposecalled, breaking user code that relies on disposal for cleanup/invariant tracking.Fixed iterators:
SizeOptIListSelectIteratorinSelect.SizeOpt.csSizeOptIListWhereIteratorinWhere.SizeOpt.csAdded the missing
Dispose()overrides using the recommended pattern:Also added shared
DisposeTrackingList<T>test helper toEnumerableTests.csfor testing enumerator disposal behavior.Customer Impact
User code with custom
IList<T>implementations that track enumerator lifecycle (e.g., detecting concurrent modification) will fail silently or throw whenUseSizeOptimizedLinq=true. This breaks real-world projects like ICSharpCode.Decompiler.Regression
Yes. Works in .NET 9 and earlier. Broke in .NET 10 when
SizeOptIListSelectIteratorandSizeOptIListWhereIteratorwere introduced without theDisposeoverrides.Testing
Select_SourceIsIList_EnumeratorDisposedOnCompleteandSelect_SourceIsIList_EnumeratorDisposedOnExplicitDisposetestsWhere_SourceIsIList_EnumeratorDisposedOnCompleteandWhere_SourceIsIList_EnumeratorDisposedOnExplicitDisposetestsDisposeTrackingList<T>helper class toEnumerableTests.csRisk
Low. Single-method additions following established patterns from
IListSelectIteratorandIEnumerableWhereIterator. No behavior change for correctly-behaving enumerators.Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Original prompt
This section details on the original issue you should resolve
<issue_title>Dispose of custom enumerator not getting called if
UseSizeOptimizedLinqis used</issue_title><issue_description>### Description
Context
Initially this was noticed in a .NET 10
dotnet run .csapp that was using the ICSharpCode.Decompiler project, where we have sanity checks in our data structures to avoid certain modifications while the data is enumerated.Analysis
dotnet run .csdotnet run .csapp.System.Linq.Enumerable.SizeOptIListSelectIterator2appears in the stack, when problem occurs, butSystem.Linq.Enumerable.IListSelectIterator2appears in the stack, when it doesn't occur.System.Linq.Enumerable.IsSizeOptimized, which is feature flag that causes theSizeOptIListSelectIteratorto be used, which causes the bug.UseSizeOptimizedLinqto false in the csproj or using#:propertyin thedotnet run .csapp.Reproduction Steps
Reproducer: