Merge pull request #22251 from hrydgard/log-callback-list

Let more than one thing receive the log stream at a time

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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> lock(ringLock_);
 	messages_[curMessage_] = message;

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<ExternalCallbackEntry> externalCallbacks_;
+	int nextExternalHandle_ = 1;
 };
 
 extern LogManager g_logManager;

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_;
 }
 

Core/Debugger/WebSocket/LogBroadcaster.h

@@ -32,4 +32,5 @@ public:
 
 private:
 	DebuggerLogListener *listener_;
+	int callbackHandle_ = -1;
 };

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