Rework the lifecycle of singleton instances

[ramezgerges]

Description of Change

Bugs Fixed

• Fixes [BUG] SKFontManager.Default throws an exception #3817

API Changes

None.

Behavioral Changes

None.

Required skia PR

None.

PR Checklist

• Has tests (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Merged related skia PRs
• Changes adhere to coding standard
• Updated documentation

[github-actions]

📦 Try the packages from this PR

Warning

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 -- 4080

PowerShell / Windows:

iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4080"

Step 2 — Add the local NuGet source

dotnet nuget add source ~/.skiasharp/hives/pr-4080/packages --name skiasharp-pr-4080

More options

Option	Description
--successful-only / -SuccessfulOnly	Only use successful builds
--force / -Force	Overwrite previously downloaded packages
--list / -List	List available artifacts without downloading
--build-id ID / -BuildId ID	Download from a specific build

Or download manually from Azure Pipelines — look for the nuget artifact on the build for this PR.

Remove the source when you're done:

dotnet nuget remove source skiasharp-pr-4080

[mattleibow]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[Copilot]

Pull request overview

This PR reworks how SkiaSharp’s “singleton” managed wrappers are created and protected from disposal, primarily to avoid static-constructor ordering cycles (fixing #3817) and to make singleton promotion/disposal race-safe under the HandleDictionary lock.

Changes:

• Replace eager singleton pre-registration with lazy initialization (per-type cached singletons), plus a module initializer to run the native-library compatibility check at assembly load.
• Strengthen HandleDictionary / disposal concurrency semantics (recursive lock policy + serialized “dispose-protected” promotion).
• Update tests and GC fixture logic to reflect the new “dispose-protected” singleton model and ownership-transfer behavior.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/Tests/SkiaSharp/SKTypefaceTest.cs	Updates ownership-transfer expectations (stream wrapper is disposed after transfer).
tests/Tests/SkiaSharp/SKManagedStreamTest.cs	Aligns tests with new transfer/disposal behavior and handle dictionary visibility.
tests/Tests/SkiaSharp/SKColorSpaceTest.cs	Updates singleton assertions to check identity + dispose-protection instead of type name.
tests/Tests/SkiaSharp/SKColorFilterTest.cs	Same as above for color-filter singletons.
tests/Tests/SkiaSharp/SKCodecTest.cs	Updates stream ownership-transfer behavior assertions.
tests/Tests/GarbageCleanupFixture.cs	Treats IgnorePublicDispose objects as expected long-lived singletons.
binding/SkiaSharp/SKTypeface.cs	Converts Default/Empty to lazy singletons and introduces dispose-protected singleton retrieval.
binding/SkiaSharp/SKPaint.cs	Lazily initializes the default font template and marks it dispose-protected.
binding/SkiaSharp/SKObject.cs	Adds dispose-protected object factory path; refactors disposal/ownership-transfer mechanics into SKNativeObject.
binding/SkiaSharp/SkiaSharpModuleInitializer.cs	Adds [ModuleInitializer] native-library version compatibility check (with polyfill).
binding/SkiaSharp/SKFontStyle.cs	Eagerly initializes style singletons as dispose-protected without HandleDictionary registration.
binding/SkiaSharp/SKFontManager.cs	Makes Default lazy and updates stream ownership-transfer to native.
binding/SkiaSharp/SKData.cs	Makes Empty lazy and dispose-protected.
binding/SkiaSharp/SKColorSpace.cs	Makes sRGB/sRGBLinear singletons lazy and dispose-protected.
binding/SkiaSharp/SKColorFilter.cs	Makes gamma filter singletons lazy and dispose-protected.
binding/SkiaSharp/SKCodec.cs	Updates stream ownership-transfer call site.
binding/SkiaSharp/SKBlender.cs	Uses dispose-protected wrappers for blend-mode singleton map.
binding/SkiaSharp/PlatformLock.cs	Enables recursive read/write lock behavior to support new disposal locking strategy.
binding/SkiaSharp/HandleDictionary.cs	Adds dispose-protected promotion within the HandleDictionary critical section.

[mattleibow]

Really nice work on this @ramezgerges — the new disposeProtected flow plus making IgnorePublicDispose a one-way latch is a much cleaner mental model than the old SKxxxStatic subclass dance, and the inline comments explaining the race reasoning are genuinely a pleasure to read 🙌

I took a thorough pass and have a few things to flag before merge. None of these change the overall design — they're mostly small fixes plus some test-coverage suggestions.

🔴 Build is currently broken

binding/SkiaSharp/SKPaint.cs:59 has a typo — SKFonxt.DefaultSize should be SKFont.DefaultSize. Confirmed locally:

SKPaint.cs(59,8): error CS0103: The name 'SKFonxt' does not exist in the current context

The new DefaultFont lazy path doesn't have direct coverage, which is why CI didn't catch it earlier. A tiny smoke test like using var p = new SKPaint(); (or exercising Reset()) would lock this in.

🔴 Likely regression in SKTypeface.CreateDefault()

return matched == IntPtr.Zero
    ? empty           // private field, null until Empty property is first read
    : GetObject (matched);

Pre-refactor empty was eagerly initialized in the cctor, so this was always non-null. Now it's lazy, so calling CreateDefault() before anyone has touched SKTypeface.Empty will return null. I think you just want the Empty property here.

🟠 Racy GetObject → PreventPublicDisposal pattern still in a few spots

SKFontManager.MatchFamily (line 73-74), MatchCharacter (177-178), and SKFontStyleSet (42, 53) still do:

var tf = SKTypeface.GetObject(...);
tf?.PreventPublicDisposal();

This is exactly the window the PR closes for singletons: between GetObject returning and PreventPublicDisposal taking the write lock, the wrapper is HD-observable in an unprotected state, and a concurrent Dispose() from another thread that fetched the same cached handle can win. Pre-existing, not something you introduced — but the new GetDisposeProtectedObject helper is the perfect tool to migrate these to while you're in the neighbourhood.

🟠 Undocumented behavioral change

The PR body says "Behavioral Changes: None", but typefaces returned from MatchFamily / MatchCharacter are now IgnorePublicDispose = true, so user tf.Dispose() becomes a silent no-op (cleanup only via finalizer). Worth a one-liner in the PR description / release notes for downstream consumers who deterministically dispose typefaces — they may notice increased native memory pressure.

🟡 Perf thought: Dispose() now takes the global write lock

The new Dispose() holds the HD write lock around the IgnorePublicDispose check + isDisposed CAS. That globally serializes every public Dispose against every reader of the dictionary. For apps that churn lots of short-lived SKPaint/SKPath/SKRect on parallel threads this could become a contention hotspot. A cheap fast-path (volatile read of the flag, CAS, then re-check under the lock only if needed) would let the common non-protected case avoid the writer entirely. Not a blocker — would just be good to benchmark before/after on a busy workload.

🟡 LockRecursionPolicy.SupportsRecursion — does anything actually recurse?

The PlatformLock comment says it's so Dispose's outer lock can re-enter DeregisterHandle via the Handle setter, but in the refactored flow the actual Dispose(bool) body runs outside the held write lock, so I don't see the recursive path anymore. The upgradeable→write transition inside GetOrAddObject → PreventPublicDisposal is a legal upgrade, not recursion. Could you double-check whether SupportsRecursion is still needed? Dropping back to NoRecursion would surface any unintended reentrancy as exceptions in tests.

🟡 Test coverage gaps worth filling

Net-new test coverage feels thin given the size of the refactor — and the unreliable repro from #3817 was dropped in 1dd6cfde. Suggestions in rough priority:

• Deterministic concurrent-init test for at least one singleton accessor (SKColorSpace.CreateSrgb, SKTypeface.Default, SKFontManager.Default, SKData.Empty, SKColorFilter.CreateSrgbToLinearGamma, the SKBlender static dict). Two threads barriered on a ManualResetEventSlim racing into the accessor, asserting Assert.Same and IgnorePublicDispose == true. That gets you a real regression net for the issue you're fixing.
• SKPaint construction smoke test — would have caught the typo above.
• SKTypeface.CreateDefault() when matched == Zero returns Empty — would catch the field-vs-property bug.
• Concurrent Dispose vs PreventPublicDisposal — the central safety claim of the new design, currently only asserted in prose.
• GetInstanceNoLocks filters IsDisposed wrappers — new behavior at HandleDictionary.cs:139.
• RegisterHandle replaces a IsDisposed=true entry — new code path.
• Singleton init under nested factory — SKTypeface.Default's factory transitively triggers SKFontManager.Default and SKFontStyle.Normal; worth a test that exercises that chain from a cold start (which is essentially [BUG] SKFontManager.Default throws an exception #3817 reproed deterministically).

✅ Things I really liked

• The disposeProtected flag on GetOrAddObject pairing the flag set with the HD critical section is a very nice abstraction.
• One-way-latch IgnorePublicDispose with a private setter — exactly right.
• The LazyInitializer + per-singleton lock pattern cleanly breaks the cctor cycle without resurrecting the old EnsureStaticInstanceAreInitialized hack.
• Eagerly cctor-initing SKFontStyle (which has no transitive singleton deps) — correct shortcut.
• TransferOwnershipToNative rename + OwnsHandle = false before DisposeInternal closes a real ownership-handoff race, and the stream tests assert the new contract well.
• Scoping [ModuleInitializer] to just the native-version check is the minimal correct surface.
• The inline race-reasoning comments in SKObject.cs and HandleDictionary.cs are excellent — please keep them.

Thanks again for tackling this — it's a tricky area and the refactor is heading somewhere much better than where we were. Happy to chat about any of the above if it'd help. 🙏

[azure-pipelines]

No pipelines are associated with this pull request.

[mattleibow]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[mattleibow]

Pushed a follow-up commit (b3f4041) on top of your work — wanted to explain the why so nothing here is surprising. This is all additive hardening/clarity around the lifecycle you reworked; no behavioural change to the locking design itself.

Library (SKObject / HandleDictionary)

• Mirror-guard leak fix in Dispose() — the diagnostic re-check (THROW_OBJECT_EXCEPTIONS) was throwing after claiming disposal but before Dispose(true), which would leak the native object in diagnostic builds. It now captures the raced state, runs Dispose(true) + SuppressFinalize, and only then throws. Production builds are unaffected (guard is compiled out).
• Comment accuracy — a few comments overstated the invariant (e.g. implying that observing IsDisposed could only mean a lock-contract violation, which isn't true given the no-lock internal/finalizer dispose paths your own comment documents). Rewrote them to state the precise invariant, and corrected stale wording on isDisposed, IgnorePublicDispose, the disposeProtected branch, and the GetOrAddObject XML doc.
• Readability — defined "compare-and-swap (CAS)" on first use and spelled out the HD shorthand as HandleDictionary.

Tests

• Added happy-path + disposed-path coverage for PreventPublicDisposal, and a real production-path test that exercises GetOrAddDisposeProtectedObject against a live existing instance (asserts same-instance return, IgnorePublicDispose, not disposed).
• Added SKSingletonConcurrencyTest with a deterministic re-entrant "GetObject during dispose" interleaving test (forces the disposed-but-not-yet-deregistered window and asserts the disposed wrapper is filtered, replaced without a double-dispose, and the trailing deregister is a no-op), plus a multi-lock singleton-contention canary across all singleton accessors.

Verified locally: build clean, filtered suite 31 passed / 0 failed / 0 skipped (Debug, with THROW_OBJECT_EXCEPTIONS on).

Separately, I'm drafting a non-blocking follow-up issue about reducing the global-lock contention via lock striping/sharding (with a benchmark plan) — that's purely an optimisation for later and won't hold up this PR. Really nice work on this rework. 🙏

[mattleibow]

Separately tracking some exploratory work on lock-striping / sharding the handle registry over in #4101 (with a draft prototype in #4102) so we don't clutter this PR with it. That investigation is independent and non-blocking — this PR stands on its own.

[azure-pipelines]

No pipelines are associated with this pull request.

[mattleibow]

I've been doing a deep review and found one regression worth flagging before merge.

TL;DR

SKCodec.Create(Stream) (and the latent equivalent in SKFontManager.CreateTypeface(Stream)) now crashes the process with a NullReferenceException when the codec actually decodes from a managed .NET stream. The new test reproduces it deterministically.

Reproduction & evidence

The test just decodes after Create() (the existing CanReadManagedStream only asserts the codec is non-null and never decodes, which is why it didn't catch this):

using var stream = new MemoryStream(File.ReadAllBytes(Path.Combine(PathToImages, "baboon.png")));
using var codec = SKCodec.Create(stream);
Assert.NotNull(codec);
Assert.Equal(SKCodecResult.Success, codec.GetPixels(out var pixels)); // 💥 crashes here

Running it aborts the test host:

Test host process crashed : Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at SkiaSharp.SKManagedStream.OnReadManagedStream(IntPtr buffer, IntPtr size) in SKManagedStream.cs:line 83
   at SkiaSharp.SKManagedStream.OnRead(IntPtr buffer, IntPtr size) in SKManagedStream.cs:line 100
   at SkiaSharp.DelegateProxies.SKManagedStreamReadProxyImplementation(...) in DelegateProxies.stream.cs:line 79

Root cause

This call site changed from RevokeOwnership(codec) to TransferOwnershipToNative():

// SKCodec.cs:260-261
var codec = GetObject (SkiaApi.sk_codec_new_from_stream (stream.Handle, r));
stream.TransferOwnershipToNative ();   // was: stream.RevokeOwnership (codec);

The two are not equivalent for managed-callback streams. On main, RevokeOwnership had two paths:

internal void RevokeOwnership (SKObject newOwner)
{
    OwnsHandle = false;
    IgnorePublicDispose = true;
    if (newOwner == null)
        DisposeInternal ();                       // dispose now
    else
        newOwner.OwnedObjects[Handle] = this;     // keep alive, dispose with the owner
}

For the codec, newOwner != null, so the managed SKManagedStream wrapper was re-parented into codec.OwnedObjects and kept alive until the codec was disposed.

TransferOwnershipToNative() always takes the dispose-now path:

internal void TransferOwnershipToNative ()
{
    OwnsHandle = false;
    DisposeInternal ();   // always disposes immediately
}

Because WrapManagedStream builds the wrapper with disposeManagedStream: true, DisposeManaged() then closes and nulls the underlying .NET stream:

// SKManagedStream.cs:65-67
if (disposeStream && stream != null) {
    stream.Dispose ();
    stream = null;
}

But the native codec reads lazily — sk_codec_new_from_stream only stores a pointer; the actual reads happen later (e.g. in GetPixels) via the managed read callback OnReadManagedStream, which now dereferences a null stream → NRE across the native→managed boundary → host crash.

Why it doesn't affect every stream

This only bites streams that native reads back through managed callbacks (SKManagedStream / SKFrontBufferedManagedStream). Native-byte-backed streams like SKMemoryStream are fine, because their bytes live in native memory and don't depend on the managed wrapper surviving — which is why the SKMemoryStream-based tests stay green and this slipped through.

Affected call sites

• binding/SkiaSharp/SKCodec.cs:261 — confirmed crash (test attached).
• binding/SkiaSharp/SKFontManager.cs (CreateTypeface(Stream) → TransferOwnershipToNative()) — same shape. It currently dodges the crash only because it copies into a memory stream first, but it relies on the same eager-dispose semantics and is worth a hard look / matching test.

Suggested direction (your call)

Restore the "keep the managed wrapper alive until its native owner is disposed" semantics for the callback-stream case — e.g. give the handoff an optional newOwner and re-parent into OwnedObjects (mirroring main's non-null RevokeOwnership path) instead of disposing eagerly, while keeping the always-dispose behavior for native-backed streams. The important invariant: Create(Stream) must not close the caller's stream, since the codec may decode long after Create() returns.

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

No pipelines are associated with this pull request.

[mattleibow]

I added test coverage on top of this — tests only, no production changes, and the latest main merged in.

Corner-case tests for the singleton/lifecycle rework (SKHandleDictionary* + helpers)

These drive HandleDictionary directly with a fake, non-owning SKObject (no native allocations):

• Concurrent construction: callers racing on the same handle get exactly one wrapper, waiters only ever observe a fully-constructed wrapper, and a throwing factory leaves nothing registered so a later call can reconstruct.
• Dispose-protected promotion: disposeProtected: true sets the one-way IgnorePublicDispose latch, and promote racing a concurrent public Dispose() on the same handle always returns a live, protected wrapper (never torn).
• Stale-wrapper / address reuse (ABA): a freed handle builds a fresh wrapper, and a stale wrapper finishing cleanup outside the lock never evicts the live replacement. These assert reference identity (AssertDeregistered) rather than "address absent", which is racy under parallel runs.

Managed-stream ownership handoff (SKManagedStreamTest, SKCodecTest, SKTypefaceTest)

When a .NET Stream is passed to SKCodec.Create(Stream) or SKTypeface.FromStream(Stream), SkiaSharp wraps it and reads from it lazily (later, on demand), so the wrapper has to outlive the call. These tests lock down that behaviour so the stream-callback fix can't regress:

• The stream stays readable after the call and decoding succeeds — including non-seekable streams, and the lazy SVG/PDF write paths (CanDecodeManagedStreamAfterCreate, ...StaysOpenUntilOwnerDisposed).
• On a failed call (invalid stream) the wrapper is disposed and deregistered immediately instead of leaking.
• If the stream is disposed while the native side is still reading, or both race at teardown, it is closed exactly once with no double-free (...ClosesManagedStreamExactlyOnce, ConcurrentDisposeOf...).

Re-entrancy contract (SKHandleDictionaryReentrancyTest)

Locks down a factory running inside the upgradeable-read lock: a re-entrant read (GetInstance) is allowed, while a nested create (GetOrAddObject) throws LockRecursionException on non-Windows. The Windows branch (recursive CRITICAL_SECTION, #1383) is skipped since it can't be exercised here.

Singleton initialization

Two pieces:

• SKSingletonInitTest (main test project): each documented singleton — SKColorSpace.CreateSrgb()/CreateSrgbLinear(), SKData.Empty, SKFontManager.Default, SKTypeface.Default/Empty, SKBlender.CreateBlendMode, the SKFontStyle statics — returns the same instance and carries the IgnorePublicDispose flag; Dispose() on a protected singleton is a no-op; concurrent access returns one instance; CreateDefault() and new SKPaint() don't throw or return null. The [BUG] SKFontManager.Default throws an exception #3817 cold-start cctor cycle is reproduced via an isolated AssemblyLoadContext.
• A separate single-test assembly (SkiaSharp.Tests.SingletonInit.Console, wired into cake + sln) whose only test touches SKFontManager.Default as the first SkiaSharp type in a fresh dotnet test host process — the genuine process-level [BUG] SKFontManager.Default throws an exception #3817 regression guard. Keeping it to one test per assembly is what preserves the "first type touched" ordering.

These are here to keep the lifecycle behaviour pinned so a future refactor of this central code can't silently regress it.

[azure-pipelines]

No pipelines are associated with this pull request.

[azure-pipelines]

No pipelines are associated with this pull request.