fix: set surface event before SetFormat to prevent draw thread deadlock - #356
Conversation
The previous approach (surfaceEvent.Reset() + return) caused the draw thread to block on surfaceEvent.Wait(5000) while SetFormat triggered a synchronous surface teardown. The draw thread never got a valid surface handle and the native watchdog fired. New approach: Set surfaceEvent BEFORE calling SetFormat so the draw thread can proceed with the current (soon-to-be-recreated) surface. The native watchdog is also ticked before SetFormat to buy time for the synchronous teardown. Order: surfaceEvent.Set() → NativeWatchdog.Heartbeat() → holder.SetFormat()
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 46 minutes and 33 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR reorders the Vulkan RGB565→RGBA8888 surface format recovery path in ChangesRGB565→RGBA8888 Recovery Event Sequencing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review feedback: setting surfaceEvent before SetFormat is dangerous because the draw thread can get a stale surface pointer that's being torn down. New approach: Set surfaceEvent AFTER SetFormat returns. By this point, the synchronous SurfaceDestroyed→SurfaceCreated→SurfaceChanged cycle has already completed and surfaceGlobalRef points to the new RGBA8888 surface. This preserves the invariant that surfaceEvent is only set when surfaceGlobalRef points to a safe-to-render surface. The NativeWatchdog.Heartbeat() is still called before SetFormat to reset the watchdog timer during the blocking teardown.
Problem
The reactive RGB565 guard in SurfaceChanged calls SetFormat(RGBA8888) which
triggers a synchronous surface teardown on the UI thread. The previous code
did surfaceEvent.Reset() + return, which caused the draw thread to block
indefinitely on surfaceEvent.Wait(5000). The native watchdog then fired
after 10s with 'no managed heartbeat ever observed'.
Fix
Files changed
Summary by CodeRabbit