From ee456ccc40f6c9025a5668364dd87dd62c034119 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 24 Sep 2026 14:03:47 +0000 Subject: [PATCH] [MG_Util, MG_Remote] (P12 review): the in-process display server forwards only its live session's own log lines to that session's client, and sends them outside the log mutex SetLogForwarder forwards every server-role thread's line, which is right for a session child (the process is the session) and wrong for the display Activity's process, where MOBILEGL_IPC_ROLE=server makes every thread a server thread: the UI thread's surface callbacks and the listener's lines - naming other peers' addresses, Busy refusals included - went into the live client's control socket. And the forward was a blocking SendFrame under the global log mutex, so a client that stopped reading held that mutex and hung the UI thread at its next log line (surfaceDestroyed's Detach logs). In-process, RunSession now installs SetSessionLogForwarder: only the session thread and the threads that opt in (the session's apply thread, SetThreadForwardsLogToPeer) forward, and the send happens after the log mutex is released, under a forward mutex of its own (WithLogBarrier takes both, so the flush ack still follows every send that began; a line logged by the send itself is not re-sent). The forked session child keeps SetLogForwarder unchanged; the pull build's Log() is unchanged (G1). --- MobileGL/MG_Remote/Server/ServerLoop.cpp | 3 ++ MobileGL/MG_Remote/Server/ServerMain.cpp | 9 ++++- MobileGL/MG_Util/Debug/Log.cpp | 51 +++++++++++++++++++++++- MobileGL/MG_Util/Debug/Log.h | 13 ++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/MobileGL/MG_Remote/Server/ServerLoop.cpp b/MobileGL/MG_Remote/Server/ServerLoop.cpp index c81ee6f8..158c9025 100644 --- a/MobileGL/MG_Remote/Server/ServerLoop.cpp +++ b/MobileGL/MG_Remote/Server/ServerLoop.cpp @@ -362,6 +362,9 @@ namespace MobileGL::MG_Remote::Server { // own log. Under spawn the process role already answers this; under INPROC it is the // only thing that can, because the client role is another thread of this same process. MG_Util::Debug::SetThreadLogRole(MG_Util::Debug::LogRole::Server); + // P12 review fix: and it is one of the session's own threads, whose lines a session-scoped + // forwarder (the in-process display server's) sends to the client. Inert otherwise. + MG_Util::Debug::SetThreadForwardsLogToPeer(true); Bool recognised = true; const char* raw = AffinityStringFromConfig(); diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index 0d09eb9b..42d0ea8b 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -574,8 +574,13 @@ int RunSession(std::unique_ptr control, std::vector lock(LogMutex()); + std::lock_guard forwardLock(ForwardMutex()); s_forwarder = forwarder; s_forwarderUser = user; + s_forwardSessionThreadsOnly = false; } + void SetSessionLogForwarder(LogForwarder forwarder, void* user) { + { + std::lock_guard lock(LogMutex()); + std::lock_guard forwardLock(ForwardMutex()); + s_forwarder = forwarder; + s_forwarderUser = user; + s_forwardSessionThreadsOnly = forwarder != nullptr; + } + t_forwardsToPeer = forwarder != nullptr; + } + + void SetThreadForwardsLogToPeer(bool forwards) { t_forwardsToPeer = forwards; } + void WithLogBarrier(void (*action)(void*), void* user) { std::lock_guard lock(LogMutex()); + // A session-scoped line is sent under ForwardMutex after the log mutex was released; the + // barrier's action comes after every such send that has begun. + std::lock_guard forwardLock(ForwardMutex()); action(user); } @@ -244,7 +271,12 @@ namespace MobileGL { } void Log(const char* levelTag, android_LogPriority androidLogLevel, const char* fmt, ...) { +#if MOBILEGL_BUILD_DISAGGREGATED + // Unlocked early for a session-scoped forward (SetSessionLogForwarder). + std::unique_lock lock(LogMutex()); +#else std::lock_guard lock(LogMutex()); +#endif #if MOBILEGL_LOG_ENABLE_STACKTRACE auto trace = std::stacktrace::current(); @@ -283,10 +315,27 @@ namespace MobileGL { WriteToFile(out.c_str()); #if MOBILEGL_BUILD_DISAGGREGATED - if (s_forwarder && ThreadIsServerRole()) s_forwarder(s_forwarderUser, out.c_str()); + bool forwardOutsideLock = false; + if (s_forwarder && ThreadIsServerRole() && !t_forwarding) { + if (!s_forwardSessionThreadsOnly) s_forwarder(s_forwarderUser, out.c_str()); + else forwardOutsideLock = t_forwardsToPeer; + } #endif va_end(args); +#if MOBILEGL_BUILD_DISAGGREGATED + if (forwardOutsideLock) { + // SetSessionLogForwarder: this thread belongs to the live session. The line goes out + // after the log mutex is released, so a stalled client holds only the forward mutex. + lock.unlock(); + std::lock_guard forwardLock(ForwardMutex()); + if (s_forwarder != nullptr && s_forwardSessionThreadsOnly) { + t_forwarding = true; + s_forwarder(s_forwarderUser, out.c_str()); + t_forwarding = false; + } + } +#endif } void Close() { diff --git a/MobileGL/MG_Util/Debug/Log.h b/MobileGL/MG_Util/Debug/Log.h index d02ca02c..0a402ad4 100644 --- a/MobileGL/MG_Util/Debug/Log.h +++ b/MobileGL/MG_Util/Debug/Log.h @@ -138,6 +138,19 @@ namespace MobileGL { #if MOBILEGL_BUILD_DISAGGREGATED using LogForwarder = void (*)(void*, const char*); void SetLogForwarder(LogForwarder forwarder, void* user); + // P12 review fix (log forwarding). THE IN-PROCESS DISPLAY SERVER'S FORWARDER, scoped to + // the live session's own threads. SetLogForwarder forwards every server-role thread's + // line, which is right for a session child (the process IS the session) and wrong for the + // display Activity's process, where the UI thread, the listener (naming other peers' + // addresses) and a previous session share the process: their lines went into the live + // client's socket. This one forwards only from the calling thread and from threads that + // opt in (SetThreadForwardsLogToPeer - the session's apply thread), and it sends OUTSIDE + // the log mutex, under a forward mutex of its own, so a client that stops reading stalls + // the session's threads and not every thread that logs (the UI thread's surface + // callbacks). SetLogForwarder(nullptr, nullptr) clears either kind and waits for a send in + // flight. + void SetSessionLogForwarder(LogForwarder forwarder, void* user); + void SetThreadForwardsLogToPeer(bool forwards); void WritePeerLog(const char* message); void WithLogBarrier(void (*action)(void*), void* user); // P6: ONE LOG PER ROLE, and the role is a per-THREAD fact because under inproc both