From 707275b46a0730dc6f73fc0ba3fc473e73340b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 17 Aug 2026 00:18:58 +0200 Subject: [PATCH] Fix deadlock when stopping a game with a debugger request in flight The WebSocket thread holds lifecycleLock across a whole event handler, and handlers do their real work through Core_RunOnCPUThread(), which blocks until the CPU thread drains the queue. Meanwhile PSP_Shutdown() -> Core_NotifyLifecycle(STOPPING) took that same lock on the CPU thread. So the debugger thread waited for the CPU thread while the CPU thread waited for the lock the debugger thread was holding, and neither ever moved. Drain the CPU queue while waiting for the lock instead of blocking on it. Core state is still alive at STOPPING (it's notified before CPU_Shutdown), so running those queued callbacks then is safe, and it lets the debugger thread finish and release the lock. Verified with a temporary instrumented build - a 3s sleep inside a handler while holding lifecycleLock, arranged to overlap the game's shutdown - which hangs reliably on the old code and exits cleanly with this change. lifecycleLock stays for now: roughly half the subscribers and all the broadcasters still read core state directly on the WebSocket thread instead of going through the queue, and this is what keeps that from racing with teardown. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9 --- AGENTS.md | 37 +++++++++++++++++++++++++++++++++++++ Core/Debugger/WebSocket.cpp | 12 +++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 65788eadf7..ce7071bd40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -383,6 +383,43 @@ Two more things found while doing this: `CDisasm::NotifyMapLoaded()`, which locks it internally) will deadlock. Keep such calls outside the queued lambda, same as the modal-dialog and `SendMessage()` rules above. +### Lock ordering: `g_frameMutex` before `Memory::Lock()`, always + +`Memory::Lock()` / `Memory::MemoryInitedLock` (a `recursive_mutex`, `g_shutdownLock` in +`Core/MemMap.cpp`) guards the memory system against being torn down or reinitialized under you. When +a function needs both it and `g_frameMutex`, **take `g_frameMutex` first**. + +The CPU thread's order is structural and can't be changed: `NativeFrame()` wraps everything below it +in `g_frameMutex`, and several things under there lock memory - `Core_ProcessCPUQueue()` running a +queued WebSocket handler, and `runImDebugger()` -> `ImMemView` -> `DisassembleRange()`. So the +GUI-thread side is the one that has to match. (Getting it backwards deadlocked for real: a paint +handler held the memory lock and waited for `g_frameMutex` while the CPU thread did the reverse.) + +Also: **a `Core_RunOnCPUThread()` callback does not need `Memory::Lock()`** - teardown only happens +on the CPU thread itself (`Memory::Shutdown()` via `CPU_Shutdown()` <- `PSP_Shutdown()`, all callers +on that thread; `Memory::Reinit()` from `Memory::DoState()` on savestate load). Don't add one. + +**The general rule behind both of these: never make the CPU thread wait for a thread that is (or may +be) waiting on the CPU thread.** `Core_RunOnCPUThread()` blocks until the CPU thread drains the +queue, so anything the CPU thread might block on must not be held across such a call. The WebSocket +debugger's `lifecycleLock` hit exactly this - it's held across a whole event handler, and the CPU +thread took it in `Core_NotifyLifecycle(STOPPING)`, so stopping a game with a debugger request in +flight hung both threads. Resolved by draining the queue while waiting for the lock rather than +blocking on it outright (`WebSocketNotifyLifecycle` in `Core/Debugger/WebSocket.cpp`). + +`lifecycleLock` itself can't just be deleted, tempting as it looks: about half the subscribers +(`GameSubscriber`, `GPU*Subscriber`, `InputSubscriber`, `MemoryInfoSubscriber`, `ReplaySubscriber`, +`ClientConfigSubscriber`) and all four broadcasters still read core state directly on the WebSocket +thread rather than going through `Core_RunOnCPUThread()`. It's what stops that racing with teardown. +Routing those through the queue is the prerequisite for removing it. + +The Win32 debugger's paint handlers *do* still need it, though, so don't "simplify" those away: +teardown is not yet fully inside the `g_frameMutex` span. `EmuScreen::render()`'s `PSP_Shutdown()` is +inside it, but the ones in `EmuScreen::sendMessage()` (`REQUEST_GAME_RESET`, loading a new game) run +from `g_screenManager->sendMessage()` in `NativeFrame()`, which sits *above* where the guard is +taken. Closing that hole - moving those shutdowns inside the span, or deferring them to render time - +is the prerequisite for dropping `Memory::Lock()` from the debugger entirely. + Painting-problem design history, in case a similar tradeoff comes up elsewhere: routing every paint through `Core_RunOnCPUThread` was rejected as too slow for something invoked continuously. A per-window snapshot/cache with a per-row-rechecked `Core_IsStepping()` guard was tried first and diff --git a/Core/Debugger/WebSocket.cpp b/Core/Debugger/WebSocket.cpp index 7603ac6192..8aa1f00a4e 100644 --- a/Core/Debugger/WebSocket.cpp +++ b/Core/Debugger/WebSocket.cpp @@ -19,6 +19,8 @@ #include #include "Common/Thread/ThreadUtil.h" +#include "Common/TimeUtil.h" +#include "Core/Core.h" #include "Core/Debugger/WebSocket.h" #include "Core/Debugger/WebSocket/WebSocketUtils.h" @@ -108,7 +110,15 @@ static void WebSocketNotifyLifecycle(CoreLifecycle stage) { if (debuggersConnected > 0) { DEBUG_LOG(Log::System, "Waiting for debugger to complete on shutdown"); } - lifecycleLock.lock(); + // Keep draining the CPU queue while we wait, instead of a plain blocking lock(). We're on + // the CPU thread here, and a debugger thread holding this lock may be parked inside + // Core_RunOnCPUThread() waiting for us to run its callback - so blocking outright means + // neither side can ever move. Core state is still fully alive at this point (STOPPING is + // notified before CPU_Shutdown), so running those callbacks now is safe. + while (!lifecycleLock.try_lock()) { + Core_ProcessCPUQueue(); + sleep_ms(1, "debugger-lifecycle"); + } break; case CoreLifecycle::START_COMPLETE: