Document threading test purpose and intentional non-disposal#4106
Open
mattleibow wants to merge 1 commit into
Open
Document threading test purpose and intentional non-disposal#4106mattleibow wants to merge 1 commit into
mattleibow wants to merge 1 commit into
Conversation
The x86 OOM fix for these GC/finalizer stress tests already landed in #3674. This adds comments documenting WHY the tests are written the way they are so future readers don't "fix" them incorrectly: - SKBitmapThreadingTest.ImageScalingMultipleThreadsTest intentionally leaves native objects undisposed so they are reclaimed only by the GC/finalizer. Adding using/Dispose would defeat the purpose of the test. - SKObjectTest.EnsureMultithreadingDoesNotThrow documents the concurrent decode stress purpose and the x86 address-space skip. No behavioral change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
📦 Try the packages from this PRWarning Do not run these scripts without first reviewing the code in this PR. Step 1 — Download the packages bash / macOS / Linux: curl -fsSL https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.sh | bash -s -- 4106PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4106"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4106/packages --name skiasharp-pr-4106More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4106 |
Contributor
|
📖 Documentation Preview The documentation for this PR has been deployed and is available at: 🔗 View Staging Site This preview will be updated automatically when you push new commits to this PR. This comment is automatically updated by the documentation staging workflow. |
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
1 participant
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
The x86 .NET Framework OOM fix for the GC/finalizer threading stress tests already landed on
mainin #3674. However, the tests don't explain why they are written the way they are, which has led to well-intentioned-but-wrong attempts to "fix" them by adding disposal.This is a comment-only change (no behavioral change) that documents the intent:
SKBitmapThreadingTest.ImageScalingMultipleThreadsTestintentionally creates native SkiaSharp objects (SKBitmap/SKImage/SKData) and does NOT dispose them, so they are reclaimed only by the GC and finalizer thread. That is the whole point — it verifies SkiaSharp objects can be finalized safely from many threads concurrently. ADo NOT add using/Disposewarning is added so future readers don't short-circuit finalization and defeat the test.SKObjectTest.EnsureMultithreadingDoesNotThrowgets a purpose comment describing the concurrent-decode stress and the existing x86 address-space skip.Context
The companion fix for
release/3.119.x(which did not yet have #3674) is #4105. This PR ensures bothmainand the release branch carry the same explanatory comments.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com