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();