From f2a51bbb066ffb69d1bef4ac042011335d2a51ed Mon Sep 17 00:00:00 2001 From: vide Date: Thu, 3 Jul 2025 18:01:26 +0300 Subject: [PATCH] Queue Lua MESSAGEMAN:Broadcast to be ran on main thread Works around a potential deadlock: T1 = Thread 1 (main thread) T2 = Thread 2 (background thread) M = MessageManager mutex L = Lua mutex 1. MessageManager::Broadcast is looping subscribers on T1, while keeping M locked. Subscribers might lock L. 2. A network request completes on T2, obtains lock on L before M is released. Lua code wants to run MessageManager::Broadcast, which waits for M to be released. We are now in a deadlock as T1 is waiting for L while holding M and T2 is waiting for M while holding L. This commit changes the Lua API to never run MessageManager::Broadcast from a non-main thread. Instead, a Message is allocated and stored in a queue, which the main thread checks in the main GameLoop. Access to the queue is guarded with a separate mutex. No other mutexes are obtained while this new mutex is locked. With this change, it should no longer be possible for M to be waited on while L is locked from a non-main thread and when attempting to broadcast. (Potentially, it could be possible for MessageManager::Broadcast to lock M and L on the main thread and run Lua which attempts to Broadcast again, that would try to obtain M which is already locked... I did not investigate whether there are some safeguards preventing this.) --- src/GameLoop.cpp | 3 +++ src/MessageManager.cpp | 36 +++++++++++++++++++++++++++++++++--- src/MessageManager.h | 12 +++++++++++- src/StepMania.cpp | 1 + 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/GameLoop.cpp b/src/GameLoop.cpp index c3cb194b27..7e18dc97ea 100644 --- a/src/GameLoop.cpp +++ b/src/GameLoop.cpp @@ -304,6 +304,9 @@ void GameLoop::UpdateAllButDraw(bool bRunningFromVBLANK) // Update the lights LIGHTSMAN->Update(fDeltaTime); + + // Process broadcast queue from other threads + MESSAGEMAN->HandleQueuedBroadcasts(); } void GameLoop::RunGameLoop() diff --git a/src/MessageManager.cpp b/src/MessageManager.cpp index 5a6420c650..82ddc53bc7 100644 --- a/src/MessageManager.cpp +++ b/src/MessageManager.cpp @@ -5,6 +5,8 @@ #include "EnumHelper.h" #include "LuaManager.h" #include "RageLog.h" +#include "arch/Threads/Threads.h" +#include "RageThreads.h" #include #include @@ -154,7 +156,7 @@ void Message::SetParamFromStack( lua_State *L, const RString &sName ) m_pParams->Set( L, sName ); } -MessageManager::MessageManager() +MessageManager::MessageManager() : m_messagemanBroadcastQueue_mutex("m_messagemanBroadcastQueue") { m_Logging= false; // Register with Lua. @@ -205,6 +207,26 @@ void MessageManager::Unsubscribe( IMessageSubscriber* pSubscriber, MessageID m ) Unsubscribe( pSubscriber, MessageIDToString(m) ); } +/** @brief Queue Message to be ran on the main thread. */ +void MessageManager::QueueBroadcast(std::unique_ptr msg) { + m_messagemanBroadcastQueue_mutex.Lock(); + m_messagemanBroadcastQueue.push(std::move(msg)); + m_messagemanBroadcastQueue_mutex.Unlock(); +} + +/** @brief Check message queue for broadcasts. ONLY CALL FROM MAIN THREAD. */ +void MessageManager::HandleQueuedBroadcasts() { + if (!m_messagemanBroadcastQueue.empty()) { + m_messagemanBroadcastQueue_mutex.Lock(); + if (!m_messagemanBroadcastQueue.empty()) { + std::unique_ptr message = std::move(m_messagemanBroadcastQueue.front()); + m_messagemanBroadcastQueue.pop(); + MESSAGEMAN->Broadcast(*message.get()); + } + m_messagemanBroadcastQueue_mutex.Unlock(); + } +} + void MessageManager::Broadcast( Message &msg ) const { if(m_Logging) @@ -289,6 +311,7 @@ void MessageSubscriber::UnsubscribeAll() // lua start #include "LuaBinding.h" +#include "RageThreads.h" /** @brief Allow Lua to have access to the MessageManager. */ class LunaMessageManager: public Luna @@ -303,8 +326,15 @@ class LunaMessageManager: public Luna lua_pushvalue( L, 2 ); ParamTable.SetFromStack( L ); - Message msg( SArg(1), ParamTable ); - p->Broadcast( msg ); + if (p->m_mainThreadID != RageThread::GetCurrentThreadID()) { + // Message constructor copies the string and LuaReference ParamTable, they're not left dangling + std::unique_ptr msg(new Message(SArg(1), ParamTable)); + p->QueueBroadcast(std::move(msg)); + } else { + Message msg( SArg(1), ParamTable ); + p->Broadcast( msg ); + } + COMMON_RETURN_SELF; } static int SetLogging(T* p, lua_State *L) diff --git a/src/MessageManager.h b/src/MessageManager.h index a2ab2cecdd..7cf789b4b5 100644 --- a/src/MessageManager.h +++ b/src/MessageManager.h @@ -4,7 +4,9 @@ #include "LuaManager.h" #include - +#include +#include +#include "RageThreads.h" struct lua_State; class LuaTable; @@ -207,8 +209,16 @@ class MessageManager void SetLogging(bool set) { m_Logging= set; } bool m_Logging; + uint64_t m_mainThreadID = 0; + void QueueBroadcast(std::unique_ptr message); + void HandleQueuedBroadcasts(); + // Lua void PushSelf( lua_State *L ); + + private: + std::queue> m_messagemanBroadcastQueue; + RageMutex m_messagemanBroadcastQueue_mutex; }; extern MessageManager* MESSAGEMAN; // global and accessible from anywhere in our program diff --git a/src/StepMania.cpp b/src/StepMania.cpp index 573410b8cf..4bf754f552 100644 --- a/src/StepMania.cpp +++ b/src/StepMania.cpp @@ -879,6 +879,7 @@ int sm_main(int argc, char* argv[]) // Set up the messaging system early to have well defined code. MESSAGEMAN = new MessageManager; + MESSAGEMAN->m_mainThreadID = RageThread::GetCurrentThreadID(); // Create game objects GAMESTATE = new GameState;