From e26e2d28a0391b55cbbe4b17716769c33751f0a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 7 Sep 2026 15:15:10 -0600 Subject: [PATCH] Let more than one thing receive the log stream at a time The log manager had a single external-callback slot, but the WebSocket debugger registers one per *connection* - LogBroadcaster is a local in the per-connection handler. So with two clients attached (the bundled JS debugger in a browser and Tools/wsdbg, say) the second one to connect silently took the log stream away from the first, and then whichever disconnected first cleared the slot and stopped delivery to the other as well. A one-shot wsdbg command is enough to do it: connect, take the stream, exit, and the long-lived listener that was watching the log goes quiet with nothing to say why. Make it a list with add/remove by handle. The dispatch loop holds the lock across the callbacks so a listener can't be freed while one is running - which is what lets LogBroadcaster delete its listener straight after removing it. Enabling and disabling LogOutput::ExternalCallback belongs to the list now, and disabling only happens when the last callback goes away. libretro registers one of these too, and never removes it; it just moves to the new call. It can't actually collide with the debugger - the libretro build doesn't compile Core/Debugger/WebSocket at all - but there's no reason for it to keep using an API that only has room for one caller. Co-Authored-By: Claude Opus 5 --- Common/Log/LogManager.cpp | 36 ++++++++++++++++++++-- Common/Log/LogManager.h | 25 ++++++++++----- Core/Debugger/WebSocket/LogBroadcaster.cpp | 9 +++--- Core/Debugger/WebSocket/LogBroadcaster.h | 1 + libretro/libretro.cpp | 6 ++-- 5 files changed, 62 insertions(+), 15 deletions(-) diff --git a/Common/Log/LogManager.cpp b/Common/Log/LogManager.cpp index 20a3569f47..be728c9bcd 100644 --- a/Common/Log/LogManager.cpp +++ b/Common/Log/LogManager.cpp @@ -364,12 +364,44 @@ void LogManager::LogLine(LogLevel level, Log type, const char *file, int line, c #endif if (outputs_ & LogOutput::ExternalCallback) { - if (externalCallback_) { - externalCallback_(message, externalUserData_); + // Held across the dispatch on purpose: RemoveExternalLogCallback() takes the same lock, so a + // listener can't be torn down (and its userdata freed) while we're in the middle of calling it. + std::lock_guard guard(externalLock_); + for (const ExternalCallbackEntry &entry : externalCallbacks_) { + entry.callback(message, entry.userdata); } } } +int LogManager::AddExternalLogCallback(LogCallback callback, void *userdata) { + if (!callback) { + return -1; + } + std::lock_guard guard(externalLock_); + const int handle = nextExternalHandle_++; + externalCallbacks_.push_back(ExternalCallbackEntry{ handle, callback, userdata }); + EnableOutput(LogOutput::ExternalCallback); + return handle; +} + +void LogManager::RemoveExternalLogCallback(int handle) { + if (handle < 0) { + return; + } + std::lock_guard guard(externalLock_); + for (size_t i = 0; i < externalCallbacks_.size(); i++) { + if (externalCallbacks_[i].handle == handle) { + externalCallbacks_.erase(externalCallbacks_.begin() + i); + break; + } + } + // Only when the last one goes away - otherwise removing one connection's callback would stop + // delivery to the ones still attached, which is the bug this list exists to avoid. + if (externalCallbacks_.empty()) { + DisableOutput(LogOutput::ExternalCallback); + } +} + void RingbufferLog::Log(const LogMessage &message) { std::lock_guard lock(ringLock_); messages_[curMessage_] = message; diff --git a/Common/Log/LogManager.h b/Common/Log/LogManager.h index 85b393d9de..633942164a 100644 --- a/Common/Log/LogManager.h +++ b/Common/Log/LogManager.h @@ -158,10 +158,15 @@ public: void Init(bool *enabledSetting, bool headless = false); void Shutdown(); - void SetExternalLogCallback(LogCallback callback, void *userdata) { - externalCallback_ = callback; - externalUserData_ = userdata; - } + // A list rather than a single slot, because the WebSocket debugger registers one callback per + // *connection* (LogBroadcaster is a local in the per-connection handler) and more than one + // client can be attached at once - the bundled JS debugger in a browser alongside Tools/wsdbg, + // say. With a single slot the second connection silently took the log stream away from the + // first, and then the first one to disconnect cleared the slot and stopped delivery to the + // other as well. + // Returns a handle to hand back to RemoveExternalLogCallback(), or -1 if it wasn't added. + int AddExternalLogCallback(LogCallback callback, void *userdata); + void RemoveExternalLogCallback(int handle); void SetFileLogPath(const Path &filename); const Path &GetLogFilePath() const { return logFilename_; } @@ -215,9 +220,15 @@ private: // Ring buffer RingbufferLog ringLog_; - // Callback - LogCallback externalCallback_ = nullptr; - void *externalUserData_ = nullptr; + // Callbacks + struct ExternalCallbackEntry { + int handle; + LogCallback callback; + void *userdata; + }; + std::mutex externalLock_; + std::vector externalCallbacks_; + int nextExternalHandle_ = 1; }; extern LogManager g_logManager; diff --git a/Core/Debugger/WebSocket/LogBroadcaster.cpp b/Core/Debugger/WebSocket/LogBroadcaster.cpp index 97cb2fab8c..fbaaec268f 100644 --- a/Core/Debugger/WebSocket/LogBroadcaster.cpp +++ b/Core/Debugger/WebSocket/LogBroadcaster.cpp @@ -100,13 +100,14 @@ static void BroadcastCallback(const LogMessage &message, void *userdata) { LogBroadcaster::LogBroadcaster() { listener_ = new DebuggerLogListener(); - g_logManager.SetExternalLogCallback(&BroadcastCallback, (void *)listener_); - g_logManager.EnableOutput(LogOutput::ExternalCallback); + // One of these exists per open connection, so it registers alongside any other client's + // rather than replacing it - see AddExternalLogCallback(). + callbackHandle_ = g_logManager.AddExternalLogCallback(&BroadcastCallback, (void *)listener_); } LogBroadcaster::~LogBroadcaster() { - g_logManager.DisableOutput(LogOutput::ExternalCallback); - g_logManager.SetExternalLogCallback(nullptr, nullptr); + // Returns only once no log call is inside our callback, so the listener is safe to delete. + g_logManager.RemoveExternalLogCallback(callbackHandle_); delete listener_; } diff --git a/Core/Debugger/WebSocket/LogBroadcaster.h b/Core/Debugger/WebSocket/LogBroadcaster.h index 13f26d4a06..543b168a46 100644 --- a/Core/Debugger/WebSocket/LogBroadcaster.h +++ b/Core/Debugger/WebSocket/LogBroadcaster.h @@ -32,4 +32,5 @@ public: private: DebuggerLogListener *listener_; + int callbackHandle_ = -1; }; diff --git a/libretro/libretro.cpp b/libretro/libretro.cpp index 9af1ce8169..8f2830bdbb 100644 --- a/libretro/libretro.cpp +++ b/libretro/libretro.cpp @@ -1174,8 +1174,10 @@ void retro_init(void) // auto-detection, which only fires if a debugger was already attached before Init() ran) so the // log always shows up in the debugger's Output window when debugging the core in-process with // RetroArch, regardless of where RetroArch itself routes the ExternalCallback log messages. - g_logManager.EnableOutput(LogOutput::ExternalCallback | LogOutput::DebugString); - g_logManager.SetExternalLogCallback(&RetroLogCallback, (void *)log_cb); + g_logManager.EnableOutput(LogOutput::DebugString); + // AddExternalLogCallback() enables LogOutput::ExternalCallback itself. Never removed - this + // callback lives as long as the core does. + g_logManager.AddExternalLogCallback(&RetroLogCallback, (void *)log_cb); } VsyncSwapIntervalReset();