Skip to content

Upload one GPU texture per path under concurrent first access - #418

Merged
matt-edmondson merged 1 commit into
mainfrom
claude/nice-davinci-cqj69a
Sep 16, 2026
Merged

matt-edmondson merged 1 commit into
mainfrom
claude/nice-davinci-cqj69a

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #405

The bug

GetOrLoadTexture did an unsynchronised check-then-act: it checked Textures, and on a miss decoded the image, uploaded a GPU texture via UploadTextureRGBA, then assigned Textures[path].

Two threads reaching the same uncached path together both miss the cache, both upload a separate texture, and both assign Textures[path]. Only the last assignment is reachable — the other handle can't be found by DeleteTexture, CleanupAllTextures or ReloadAllTextures, and leaks for the life of the GL context. Background asset loading makes this reachable in practice: the Invoker/GL-thread marshaling exists precisely to support calling into ImGuiApp from non-UI threads.

The fix

Move the cache re-check and the publish inside the same Invoker.Invoke that already performs the upload.

Invoke bodies never overlap — they run inline on the invoker thread, and from every other thread they are queued and drained one at a time — so the invoker thread becomes the single place a texture for a given path can be created. The re-check inside that body therefore cannot be raced.

Decoding deliberately stays outside the Invoke: it touches no GPU state, and moving it in would drag every background load onto the render thread. A racing thread may decode the image twice, which costs CPU but leaks nothing.

Why not a lock

Guarding the sequence with a per-path lock or Lazy<T> (as the issue suggests) would introduce a deadlock that doesn't exist today: a caller holding the gate blocks inside Invoke waiting for the render thread to pump DoInvokes, and the render thread — which calls GetOrLoadTexture from application draw code — would then block on that same gate instead of pumping. Serialising on the invoker thread, which the upload already marshals to, gets single-flight semantics with no new blocking primitive.

Test

GetOrLoadTexture_WithConcurrentFirstAccessToOnePath_UploadsExactlyOneTexture drives four threads at one uncached path, with the test thread standing in for the render thread and holding back the pump so every caller lands on the uncached side of the check. It reuses the existing FakeRendererBackend seam to count uploads without a GL context.

Verified by reverting the ImGuiApp.cs change and re-running: fails with expected: 1, actual: 4, passes with the fix.

Checks run

  • tests/ImGui.App.Tests — 428/428 pass
  • tests/ImGui.App.Testing.Tests — 82/82 pass
  • Build clean, 0 warnings

tests/ImGuiAppDemo.UITests fails 26/26 in my container, but it fails identically on the base commit — the failure is an embedded-resource lookup in BuildConfig() during SetUp, unrelated to this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LyFvutjJ2Po2WmjiL7z9iP


Generated by Claude Code

GetOrLoadTexture checked the texture cache, decoded the image and uploaded
a new GPU texture without any synchronisation around the sequence. Two
threads reaching the same uncached path together both missed the cache,
both uploaded a separate texture, and both assigned Textures[path]. Only
the last assignment was reachable, so the other handle could not be found
by DeleteTexture, CleanupAllTextures or ReloadAllTextures and leaked for
the life of the GL context. Background asset loading makes this reachable:
the invoker exists to marshal exactly those calls onto the render thread.

Move the cache re-check and the publish inside the same Invoker.Invoke
that already performs the upload. Invoke bodies never overlap - they run
inline on the invoker thread and are drained one at a time from every
other thread - so the invoker thread becomes the single place a texture
for a path can be created. Decoding deliberately stays outside, since it
touches no GPU state and moving it in would drag every background load
onto the render thread; a racing thread may decode twice, which costs CPU
but leaks nothing.

Locking around the sequence instead would deadlock: a caller holding the
lock blocks inside Invoke waiting for the render thread to pump, and the
render thread would then block on that same lock instead of pumping.

Fixes #405

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LyFvutjJ2Po2WmjiL7z9iP

Copy link
Copy Markdown
Contributor Author

Test on ubuntu-latest is red, and it isn't this PR's

What's failing: Test on ubuntu-latest (job). 1723 tests, 26 failures — all 26 in ImGuiAppDemo.UITests, every one of them AppDemoUITests.SetUp throwing before a test body runs. Every other test project passes, including ImGui.App.Tests (which carries this PR's new test) and ImGui.App.Testing.Tests. Test on windows-latest and Test on macos-latest both pass, as do CodeQL and the iOS build.

Why it isn't this PR's: the same job fails on main at 823c158 — the exact commit this branch is based on — in run 35048898822, with the same ubuntu-only pattern (windows and macOS green there too). The two scheduled main runs before it are red the same way; the last green main run was 1789 at ea7f9e6. This PR changes only ImGui.App/ImGuiApp.cs and tests/ImGui.App.Tests/ImGuiAppTests.cs, and touches nothing under examples/.

Root cause:

System.Resources.MissingManifestResourceException: Could not find the resource
"ktsu.examples.ImGuiAppDemo.Properties.Resources.resources" among the resources
"ktsu.ImGuiAppDemo.Properties.Resources.resources" embedded in the assembly "ktsu.ImGuiAppDemo"
   at ktsu.ImGui.Examples.App.Properties.Resources.get_CARDCHAR() in examples/ImGuiAppDemo/Properties/Resources.Designer.cs:68
   at ktsu.ImGui.Examples.App.ImGuiAppDemo.BuildConfig() in examples/ImGuiAppDemo/ImGuiAppDemo.cs:52
   at ktsu.examples.ImGuiAppDemo.UITests.AppDemoUITests.SetUp() in tests/ImGuiAppDemo.UITests/AppDemoUITests.cs:44

The checked-in Resources.Designer.cs hardcodes the base name ktsu.examples.ImGuiAppDemo.Properties.Resources, but the embedded resource is named from RootNamespace, and on Linux that now resolves to ktsu.ImGuiAppDemo — I confirmed this locally:

$ dotnet msbuild examples/ImGuiAppDemo/ImGuiAppDemo.csproj -getProperty:RootNamespace
{ "Properties": { "RootNamespace": "ktsu.ImGuiAppDemo" } }

The examples. infix is missing. Since the windows and macOS legs pass, it presumably still resolves to ktsu.examples.ImGuiAppDemo there, which would point at path-separator handling in whatever derives RootNamespace from the project's directory. The first red run is 1791 at 84e5222, "Bump the ktsu group with 20 updates", so a ktsu.Sdk bump in that batch is the likely trigger — I haven't bisected the individual package to confirm.

No fix exists to port, so I'm not widening this PR with one. A proposed patch, for whoever picks this up — pinning the resource's logical name to what the designer file asks for:

<EmbeddedResource Update="Properties\Resources.resx">
  <Generator>ResXFileCodeGenerator</Generator>
  <LastGenOutput>Resources.Designer.cs</LastGenOutput>
  <LogicalName>ktsu.examples.ImGuiAppDemo.Properties.Resources.resources</LogicalName>
</EmbeddedResource>

I tried this locally and it does clear the MissingManifestResourceException — but it then uncovers a second, separate failure in the same suite (InvalidImageDataException: Unrecognised image format; the file starts with 76 65 72 73, i.e. ASCII vers), so it is not a complete fix on its own. Fixing RootNamespace in the SDK rather than pinning LogicalName here may well be the better direction.

Not re-running the job: this is deterministic, not a flake — it reproduces identically on the base commit both in CI and locally, so a re-run would only confirm what the main run already shows.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GetOrLoadTexture leaks a GPU texture when the same uncached path is requested concurrently

2 participants