From ffb9cb5410fb48e8f67770dd76dbfa2394502f57 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 14 Apr 2026 08:31:45 -0700 Subject: [PATCH 1/3] Hide arcana behind pimpl in AppRuntime Move arcana types (cancellation_source, manual_dispatcher), thread, env, suspension lock, and the Append template into a private Impl class defined in AppRuntime.cpp. This keeps arcana out of the public header so consumers don't need arcana's include directories. The worker thread is joined before Impl is destroyed, preserving the same lifetime guarantees as before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 32 +--------- Core/AppRuntime/Source/AppRuntime.cpp | 67 +++++++++++++++----- 2 files changed, 55 insertions(+), 44 deletions(-) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 0de98b8d..3a70843a 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -6,16 +6,9 @@ #include -#include -#include - #include #include #include -#include -#include -#include -#include namespace Babylon { @@ -48,23 +41,6 @@ namespace Babylon static void BABYLON_API DefaultUnhandledExceptionHandler(const Napi::Error& error); private: - template - void Append(CallableT callable) - { - if constexpr (std::is_copy_constructible::value) - { - m_dispatcher.queue([this, callable = std::move(callable)]() { - callable(m_env.value()); - }); - } - else - { - m_dispatcher.queue([this, callablePtr = std::make_shared(std::move(callable))]() { - (*callablePtr)(m_env.value()); - }); - } - } - // These three methods are the mechanism by which platform- and JavaScript-specific // code can be "injected" into the execution of the JavaScript thread. These three // functions are implemented in separate files, thus allowing implementations to be @@ -84,10 +60,8 @@ namespace Babylon void Execute(Dispatchable callback); Options m_options; - std::optional m_env{}; - std::optional> m_suspensionLock{}; - arcana::cancellation_source m_cancelSource{}; - arcana::manual_dispatcher<128> m_dispatcher{}; - std::thread m_thread; + + class Impl; + std::unique_ptr m_impl; }; } diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index ae1ee4f9..99298df2 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -1,8 +1,43 @@ #include "AppRuntime.h" + +#include +#include + #include +#include +#include +#include +#include namespace Babylon { + class AppRuntime::Impl + { + public: + template + void Append(CallableT callable) + { + if constexpr (std::is_copy_constructible::value) + { + m_dispatcher.queue([this, callable = std::move(callable)]() { + callable(m_env.value()); + }); + } + else + { + m_dispatcher.queue([this, callablePtr = std::make_shared(std::move(callable))]() { + (*callablePtr)(m_env.value()); + }); + } + } + + std::optional m_env{}; + std::optional> m_suspensionLock{}; + arcana::cancellation_source m_cancelSource{}; + arcana::manual_dispatcher<128> m_dispatcher{}; + std::thread m_thread; + }; + AppRuntime::AppRuntime() : AppRuntime{{}} { @@ -10,8 +45,10 @@ namespace Babylon AppRuntime::AppRuntime(Options options) : m_options{std::move(options)} - , m_thread{[this] { RunPlatformTier(); }} + , m_impl{std::make_unique()} { + m_impl->m_thread = std::thread{[this] { RunPlatformTier(); }}; + Dispatch([this](Napi::Env env) { JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); }); }); @@ -19,9 +56,9 @@ namespace Babylon AppRuntime::~AppRuntime() { - if (m_suspensionLock.has_value()) + if (m_impl->m_suspensionLock.has_value()) { - m_suspensionLock.reset(); + m_impl->m_suspensionLock.reset(); } // Cancel immediately so pending work is dropped promptly, then append @@ -33,44 +70,44 @@ namespace Babylon // callbacks are dropped on cancellation. A more complete solution // would add cooperative shutdown (e.g. NotifyDisposing/Rundown) so // consumers can finish cleanup work before the runtime is destroyed. - m_cancelSource.cancel(); - Append([](Napi::Env) {}); + m_impl->m_cancelSource.cancel(); + m_impl->Append([](Napi::Env) {}); - m_thread.join(); + m_impl->m_thread.join(); } void AppRuntime::Run(Napi::Env env) { - m_env = std::make_optional(env); + m_impl->m_env = std::make_optional(env); - m_dispatcher.set_affinity(std::this_thread::get_id()); + m_impl->m_dispatcher.set_affinity(std::this_thread::get_id()); - while (!m_cancelSource.cancelled()) + while (!m_impl->m_cancelSource.cancelled()) { - m_dispatcher.blocking_tick(m_cancelSource); + m_impl->m_dispatcher.blocking_tick(m_impl->m_cancelSource); } // The dispatcher can be non-empty if something is dispatched after cancellation. - m_dispatcher.clear(); + m_impl->m_dispatcher.clear(); } void AppRuntime::Suspend() { auto suspensionMutex = std::make_shared(); - m_suspensionLock.emplace(*suspensionMutex); - Append([suspensionMutex{std::move(suspensionMutex)}](Napi::Env) { + m_impl->m_suspensionLock.emplace(*suspensionMutex); + m_impl->Append([suspensionMutex{std::move(suspensionMutex)}](Napi::Env) { std::scoped_lock lock{*suspensionMutex}; }); } void AppRuntime::Resume() { - m_suspensionLock.reset(); + m_impl->m_suspensionLock.reset(); } void AppRuntime::Dispatch(Dispatchable func) { - Append([this, func{std::move(func)}](Napi::Env env) mutable { + m_impl->Append([this, func{std::move(func)}](Napi::Env env) mutable { Execute([this, env, func{std::move(func)}]() mutable { try { From 73aa38e0d8ba769cfb1ba69588aad8bfa34f81d5 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 14 Apr 2026 08:41:26 -0700 Subject: [PATCH 2/3] Explicitly delete copy and move for AppRuntime The pimpl change made AppRuntime movable (unique_ptr is movable), but moving an AppRuntime with a running thread and captured this pointers in lambdas would be undefined behavior. Explicitly delete both copy and move to match the previous implicit behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 3a70843a..860c9f1d 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -32,6 +32,11 @@ namespace Babylon AppRuntime(Options options); ~AppRuntime(); + AppRuntime(const AppRuntime&) = delete; + AppRuntime& operator=(const AppRuntime&) = delete; + AppRuntime(AppRuntime&&) = delete; + AppRuntime& operator=(AppRuntime&&) = delete; + void Suspend(); void Resume(); From fa9b9f879a91f10c0794ffefc3e4e3492fd217d8 Mon Sep 17 00:00:00 2001 From: Gary Hsu Date: Tue, 14 Apr 2026 08:42:50 -0700 Subject: [PATCH 3/3] Add comments to copy/move deletion matching codebase style Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index 860c9f1d..f6ca7dfd 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -32,8 +32,11 @@ namespace Babylon AppRuntime(Options options); ~AppRuntime(); + // Copy semantics AppRuntime(const AppRuntime&) = delete; AppRuntime& operator=(const AppRuntime&) = delete; + + // Move semantics AppRuntime(AppRuntime&&) = delete; AppRuntime& operator=(AppRuntime&&) = delete;