From 5f246d736b20980b55003352b5f0440ee21cce69 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 16 Sep 2026 23:01:52 -0700 Subject: [PATCH] Keep runtime schedulers safe through teardown Share synchronized dispatch state with scheduler copies, close it before AppRuntime discards pending work, and release dispatch captures outside the lock. Preserve existing callable overload behavior while accepting mutable and environment-taking callbacks. Add bounded native regression tests with owned timeout state. Adapted from integration commits by Matt Hargett: Source-commit: 7093c83d3a29020fc52f070859b7f7514ed3195f Source-commit: 0a5d570d156d75f5fc01162f5fe6a7ccc891e3c3 --- Core/AppRuntime/Source/AppRuntime.cpp | 8 +- Core/JsRuntime/Include/Babylon/JsRuntime.h | 12 +- .../Include/Babylon/JsRuntimeScheduler.h | 24 +- Core/JsRuntime/Source/JsRuntime.cpp | 48 ++- Tests/UnitTests/CMakeLists.txt | 1 + Tests/UnitTests/Shared/JsRuntimeScheduler.cpp | 305 ++++++++++++++++++ 6 files changed, 387 insertions(+), 11 deletions(-) create mode 100644 Tests/UnitTests/Shared/JsRuntimeScheduler.cpp diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index 78ebe2d9..77d9c4aa 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -34,6 +34,7 @@ namespace Babylon } std::optional m_env{}; + std::shared_ptr m_jsRuntimeState{}; std::optional> m_suspensionLock{}; arcana::cancellation_source m_cancelSource{}; arcana::manual_dispatcher<128> m_dispatcher{}; @@ -54,7 +55,7 @@ namespace Babylon m_impl->m_thread = std::thread{[this] { RunPlatformTier(); }}; Dispatch([this](Napi::Env env) { - JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); }); + m_impl->m_jsRuntimeState = JsRuntime::CreateForJavaScript(env, [this](auto func) { Dispatch(std::move(func)); }).m_state; Internal::DelayedTaskScheduler::SetForJavaScript(env, GetDelayedTaskScheduler()); m_impl->m_delayedTaskSchedulerRegistered = true; }); @@ -94,6 +95,11 @@ namespace Babylon } Napi::HandleScope scope{env}; + + // Stop native completions before discarding work, while captures can still + // safely release environment-owned values. Do not rely on JS finalizer order. + JsRuntime::Close(m_impl->m_jsRuntimeState); + ShutdownEnvironment(env); if (m_impl->m_delayedTaskSchedulerRegistered) diff --git a/Core/JsRuntime/Include/Babylon/JsRuntime.h b/Core/JsRuntime/Include/Babylon/JsRuntime.h index 75a7f4d9..da11b8df 100644 --- a/Core/JsRuntime/Include/Babylon/JsRuntime.h +++ b/Core/JsRuntime/Include/Babylon/JsRuntime.h @@ -4,7 +4,7 @@ #include #include -#include +#include namespace Babylon { @@ -45,9 +45,15 @@ namespace Babylon JsRuntime& operator=(const JsRuntime&) = delete; private: + friend class AppRuntime; + friend class JsRuntimeScheduler; + JsRuntime(Napi::Env, DispatchFunctionT); + ~JsRuntime(); + + static void Dispatch(const std::shared_ptr&, std::function); + static void Close(const std::shared_ptr&); - DispatchFunctionT m_dispatchFunction{}; - std::mutex m_mutex{}; + std::shared_ptr m_state; }; } diff --git a/Core/JsRuntime/Include/Babylon/JsRuntimeScheduler.h b/Core/JsRuntime/Include/Babylon/JsRuntimeScheduler.h index b123611d..6f5cf6da 100644 --- a/Core/JsRuntime/Include/Babylon/JsRuntimeScheduler.h +++ b/Core/JsRuntime/Include/Babylon/JsRuntimeScheduler.h @@ -2,29 +2,45 @@ #include "JsRuntime.h" +#include +#include + namespace Babylon { /** * Scheduler that invokes continuations via JsRuntime::Dispatch. * Intended to be consumed by arcana.cpp tasks. + * Copies can outlive the runtime; dispatch after shutdown is discarded. */ class JsRuntimeScheduler { public: explicit JsRuntimeScheduler(JsRuntime& runtime) - : m_runtime{runtime} + : m_runtimeState{runtime.m_state} { } template void operator()(CallableT&& callable) const { - m_runtime.Dispatch([callable{std::forward(callable)}](Napi::Env) { - callable(); + JsRuntime::Dispatch(m_runtimeState, [callable{std::forward(callable)}](Napi::Env env) mutable { + // Preserve the original const, zero-argument invocation when available. + if constexpr (std::is_invocable_v) + { + std::as_const(callable)(); + } + else if constexpr (std::is_invocable_v) + { + callable(); + } + else + { + callable(env); + } }); } private: - JsRuntime& m_runtime; + std::shared_ptr m_runtimeState; }; } diff --git a/Core/JsRuntime/Source/JsRuntime.cpp b/Core/JsRuntime/Source/JsRuntime.cpp index 27db82b0..8c7155bc 100644 --- a/Core/JsRuntime/Source/JsRuntime.cpp +++ b/Core/JsRuntime/Source/JsRuntime.cpp @@ -1,8 +1,22 @@ #include "JsRuntime.h" #include "Babylon/DebugTrace.h" +#include +#include + namespace Babylon { + struct JsRuntime::InternalState + { + explicit InternalState(DispatchFunctionT dispatchFunction) + : DispatchFunction{std::move(dispatchFunction)} + { + } + + DispatchFunctionT DispatchFunction; + std::mutex Mutex; + }; + namespace { static constexpr auto JS_RUNTIME_NAME = "runtime"; @@ -10,7 +24,7 @@ namespace Babylon } JsRuntime::JsRuntime(Napi::Env env, DispatchFunctionT dispatchFunction) - : m_dispatchFunction{std::move(dispatchFunction)} + : m_state{std::make_shared(std::move(dispatchFunction))} { auto global = env.Global(); @@ -28,6 +42,22 @@ namespace Babylon DEBUG_TRACE("JsRuntime created"); } + JsRuntime::~JsRuntime() + { + Close(m_state); + } + + void JsRuntime::Close(const std::shared_ptr& state) + { + DispatchFunctionT dispatchFunction; + if (state) + { + std::scoped_lock lock{state->Mutex}; + dispatchFunction = std::exchange(state->DispatchFunction, {}); + } + // Captured objects may dispatch from their destructors. Release them unlocked. + } + JsRuntime& BABYLON_API JsRuntime::CreateForJavaScript(Napi::Env env, DispatchFunctionT dispatchFunction) { auto* runtime = new JsRuntime(env, std::move(dispatchFunction)); @@ -45,8 +75,20 @@ namespace Babylon void JsRuntime::Dispatch(std::function function) { - std::scoped_lock lock{m_mutex}; - m_dispatchFunction([function = std::move(function)](Napi::Env env) { + Dispatch(m_state, std::move(function)); + } + + void JsRuntime::Dispatch(const std::shared_ptr& state, std::function function) + { + // Keep the host enqueue and Close mutually exclusive: copying the dispatch + // function out of the lock would allow it to run after the host is gone. + std::scoped_lock lock{state->Mutex}; + if (!state->DispatchFunction) + { + return; + } + + state->DispatchFunction([function = std::move(function)](Napi::Env env) { function(env); // The environment will be in a pending exceptional state if diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index f3676d7d..0133ed77 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -9,6 +9,7 @@ file(GLOB ASSETS "${CMAKE_CURRENT_SOURCE_DIR}/Assets/*") set(SOURCES "Shared/DelayedTaskScheduler.cpp" + "Shared/JsRuntimeScheduler.cpp" "Shared/StandardStreamLogger.cpp" "Shared/TimeoutDispatcher.cpp" "Shared/Shared.cpp" diff --git a/Tests/UnitTests/Shared/JsRuntimeScheduler.cpp b/Tests/UnitTests/Shared/JsRuntimeScheduler.cpp new file mode 100644 index 00000000..a17d39a2 --- /dev/null +++ b/Tests/UnitTests/Shared/JsRuntimeScheduler.cpp @@ -0,0 +1,305 @@ +#include +#include +#include "DelayedTaskScheduler.h" + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +using namespace std::chrono_literals; + +namespace +{ + template + T GetWithTimeout(std::future& future) + { + if (future.wait_for(5s) != std::future_status::ready) + { + throw std::runtime_error{"Runtime callback timed out"}; + } + return future.get(); + } + + // Include destruction in the deadline. A deadlocked worker may be detached, + // so every test and runtime callback owns its state, never a caller's stack. + template + void RunWithTimeout(CallbackT callback) + { + auto completed = std::make_shared>(); + auto future = completed->get_future(); + std::thread worker{[callback = std::move(callback), completed]() { + try + { + callback(); + completed->set_value(); + } + catch (...) + { + completed->set_exception(std::current_exception()); + } + }}; + + if (future.wait_for(15s) != std::future_status::ready) + { + worker.detach(); + FAIL() << "Runtime lifecycle did not finish within 15 seconds"; + } + worker.join(); + EXPECT_NO_THROW(future.get()); + } + + Babylon::JsRuntimeScheduler GetScheduler(Babylon::AppRuntime& runtime) + { + auto ready = std::make_shared>(); + auto future = ready->get_future(); + runtime.Dispatch([ready](Napi::Env env) { + ready->set_value(Babylon::JsRuntimeScheduler{Babylon::JsRuntime::GetFromJavaScript(env)}); + }); + return GetWithTimeout(future); + } + + struct OnDestruction + { + std::function Callback; + ~OnDestruction() + { + Callback(); + } + }; +} + +TEST(JsRuntimeScheduler, CopiesCanOutliveRuntime) +{ + RunWithTimeout([]() { + auto runtime = std::make_unique(); + const auto scheduler = GetScheduler(*runtime); + auto copy = scheduler; + auto moved = std::move(copy); + runtime.reset(); + + auto called = std::make_shared>(false); + scheduler([called]() { *called = true; }); + moved([called]() { *called = true; }); + if (called->load()) + { + throw std::runtime_error{"Callback ran after runtime destruction"}; + } + }); +} + +TEST(JsRuntimeScheduler, AcceptsMutableZeroArgumentLvalue) +{ + RunWithTimeout([]() { + Babylon::AppRuntime runtime; + const auto scheduler = GetScheduler(runtime); + auto completed = std::make_shared>(); + auto future = completed->get_future(); + auto callable = [completed, count = 0]() mutable { completed->set_value(++count); }; + scheduler(callable); + if (GetWithTimeout(future) != 1) + { + throw std::runtime_error{"Mutable callable was not invoked"}; + } + }); +} + +TEST(JsRuntimeScheduler, ForwardsEnvironmentAndAllowsNestedDispatch) +{ + RunWithTimeout([]() { + Babylon::AppRuntime runtime; + auto completed = std::make_shared>(); + auto future = completed->get_future(); + runtime.Dispatch([completed](Napi::Env env) { + const Babylon::JsRuntimeScheduler scheduler{Babylon::JsRuntime::GetFromJavaScript(env)}; + scheduler([completed, scheduler, expected = static_cast(env), count = 0](Napi::Env callbackEnv) mutable { + const bool correct = static_cast(callbackEnv) == expected && ++count == 1; + scheduler([completed, correct]() { completed->set_value(correct); }); + }); + }); + if (!GetWithTimeout(future)) + { + throw std::runtime_error{"Scheduler forwarded the wrong environment"}; + } + }); +} + +TEST(JsRuntimeScheduler, PrefersExistingZeroArgumentOverload) +{ + struct Callable + { + std::shared_ptr> Completed; + void operator()() const { Completed->set_value(true); } + void operator()(Napi::Env) const { Completed->set_value(false); } + }; + + RunWithTimeout([]() { + Babylon::AppRuntime runtime; + auto completed = std::make_shared>(); + auto future = completed->get_future(); + const Callable callable{completed}; + GetScheduler(runtime)(callable); + if (!GetWithTimeout(future)) + { + throw std::runtime_error{"Existing zero-argument overload was bypassed"}; + } + }); +} + +TEST(JsRuntimeScheduler, RunsArcanaContinuation) +{ + RunWithTimeout([]() { + Babylon::AppRuntime runtime; + auto completed = std::make_shared>(); + auto future = completed->get_future(); + auto scheduler = GetScheduler(runtime); + arcana::task_from_result().then( + scheduler, arcana::cancellation::none(), [completed]() { completed->set_value(); }); + GetWithTimeout(future); + }); +} + +TEST(JsRuntimeScheduler, PreservesConstZeroArgumentInvocation) +{ + struct Callable + { + std::shared_ptr> Completed; + void operator()() const { Completed->set_value(true); } + void operator()() { Completed->set_value(false); } + }; + + RunWithTimeout([]() { + Babylon::AppRuntime runtime; + auto completed = std::make_shared>(); + auto future = completed->get_future(); + GetScheduler(runtime)(Callable{completed}); + if (!GetWithTimeout(future)) + { + throw std::runtime_error{"Existing const invocation was bypassed"}; + } + }); +} + +TEST(JsRuntimeScheduler, DispatchCanRaceRuntimeTeardown) +{ + RunWithTimeout([]() { + auto runtime = std::make_unique(); + const auto scheduler = GetScheduler(*runtime); + auto started = std::make_shared>(); + auto future = started->get_future(); + auto called = std::make_shared>(0); + auto stop = std::make_shared>(false); + std::thread producer{[scheduler, started, called, stop]() { + const auto deadline = std::chrono::steady_clock::now() + 5s; + size_t attempts{}; + while (!stop->load() && std::chrono::steady_clock::now() < deadline) + { + scheduler([called]() { called->fetch_add(1); }); + if (++attempts == 100) + { + started->set_value(); + } + std::this_thread::yield(); + } + }}; + auto joinProducer = gsl::finally([&producer, stop]() { + *stop = true; + producer.join(); + }); + GetWithTimeout(future); + runtime.reset(); + + const auto countAfterTeardown = called->load(); + scheduler([called]() { called->fetch_add(1); }); + if (called->load() != countAfterTeardown) + { + throw std::runtime_error{"Callback ran after concurrent teardown"}; + } + }); +} + +TEST(JsRuntimeScheduler, ClosesBeforeDiscardingPendingWork) +{ + RunWithTimeout([]() { + auto runtime = std::make_unique(); + auto ready = std::make_shared>(); + auto future = ready->get_future(); + auto rejected = std::make_shared>(false); + runtime->Dispatch([ready, rejected](Napi::Env env) { + const Babylon::JsRuntimeScheduler scheduler{Babylon::JsRuntime::GetFromJavaScript(env)}; + auto probe = std::make_shared(); + probe->Callback = [scheduler, rejected]() { + auto token = std::make_shared(0); + std::weak_ptr weak = token; + scheduler([token = std::move(token)]() {}); + *rejected = weak.expired(); + }; + // Shutdown discards this capture before clearing the runtime queue. + Babylon::Internal::DelayedTaskScheduler::GetFromJavaScript(env)->Schedule(1h, [probe]() {}); + // Shutdown must not retrieve its state through a mutable JS property. + env.Global().Set("savedNative", env.Global().Get("_native")); + env.Global().Set("_native", env.Undefined()); + ready->set_value(); + }); + GetWithTimeout(future); + runtime.reset(); + if (!rejected->load()) + { + throw std::runtime_error{"Shutdown accepted work while discarding pending captures"}; + } + }); +} + +TEST(JsRuntimeScheduler, RejectedCaptureCanRedispatchFromDestructor) +{ + RunWithTimeout([]() { + auto runtime = std::make_unique(); + const auto scheduler = GetScheduler(*runtime); + runtime.reset(); + auto released = std::make_shared>(false); + auto probe = std::make_shared(); + probe->Callback = [scheduler, released]() { + scheduler([]() {}); + *released = true; + }; + scheduler([probe = std::move(probe)]() {}); + if (!released->load()) + { + throw std::runtime_error{"Rejected callback retained its captures"}; + } + }); +} + +TEST(JsRuntimeScheduler, FinalizerReleasesDispatchCapturesOutsideLock) +{ + RunWithTimeout([]() { + auto runtime = std::make_unique(); + auto ready = std::make_shared>(); + auto future = ready->get_future(); + auto released = std::make_shared>(false); + runtime->Dispatch([ready, released](Napi::Env env) { + auto probe = std::make_shared(); + // An independently created JsRuntime exercises finalizer-only closure, + // including breaking a dispatch capture that owns a scheduler copy. + auto& nativeRuntime = Babylon::JsRuntime::CreateForJavaScript(env, [probe](auto) {}); + probe->Callback = [scheduler = Babylon::JsRuntimeScheduler{nativeRuntime}, released]() { + scheduler([]() {}); + *released = true; + }; + ready->set_value(); + }); + GetWithTimeout(future); + runtime.reset(); + if (!released->load()) + { + throw std::runtime_error{"Finalizer retained the dispatch function"}; + } + }); +}