[EXPERIMENT] Lock-striped HandleDictionary prototype (#4101)#4102
[EXPERIMENT] Lock-striped HandleDictionary prototype (#4101)#4102mattleibow wants to merge 28 commits into
Conversation
The file es-metadata.yml (added in #4075) is repository metadata and is unrelated to any build job. Without this entry the 'Validate cache config coverage' step fails on every PR with: [FAIL] UNCOVERED FILES: es-metadata.yml Adding it to the top-level exclude array unblocks PR builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#4062) Migrate Linux Docker builds to .NET official cross-compilation images (#4062) The old per-Dockerfile approach assembled cross-compile toolchains by hand on Debian bases: debian/11/Dockerfile pulled `gcc-cross` apt packages and clang-13, debian/13/Dockerfile was a separate image just to get a newer lld for riscv64/loongarch64, alpine/Dockerfile downloaded apk.static at a pinned SHA and built up a musl sysroot under /alpine, and bionic/Dockerfile fetched the Android NDK r26b zip from dl.google.com. Each had its own arch case statement and its own toolchain assumptions, and _clang-cross-common.sh tried to share what it could. Replace all of that with thin layers on top of the .NET infra team's prereqs images: mcr.microsoft.com/dotnet-buildtools/prereqs: azurelinux-3.0-net10.0-cross-<arch>[-musl] azurelinux-3.0-net10.0-android-amd64 These already ship Clang 20 + lld built from source, a complete sysroot per arch under /crossrootfs/<arch>/, libc++ (static) for the arches the .NET team ships binaries for, and current security patches. Our Dockerfiles now just add the .NET SDK, fontconfig-dev headers extracted from SHA-pinned .debs, and a /etc/skia-env file that the Cake build scripts source for SYSROOT_ROOT, SYSROOT_PATH, GCC_LIB_DIR, and the toolchain triple. ~250 LOC of bespoke toolchain construction gone; future updates ride .NET's release cadence. * glibc/Dockerfile covers arm, arm64, x64, riscv64, loongarch64. Multi-FROM stage aliases (FROM ... AS base-x64, ..., FROM base-${BUILD_ARCH}) hide the .NET x64→amd64 image-tag mapping inside the Dockerfile; callers (build-local.sh, CI YAML) only ever pass BUILD_ARCH. BuildKit DAG-prunes the unused stages so only one base image is pulled per build. * glibc-x86/Dockerfile is the one exception: the .NET team does not ship a libc++ build for linux-x86, so stage 1 builds it from source by mirroring the upstream amd64 prereqs Dockerfile verbatim except for three substitutions (ROOTFS_DIR, build-rootfs.sh arch, TARGET_TRIPLE). Stage 2 layers on the same SkiaSharp env as glibc/. * alpine/Dockerfile uses the cross-<arch>-musl variants directly, replacing the Debian 12 + apk.static dance entirely. * bionic/Dockerfile uses the android-amd64 image whose NDK is discovered dynamically with `find ... | sort -V | tail -1`, no hardcoded version string. * Fontconfig dev headers are SHA-256-pinned .debs extracted into the sysroot at image-build time, with a post-extract sanity check that fails fast if usr/include/fontconfig/fontconfig.h is missing — catches transitional empty packages like libfontconfig1-dev on Ubuntu 22.04+ (a previous CI failure of this PR). * Per-arch verifyGlibcMax in CI fails the build if a future .NET image bump silently raises the sysroot glibc beyond the documented target. ~~ C++ runtime ~~ Most .NET cross-image sysroots are Ubuntu 18.04 (bionic) with GCC 6/7, which lacks the C++20 support Skia now requires. native/linux/build.cake therefore switches from `-static-libstdc++ -static-libgcc` to `-stdlib=libc++`, picking up the libc++ that ships in the image. That libc++.a is built with `-DLIBCXX_CXX_ABI=libstdc++`, so its ABI symbols (operator new/delete, __cxa_throw, exception type info, etc.) are satisfied at runtime by libstdc++. native/linux-clang-cross/build.cake adds `-Wl,--whole-archive,-lstdc++,--no-whole-archive` to provide those symbols dynamically; the net effect is a new `libstdc++.so.6` DT_NEEDED on every glibc and musl SkiaSharp/HarfBuzzSharp .so. This is not a real new requirement: every Linux .NET runtime native library (libcoreclr.so, libclrjit.so, libclrgc.so, libhostfxr.so, libhostpolicy.so, libmscordaccore.so) already lists libstdc++.so.6 in NEEDED on every glibc and musl RID, so any process running `dotnet` already has it loaded. linux-bionic-* and android-* are unaffected because the NDK ships a self-contained libc++_static.a; build.cake re-adds `-static-libstdc++` for the bionic variant only to avoid a libc++_shared.so runtime dep (the initial switch to -stdlib=libc++ regressed bionic before this fix landed). Eliminating the libstdc++.so.6 dep would require building libc++/libc++abi from source for every glibc/musl arch (extending glibc-x86's multi-stage pattern to all other Dockerfiles, ~250 LOC + ~15 min CI per arch); not worth it given the .NET runtime baseline. ~~ Sysroot glibc deltas ~~ The .NET cross-image sysroots use different Ubuntu/Debian releases than the old Debian 11/13 + gcc-cross combination, so the verifyGlibcMax targets are: arm64 / x64 / x86 : 2.27 (bionic 18.04, ↓ from 2.31, broader compat) arm : 2.35 (jammy 22.04) riscv64 : 2.39 (noble 24.04) loongarch64 : 2.42 (debian sid) musl/Alpine variants stay ABI-stable across musl 1.2.x. Android NDK remains at 27.2.12479018 / API 29. ~~ Validation ~~ Extracted every CI-produced nupkg and ran readelf -d + readelf -V on all 51 .so files across SkiaSharp/HarfBuzzSharp Linux/Android/Tizen native packages: every arch's DT_NEEDED matches the expected set, GLIBC max stays at or below the configured target for each RID, musl/bionic/android binaries have zero GLIBC_ symbols, and linux-bionic-* correctly links libc++_static.a (no libc++_shared.so dep). Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add --pull to Docker builds to ensure fresh base images (#4087) Without --pull, Docker reuses whatever base image layer is cached on the build agent, which can lag behind upstream MCR images by days or weeks. Updates shipped in the .NET prereqs images never reach our builds until an agent happens to lose its cache. Add --pull to both the 1ES and non-1ES Docker build commands so the agent always checks MCR for an updated base image before building. Adds a few seconds of registry metadata lookup per build but ensures containers always start from the latest upstream base layer. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[CI] Install .NET SDK for native builds to fix Windows failures (#4089) Context: #4055 After #4055 bumped the required SDK from 10.0.105 to 10.0.108, native Windows builds began failing at the "Restore the .NET tools" step with: Requested SDK version: 10.0.108 Installed SDKs: 8.0.416, 9.0.310, 10.0.101 The `UseDotNet@2` task that installs the configured SDK version was inside the managed-only section (`not(startsWith(name, 'native_'))`), so native builds relied entirely on the agent's pre-installed SDK. macOS agents weren't affected because they ship 10.0.300, which satisfies `rollForward: latestFeature` from 10.0.108. Windows agents only have 10.0.101 in the 1xx band — below the minimum. Move the SDK install (`UseDotNet@2`), Linux dotnet patch, and `dotnet --info` into a new top-level block guarded only by `installDotNet` and `skipInstall`, so both native and managed builds get the correct SDK. Workload installation remains in the managed-only section since native builds don't need workloads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Add ci-status skill for daily CI health dashboard (#4094) Reviewing CI health for SkiaSharp meant manually opening four separate Azure DevOps pipelines across two orgs and mentally diffing the last few builds per branch. The existing release-status skill only tracks a single release through its pipeline chain, so there was no broad, at-a-glance view of whether main and the active release/* branches were healthy. Add a ci-status skill that collects build status across all tracked pipelines and layers AI analysis on top to turn raw red/green into actionable diagnosis. Pipelines tracked: * SkiaSharp (Public) — xamarin/public def 4 (push/PR to main, release/*) * SkiaSharp-Native (26493) -> SkiaSharp (10789) -> SkiaSharp-Tests (15756) — the sequential devdiv/DevDiv release chain scripts/ci-status.py queries each pipeline once and renders three views from a single data collection pass: * console summary for quick terminal use * markdown report (--output) with a health matrix, per-branch commit info (SHA/author/message), durations, pass-rate stats, and a green->red regression table * structured JSON (--json) for the AI analysis step to consume It auto-discovers the N most recent release/* branches (including .x servicing branches, which are the servicing equivalent of main), extracts errors/warnings from failed builds, caches the ADO token across requests, and pre-computes regression transitions. SKILL.md drives the AI layer: root-cause clustering by normalized error signature, a classification decision tree (infra/flake/regression/quota/ chain blockage), mandatory pipeline-chain cascade analysis, flake detection, cross-branch correlation, release-risk assessment, and a capped top-5 prioritized action list — every claim tied to a real build ID and URL. The skill was benchmarked with skill-creator's evaluator across three scenarios (daily health check, full report with analysis, release readiness). The eval surfaced one recurring miss — the analysis sometimes listed internal pipelines as independent failures instead of collapsing cascaded downstream failures to their upstream root cause — which is why Step 2.3 is now mandatory and the chain verdict is surfaced in the presentation. The eval test cases are committed under evals/ so the skill stays benchmarkable. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix WinUI Projection DLL not resolved for .NET 9 consumers on Windows (#4084) Fixes: #4082 When TFMCurrent moved from net8.0 to net10.0 (in #3514), the `NativeAssetPackageFile` entries in `SkiaSharp.NativeAssets.WinUI.csproj` placed `SkiaSharp.Views.WinUI.Native.Projection.dll` only into: runtimes/win-{arch}/lib/net10.0-windows10.0.19041.0/ NuGet's TFM resolution algorithm won't serve a net10.0 runtime asset to a net9.0 consumer — higher TFMs are not backward-compatible. .NET 9 apps hit a `FileNotFoundException` at startup: Could not load file or assembly 'SkiaSharp.Views.WinUI.Native.Projection, ...' In v3.119.2 (TFMCurrent=net8.0) this worked because the DLL landed in `lib/net8.0-windows.../`, which both net8 and net9 consumers could resolve. Include the Projection DLL in both `$(WindowsTargetFrameworksCurrent)` and `$(WindowsTargetFrameworksPrevious)` folders so NuGet serves it regardless of whether the consuming app targets net9 or net10. The DLL itself is built targeting net9.0-windows and is binary-compatible with both. Size overhead is ~25 KB × 3 RIDs — negligible against the ~450 MB package. Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[Security] Add Chrome Releases blog integration to security-audit skill (#4097) The Chrome Releases blog frequently discloses Skia and ANGLE CVEs days or weeks before NVD processes them. This adds the blog as a first-class data source for the security-audit skill, closing the early-detection gap and providing cross-validation with NVD results. ~~ Data collection ~~ New script `query-chrome-releases.py` fetches the Blogger Atom feed (paginated at 25/page — a platform hard-cap), filters for Skia-relevant keywords, and extracts structured CVE data via regex. The regex now handles embargoed entries that omit reporter/date attribution. Results are cached for 24 hours with parameter-aware invalidation (requesting a wider month range than cached automatically re-fetches). ~~ Report pipeline ~~ * SKILL.md Step 8 now explicitly lists `chromeReleases` as top-level key #6, with snake_case→camelCase field mapping instructions so the AI knows to transform the script output and propagate `blogPostUrl` onto matching findings CVEs. * `report-schema.md` documents `structuredCves[]` as the primary rendered array (not `earlyDisclosures`), fixing a three-way field- name contradiction between schema, validator, and renderers that would have caused the Chrome Releases section to silently render blank in production. * `security-audit-schema.json` updated: `structuredCves` required, `earlyDisclosures` optional. ~~ Markdown renderer (new) ~~ `render-security-audit-md.py` produces a comprehensive report for AI consumption — full descriptions (no truncation), sanitized table cells, Skia resolution details (fixCommit, reachability, inOurTree), and upstream verification metadata. Section order: Summary → Findings → CG Alerts → Chrome Releases → Version Verification → Next Steps. ~~ HTML viewer ~~ * Chrome Releases section with severity cards and CVE table * Blog post links, component priority sorting (Skia first) * Compact tables with click-to-expand descriptions * Conditional "Fixed In" column, affected-first sort order * Summary card descriptive text ~~ Evals ~~ 6 Chrome Releases expectations added to eval #1; new eval #4 dedicated to Chrome Releases validation (10 expectations). Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ct → AI → render pipeline (#4099) [CI Status] Add GitHub Actions tracking and restructure as collect → AI → render pipeline (#4099) The ci-status skill previously only checked Azure DevOps pipelines and produced a one-shot console/markdown report with no persisted AI analysis. This meant the analysis lived in chat context and was lost after the session. Restructure to follow the security-audit pattern: a collector script produces raw JSON, the AI reads and assembles an augmented report JSON with root-cause analysis and recommendations, then validate/render scripts produce self-contained HTML and Markdown outputs that persist to disk. ~~ GitHub Actions integration ~~ Track 18 workflows across mono/SkiaSharp and mono/SkiaSharp-API-docs with scope-aware collection: * Branch-scoped (main + release/*): Docs-Deploy, Publish Samples, Update Release Notes * Global (last run only): cron/dispatch/event workflows like Auto API Docs Writer, Validate Gallery Samples, stale issue management Failed job details are fetched for the latest failed run per workflow (~1.5s per gh run view call). ~~ New pipeline architecture ~~ Step 1: Collector → ci-status-data.json (raw AzDO + GHA data) Step 2: AI reads raw data, identifies root causes, regressions, flakes Step 3: AI assembles ci-status-YYYY-MM-DD.json (augmented report) Step 4: Validator checks schema + cross-references (exit 0/1/2) Step 5: Renderers produce .html (Bootstrap 5 dashboard) + .md New files: validate-ci-status.py, render-ci-status.py, render-ci-status-md.py, viewer.html, references/report-schema.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
📦 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 -- 4102PowerShell / Windows: iex "& { $(irm https://raw.githubusercontent.com/mono/SkiaSharp/main/scripts/get-skiasharp-pr.ps1) } 4102"Step 2 — Add the local NuGet source dotnet nuget add source ~/.skiasharp/hives/pr-4102/packages --name skiasharp-pr-4102More options
Or download manually from Azure Pipelines — look for the Remove the source when you're done: dotnet nuget remove source skiasharp-pr-4102 |
…test` (#4004) Migrate device and WASM tests to DeviceRunners dotnet test (#4004) Context: https://github.com/mattleibow/DeviceRunners Context: #4004 Replace the XHarness-based Android, iOS, and Mac Catalyst test runners and the custom Selenium-based WASM runner with DeviceRunners' `dotnet test` integration. This moves all device/browser test execution onto the standard test command while still producing CI-friendly TRX results. The WASM path now runs the shared SkiaSharp test suite instead of the old placeholder coverage. CI validation increased WASM coverage from 3 placeholder tests to 3889 tests with 0 failures, and the final PR run passed Android, iOS, Mac Catalyst, WASM, Windows, macOS, Linux, samples, API diff, and packaging. Key implementation details: * add DeviceRunners visual/CLI runner setup for MAUI and Blazor hosts * replace XHarness and the old WASM Selenium launcher with `dotnet test` * publish TRX/VSTest results instead of xUnit XML * keep Apple device tests in Debug to preserve full ApiTest P/Invoke reflection coverage * pre-build Android before emulator startup and run tests with `--no-build` to avoid CI memory pressure * use the repo restore/package-cache settings for DeviceRunners test runs * make DeviceRunners failures propagate so any platform test failure fails CI Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Picks up es-metadata.yml scaffold addition from mono/skia. Changes: - externals/skia: advance submodule to bf112f783f (adds es-metadata.yml) - cgmanifest.json: update mono/skia commitHash to bf112f783f No upstream C++ changes, no C API changes, no binding regeneration needed. Build: Linux x64 clean, 5504 tests passed (171 skipped - GPU hardware not available). Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The first striping prototype ran the object factory while holding a shard
lock. With nested, different-handle construction that creates a cross-shard
AB-BA deadlock hazard (thread 1 holds shard A and wants shard B; thread 2
holds shard B and wants shard A).
This reworks GetOrAddObject into a two-phase, reservation-gate design so the
factory NEVER runs under a shard lock:
Phase 1 (under shard lock, O(1)): dedup against a live instance; else if
another thread is already building this handle, wait on its
per-handle gate and retry; else install our own Reservation.
Phase 2 (NO lock held): run the factory. The wrapper ctor's RegisterHandle
sees our reservation and suppresses publication, so a half-built
wrapper is never visible.
Phase 3 (under shard lock, O(1)): publish the finished wrapper, drop the
reservation, then signal the gate.
Because no shard lock is held across the factory, a thread holds at most one
shard lock at a time, so the cross-shard cycle is structurally impossible.
Residual hazards are documented: re-entrant same-thread same-handle
construction is detected and thrown (never hung), and a genuine cross-object
construction-time dependency cycle is an application bug.
In-flight handles live in a separate per-shard "reservations" map, so
"instances" still only ever holds fully-constructed wrappers and the
GetInstance / DeregisterHandle / diagnostics paths need no changes.
Adds SKHandleDictionaryReservationTest pinning every claim: exactly-once
construction, publication safety (waiters only see a fully-built wrapper),
re-entrant throw, factory-failure recovery, nested different-handle
construction, the live cross-shard deadlock regression, and dispose-protected
fresh construction.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update: reservation-gate two-phase constructionPushed The hazard it removesThe initial prototype ran the factory while holding the shard lock. Nested, different-handle construction could then form a classic AB-BA cross-shard cycle:
With a single global lock this could never happen (one lock, no ordering). Striping reintroduced it. How it works
Because no shard lock is ever held across the factory, a thread holds at most one shard lock at any instant, so the cross-shard cycle is structurally impossible — not merely unlikely. In-flight handles live in a separate per-shard What it solves vs. the "construct then dispose the loser" approachesEarlier we worried about dedupe strategies that build a C# object and then dispose the duplicate — unacceptable because user code may already hold (and act on) the losing handle/wrapper. The reservation gate sidesteps that entirely: only one wrapper is ever constructed per handle, so there is no loser to dispose and no observable half-built object. Residual hazards (documented, tested where deterministic)
Tests added (
|
Update expat to 2.8.1 (#4079) Changes: libexpat/libexpat@R_2_7_5...R_2_8_1 Required skia PR: mono/skia#245 Bump libexpat from 2.7.5 to 2.8.1 via the skia submodule. The upstream 2.8.x release includes security hardening and refactors entropy source selection into separate compilation units. BUILD.gn in the skia fork was updated to handle the new source file structure (see mono/skia#245). * cgmanifest.json: version bump for component governance tracking * externals/skia: submodule updated to include DEPS + BUILD.gn changes Co-authored-by: Matthew Leibowitz <mattleibow@live.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refactor HandleDictionary.ShardFor to delegate to a pure internal static ShardIndexFor(handle, mask) so the routing can be verified deterministically in isolation (mask == 0 collapses every handle onto shard 0 — byte-for-byte the original single-lock design). Close the three coverage gaps called out in the #4101 learnings: - AddressReuseAfterDisposeConstructsFreshWrapper (ABA: a reused native address after dispose builds a fresh wrapper, registry stays exception-clean because the loser is already disposed). - DisposeProtectedRacingPublicDisposeNeverTears (barrier-synced promote-vs-public-dispose; the disposeProtected caller always gets back a live, protected wrapper across 300 iterations). - SingleShardRoutesEveryHandleToZero (N==1 parity via ShardIndexFor(handle, mask: 0) == 0) and a multi-shard routing sanity test (in range + actually spreads). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A real BenchmarkDotNet benchmark for the lock-striped registry so the
striping experiment can be measured reproducibly instead of via ad-hoc
harnesses. Each job pins SKIASHARP_HANDLE_SHARDS in its own process, so
HandleDictionary.ShardCount (resolved once at static init) takes the
job's value and the SAME code runs single-lock (N=1) vs N-way striped.
Two workloads:
- ChurnDistinct: threads construct+dispose distinct handles (striping's
intended target — registrations spread across shards).
- SharedLookup: threads resolve a few shared handles via the dedup path
(honest counter-signal — few shards, contention can't spread).
Program.cs now uses BenchmarkSwitcher so a single benchmark can be run
with --filter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y-read crash (#4101) PR #4080 changed TransferOwnershipToNative to EAGERLY dispose the managed stream wrapper after handing the native object to SKCodec / SKTypeface. For streams that decode lazily (SKManagedStream / SKFrontBufferedManagedStream invoke managed read callbacks during a later GetPixels()/table read), that eager dispose closes the underlying .NET stream and frees the callback GCHandle, so the subsequent lazy read crashes across the native->managed boundary and brings down the host. Restore main's lazy-reparent semantics, with a race fix: - newOwner == null (native creation failed): dispose eagerly as before (no receiver to keep the wrapper alive). - newOwner != null: re-parent the wrapper as a non-owning owned-child of the receiving codec/typeface. It stays alive (rooted in OwnedObjects) and is torn down only when the owner is disposed. DisposeUnownedManaged() disposes !OwnsHandle children BEFORE the owner frees its native object, so the wrapper stops being read before the native object is freed. Race fix: latch OwnsHandle=false AND PreventPublicDisposal() together under the same shard write lock that public Dispose() takes before reading IgnorePublicDispose and claiming the isDisposed CAS. Setting OwnsHandle=false outside the lock first would let a racing Dispose() observe IgnorePublicDispose==false, win the claim, skip DisposeNative (OwnsHandle already false) and run DisposeManaged() — re-creating the crash as a race. Tests rewritten to lazy semantics + new regression coverage: - SKCodecTest / SKManagedStreamTest / SKTypefaceTest: the 6 eager-dispose assertions now assert the wrapper stays alive/registered until the owner is disposed; the underlying .NET stream stays readable (CanRead) until then. - New: CanDecodeManagedStreamAfterCreate, CanDecodeNonSeekableManagedStreamAfterCreate (lazy decode after Create), and the SKManagedStream stays-open guard. - InvalidStreamIsDisposedImmediately (failure path) unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er livelock (#4101) The reservation-gate construction in GetOrAddObject publishes a wrapper, then in a finally block removes its reservation from the shard map and signals the gate. If the publish-phase write-lock acquisition itself throws, the reservation can remain in the map while its gate is already signaled. A waiter that later finds this stale reservation would wait on an already-set gate, re-check, still see it present, and busy-livelock. Add a `volatile bool retired` flag to Reservation, set BEFORE the gate is signaled on every exit path. A waiter that observes a still-present reservation whose `retired` is true (and confirms nothing was published) takes the reservation over and reconstructs, instead of livelocking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…le (#4101) Add two Owned-path lifecycle tests proving a managed write stream handed to native (SVG canvas, PDF document) stays open until the OWNER is disposed, not eagerly torn down during ownership transfer. Adds a LifecycleTrackingStream helper. Pins the lazy ownership-transfer semantics restored earlier. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… it (#4101) Reorder the Phase 1b guards in GetOrAddObject so a retired (fully-unwound) reservation is taken over BEFORE the same-thread re-entrancy throw. A retired reservation has already run ConstructAndPublish's outer finally, so it can never be the current thread's own active construction; taking it over is always safe, even for the thread that originally owned it and is now retrying the same handle after catching a publish-phase failure. Previously that retry wrongly hit the re-entrant throw. Add a Debug-only (THROW_OBJECT_EXCEPTIONS) PublishPhaseHook fault-injection seam invoked just before the Phase 3 publish write-lock, so a test can deterministically drive a reservation into the 'retired but present' state (publish lock could not be acquired / Remove never ran) — otherwise that branch is unreachable via normal APIs. Release builds compile the seam out entirely. Add two deterministic tests: same-thread retry takes over the retired reservation (pins the guard ordering), and a different thread takes it over without livelocking on the already-signaled gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cle tests Hunt for concurrency/lifecycle loopholes in the SKObject/HandleDictionary rework (#4080). Tests only; no production changes. Drawables DO have a 'native outlives managed' path: a live SKPictureRecorder retains a native sk_sp<SkDrawable> across the managed wrapper's Dispose(), whereas the finished SKPicture keeps only a snapshot. New tests: - NativeOwnerOutlivingManagedDrawableDefersDestroyExactlyOnce: proves managed Dispose() only unrefs (fromNative==0 right after dispose) while the recorder holds the ref, then the deferred destroy fires (fromNative latches to 1) when the last native owner is released; second Dispose() is a safe no-op. - ConcurrentDisposeOfDrawableAndNativeOwnerIsSafe: 200-iteration Barrier-synced stress race of managed Dispose() vs native-owner release. - RecordedPictureIsIndependentOfManagedDrawableLifetime: rewritten to assert only version-independent invariants (InRange(fromNative,0,1), disposed, deregistered, picture still renders blue). Plus 5 SKAbstractManagedStream re-entrancy/destroy-callback tests mirroring the same fromNative deferred-destroy pattern. All tests use try/finally idempotent cleanup so GarbageCleanupFixture stays clean on any mid-test throw. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…roof Strengthen the SKManagedStream codec ownership-handoff race tests that pin the LAZY-reparent fix in #4080: - PublicDisposeWhileCodecReadIsInFlightIsIgnored: deterministic gated race that parks the native lazy read inside the .NET Stream.Read callback, fires the reparented wrapper's public Dispose while the read is provably in-flight, then releases. Reader Task is hoisted and drained in finally so a primary assertion failure is never masked by a background decode against a disposed codec. - PublicDisposeAroundLazyCodecReadIsIgnoredStress / ...TeardownClosesManagedStream ExactlyOnce: bounded Task.WaitAll(30s) so a deadlock regression fails fast instead of hanging the suite. - GatedCountingStream now records FirstDisposeThreadId so the teardown-race test proves the codec owner-teardown (not the ignored public Dispose) actually closed the .NET stream — DisposeCount==1 alone cannot distinguish which path won. Tests only; no production changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e/race Adds tests-only coverage for the non-seekable codec path, where SKCodec.Create wraps the user stream in an SKFrontBufferedManagedStream whose ctor builds a private INNER SKManagedStream. Only the outer fb wrapper is reparented onto the codec (OwnsHandle=false, IgnorePublicDispose=true); the inner is untouched and is torn down transitively via fb.DisposeManaged() at codec teardown. This distinct object graph was previously uncovered. New helper NonSeekableGatedCountingStream (CanSeek=false) counts Dispose(true) and supports a gated first-read to deterministically park an in-flight codec read. Tests: - NonSeekableStreamReparentedToCodecTearsDownNestedStreamExactlyOnce: end-to-end reparent + single nested teardown; ignored fb.Dispose() is a provable no-op. - PublicDisposeWhileFrontBufferedCodecReadIsInFlightIsIgnored: deterministic in-flight-read race; public Dispose during a parked inner read is ignored, decode still succeeds, teardown disposes once. - InvalidNonSeekableStreamFailedCodecCreateClosesNestedStreamExactlyOnce: failed Create disposes the nested stream exactly once. - FrontBufferedCodecWithoutOwnershipDoesNotCloseUnderlyingStream: disposeUnderlyingStream:false leaves the user's .NET stream open until the user closes it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-wrapper race Pin the lock-paired Dispose safety window (cleanup runs OUTSIDE the per-handle shard lock) with deterministic tests on the reused-handle path: - Make the GetOrAddObject and direct-ctor replacement tests leak-proof: dispose the replacement wrapper on every path via an outer try/finally so an assertion failure mid-window cannot leak a live wrapper and have GarbageCleanupFixture mask the real failure. - Add TwoStaleWrappersDeregisteringDoNotEvictThirdReplacementForReusedHandle: W1 and W2 are BOTH parked in DisposeManaged (disposed, lock released, Handle not yet zeroed) for the same reused handle while a live W3 owns the slot. The two stale wrappers then run DeregisterHandle in an order DIFFERENT from publication (W2 before W1); neither may evict W3 nor throw. Exercises the 'only remove if the entry still points to THIS instance' guard across overlapping stale deregistrations, which the single-stale tests do not cover. Tests only; no production changes. 48 concurrency tests pass, fixture clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#4107) Adds the dedicated SkiaSharp.Tests.SingletonInit.Console assembly with a single [Fact] that touches SKFontManager.Default as the FIRST SkiaSharp type in a fresh test-host process, guarding the #3817 re-entrant static-cctor regression. Wires it into tests-netcore.cake, tests-netfx.cake and the console solution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Hardens test coverage around the reworked HandleDictionary/SKObject singleton lifecycle. These cases were discovered while exploring the lock-striping experiment (mono#4101/mono#4102); the experiment itself adds no measurable value, but the corner cases it surfaced are worth locking in against the single-global-lock design in this PR. Adds (tests only — no production changes): - SKHandleDictionaryTestHelpers.cs: shared internal fakes (FakeNativeObject, GatedCleanupObject) and deterministic concurrency helpers. RunConcurrent uses one dedicated Thread per participant with a Barrier (the thread pool can't guarantee N simultaneous Parallel.For bodies, which deadlocks against Barrier(N)); RunWithTimeout bounds dispose-storms so a production deadlock fails cleanly instead of hanging. AssertDeregistered<T> asserts a handle no longer resolves to a specific wrapper by reference identity, which is safe against native address reuse under xUnit's parallel collections (asserting the address is simply absent is racy — a parallel test can reallocate a fresh wrapper at the same freed pointer). - SKHandleDictionaryReservationTest.cs: concurrent same-handle construction resolves to exactly one wrapper; waiters only observe a fully-constructed wrapper; factory failure clears the reservation and lets a waiter/retry reconstruct; cross-handle construction in opposite order does not deadlock under the single lock. - SKHandleDictionaryDisposeProtected.cs: dispose-protected construction sets the flag; a racing public Dispose() never tears down a dispose-protected wrapper. - SKHandleDictionaryStaleWrapperTest.cs: a stale wrapper deregistering after address reuse never evicts the live replacement (GetOrAdd and direct-ctor paths, including two stale wrappers vs a third replacement). - SKManagedStreamTest.cs / SKCodecTest.cs: exercise the managed-Stream callback path that the existing SKMemoryStream tests don't reach — public Dispose() ignored while a codec read is in flight; lazy decode after ownership transfer; non-seekable/front-buffered reparenting tears down the nested stream exactly once; failed Create closes the stream exactly once (seekable and non-seekable); PDF/SVG writer streams stay open until the owner is disposed; concurrent dispose of many/same managed streams is safe. Also corrects the InvalidStreamIsDisposedImmediately expectation in three files: the failed-create path runs RevokeOwnership(null) -> DisposeInternal(), which genuinely disposes (and deregisters) the wrapper rather than marking it IgnorePublicDispose. ff18b0f's blanket IsDisposed -> IgnorePublicDispose rename was right for the success/reparent path but wrong here, where there is no new owner to protect the wrapper for. Finally, fixes an intermittent flake in the existing ManagedStreamIsCollectedWhenTypefaceIsDisposed: it used the same racy absence assertion (Assert.False(GetInstance(handle, out _))) that fails when a parallel test reuses the freed native pointer. Switched to the reference-identity AssertDeregistered<T> helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Hardens test coverage around the reworked HandleDictionary/SKObject singleton lifecycle. These cases were discovered while exploring the lock-striping experiment (mono#4101/mono#4102); the experiment itself adds no measurable value, but the corner cases it surfaced are worth locking in against the single-global-lock design in this PR. Adds (tests only — no production changes): - SKHandleDictionaryTestHelpers.cs: shared internal fakes (FakeNativeObject, GatedCleanupObject) and deterministic concurrency helpers. RunConcurrent uses one dedicated Thread per participant with a Barrier (the thread pool can't guarantee N simultaneous Parallel.For bodies, which deadlocks against Barrier(N)); RunWithTimeout bounds dispose-storms so a production deadlock fails cleanly instead of hanging. AssertDeregistered<T> asserts a handle no longer resolves to a specific wrapper by reference identity, which is safe against native address reuse under xUnit's parallel collections (asserting the address is simply absent is racy — a parallel test can reallocate a fresh wrapper at the same freed pointer). - SKHandleDictionaryReservationTest.cs: concurrent same-handle construction resolves to exactly one wrapper; waiters only observe a fully-constructed wrapper; factory failure clears the reservation and lets a waiter/retry reconstruct; cross-handle construction in opposite order does not deadlock under the single lock. - SKHandleDictionaryDisposeProtected.cs: dispose-protected construction sets the flag; a racing public Dispose() never tears down a dispose-protected wrapper. - SKHandleDictionaryStaleWrapperTest.cs: a stale wrapper deregistering after address reuse never evicts the live replacement (GetOrAdd and direct-ctor paths, including two stale wrappers vs a third replacement). - SKManagedStreamTest.cs / SKCodecTest.cs: exercise the managed-Stream callback path that the existing SKMemoryStream tests don't reach — public Dispose() ignored while a codec read is in flight; lazy decode after ownership transfer; non-seekable/front-buffered reparenting tears down the nested stream exactly once; failed Create closes the stream exactly once (seekable and non-seekable); PDF/SVG writer streams stay open until the owner is disposed; concurrent dispose of many/same managed streams is safe. Also corrects the InvalidStreamIsDisposedImmediately expectation in three files: the failed-create path runs RevokeOwnership(null) -> DisposeInternal(), which genuinely disposes (and deregisters) the wrapper rather than marking it IgnorePublicDispose. ff18b0f's blanket IsDisposed -> IgnorePublicDispose rename was right for the success/reparent path but wrong here, where there is no new owner to protect the wrapper for. Finally, fixes an intermittent flake in the existing ManagedStreamIsCollectedWhenTypefaceIsDisposed: it used the same racy absence assertion (Assert.False(GetInstance(handle, out _))) that fails when a parallel test reuses the freed native pointer. Switched to the reference-identity AssertDeregistered<T> helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
⚗️ DRAFT / EXPERIMENT — not for merge. Opened to answer one question: does lock-striping the
HandleDictionaryhave any measurable value worth pursuing? Answer below: conditional yes, and the original deadlock blocker is now resolved by a reservation-gate construction redesign.Stacked on #4080 (base =
dev/issue-3817-fix-singleton-init, so this diff shows only the striping change). Tracks #4101.What this does
Replaces the single global
HandleDictionary.instancesLock+ singleDictionarywith N shards, each its ownDictionary+IPlatformLock(+ DEBUGstackTraces). A handle is routed to exactly one shard via a golden-ratio bit-mix:N = next-pow2(Environment.ProcessorCount)clamped[1, 64].N=1is byte-for-byte today's behavior (safety fallback + benchmark baseline).IPlatformLockis public/locked):AppContext.GetData("SkiaSharp.HandleDictionary.ShardCount")→ envSKIASHARP_HANDLE_SHARDS→ auto. Read once at first use.PlatformLock.Factory()is called N times (one lock per shard) — the existing plug-in contract is preserved unchanged.instances/stackTracesare now aggregating views over all shards soGarbageCleanupFixtureand diagnostics keep working.Why it might help
Every wrapper construction (
GetOrAddObject) and disposal (SKObject.Dispose) takes the registry lock. Under heavy multi-threaded creation/teardown of distinct objects, that single lock serializes everything. Sharding lets disjoint handles proceed in parallel.Benchmark — does it actually help? (12-core, Server GC, 7-rep median)
Churn of distinct handles, with realistic outside-lock work (50 spins/op):
Knee ≈
ProcessorCount. Golden-ratio mix spreads aligned pointers perfectly (CoV ≈ 0%).Counter-signal — shared-singleton workload (all threads hit ~8 shared handles):
Here striping hurts (0.5–0.8x): contention is on a few handles that can't be spread, and the extra locks just bounce cache lines. The hot singletons (
SKColorSpace.Srgb, etc.) are exactly this case.Honest verdict
This is "worth a guarded experiment," not a slam-dunk.
✅ Cross-shard deadlock blocker — RESOLVED (reservation-gate two-phase construction)
The first cut of this prototype ran the wrapper factory under the shard lock. With one global lock a factory that constructs a second, different-handle
SKObjectsimply re-enters the same lock; with striping that second object may hash to a different shard, so a thread holding shard A grabs shard B while another holding B wants A → a classic AB-BA cross-shard deadlock that cannot occur today.Commit
28c63c1removes this structurally by reworkingGetOrAddObjectso the factory never runs under any shard lock:Reservationand release the lock.RegisterHandlesees our reservation (same thread + handle) and suppresses publication, so a half-built wrapper is never visible to other threads.instances, drop the reservation, then signal the gate so waiters wake to a fully-constructed object.Because no shard lock is ever held across the factory, a thread holds at most one shard lock at any instant → the cross-shard cycle is impossible, not merely unlikely. In-flight handles live in a separate per-shard
reservationsmap, soinstancesstill only ever holds finished wrappers andGetInstance/DeregisterHandle/ the diagnostics aggregators needed no changes.This also avoids the rejected "construct then dispose the duplicate" dedupe strategies: exactly one wrapper is ever constructed per handle, so there is no loser to dispose and no observable half-built object that user code might already hold.
Residual hazards (documented, tested where deterministic):
GetOrAddObjectfor the same handle) is detected and thrown — never hung.ManualResetEventSlimallocation on the contended path; GC reclaims it.Test status
GarbageCleanupFixtureworks (no leak/exception drain).SKHandleDictionaryReservationTest(white-box, drives the registry with a non-owningFakeNativeObject) pins every claim of the reservation-gate design — all green:ConcurrentSameHandleConstructsExactlyOnce— 32 threads + barrier; factory runs once; allSame.WaitersOnlyObserveFullyConstructedWrapper— publication safety; waiters always observe a fully-built wrapper.ReentrantSameThreadSameHandleThrows— fail-fast under a timeout guard.FactoryFailureClearsReservationAndAllowsReconstruction+ConcurrentFactoryFailureRecoveredByWaiter— a throwing owner clears the reservation; a waiter reconstructs.NestedDifferentHandleConstructionRegistersBoth— single-thread nesting completes.CrossShardOppositeOrderConstructionDoesNotDeadlock— the live deadlock regression: probes two distinct shards viaGetLockForidentity (skips ifShardCount == 1), drives the exact AB-BA interleaving, asserts completion under a 15s timeout. The old factory-under-lock design hangs here.DisposeProtectedFreshConstructionSetsFlag— Phase 3 setsIgnorePublicDisposeunder the write lock.SKManagedStream.OnReadManagedStreamNRE host-crash — intermittent, reproduces identically on the Rework the lifecycle of singleton instances #4080 base branch, unrelated to striping.cc @ramezgerges — purely exploratory, building on your #4080 work. Not asking you to take this on; just parking the data + the deadlock-free redesign so we can decide together whether it's worth a real PR later.