From 0af6de498d9872a9dcfeb7dee0c4ad5d6d45373a Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 29 Jul 2026 16:46:31 -0700 Subject: [PATCH 1/2] Drain deferred V8 N-API finalizers after dispatch V8 weak callbacks enqueue N-API finalizers for a safe second pass, but AppRuntime did not drain that queue. Run the queue after each host dispatch and cover the behavior with a deterministic V8-only unit test. --- Core/AppRuntime/Source/AppRuntime_V8.cpp | 5 ++- Core/Node-API/Include/Engine/V8/napi/env.h | 2 ++ Core/Node-API/Source/env_v8.cc | 13 +++++++ .../Android/app/src/main/cpp/CMakeLists.txt | 3 ++ Tests/UnitTests/CMakeLists.txt | 3 ++ Tests/UnitTests/Shared/Shared.cpp | 34 +++++++++++++++++++ 6 files changed, 59 insertions(+), 1 deletion(-) diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index 83da9550..dd0955d7 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -122,8 +122,11 @@ namespace Babylon Module::Instance().Platform().UnregisterHost(v8::Isolate::GetCurrent()); } - void AppRuntime::DrainMicrotasks(Napi::Env) + void AppRuntime::DrainMicrotasks(Napi::Env env) { // V8 auto-drains microtasks. Foreground tasks run on AppRuntime's dispatcher. + // N-API finalizers deferred by a V8 garbage collection still require a safe + // host callback boundary. + Napi::DrainFinalizers(env); } } diff --git a/Core/Node-API/Include/Engine/V8/napi/env.h b/Core/Node-API/Include/Engine/V8/napi/env.h index 22334ec5..65406498 100644 --- a/Core/Node-API/Include/Engine/V8/napi/env.h +++ b/Core/Node-API/Include/Engine/V8/napi/env.h @@ -52,6 +52,8 @@ namespace Napi void Detach(Napi::Env); + void DrainFinalizers(Napi::Env); + Napi::Value Eval(Napi::Env env, const char* source, const char* sourceUrl); v8::Local GetContext(Napi::Env); diff --git a/Core/Node-API/Source/env_v8.cc b/Core/Node-API/Source/env_v8.cc index 1fbd7d30..596456e5 100644 --- a/Core/Node-API/Source/env_v8.cc +++ b/Core/Node-API/Source/env_v8.cc @@ -16,6 +16,19 @@ namespace Napi env_ptr->DeleteMe(); } + void DrainFinalizers(Env env) + { + napi_env env_ptr{env}; + + // Finalizers can mutate this queue, so continue until it is empty. + while (!env_ptr->pending_finalizers.empty()) + { + auto* finalizer = *env_ptr->pending_finalizers.begin(); + env_ptr->pending_finalizers.erase(finalizer); + finalizer->Finalize(); + } + } + v8::Local GetContext(Env env) { napi_env env_ptr{env}; diff --git a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt index 2e31e0fb..941643e1 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -32,6 +32,9 @@ endif() target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") target_compile_definitions(UnitTestsJNI PRIVATE NAPI_JAVASCRIPT_ENGINE="${NAPI_JAVASCRIPT_ENGINE}") target_compile_definitions(UnitTestsJNI PRIVATE ARCANA_TEST_HOOKS) +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") + target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_V8) +endif() target_include_directories(UnitTestsJNI PRIVATE ${UNIT_TESTS_DIR}) diff --git a/Tests/UnitTests/CMakeLists.txt b/Tests/UnitTests/CMakeLists.txt index f3676d7d..0028a3e8 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -59,6 +59,9 @@ target_compile_definitions(UnitTests PRIVATE NAPI_JAVASCRIPT_ENGINE="${NAPI_JAVA if(NAPI_JAVASCRIPT_ENGINE STREQUAL "JSI") target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_JSI) endif() +if(NAPI_JAVASCRIPT_ENGINE STREQUAL "V8") + target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_NAPI_ENGINE_V8) +endif() target_link_libraries(UnitTests PRIVATE AppRuntime diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 1c7e9ff7..6d7b698f 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -22,6 +22,10 @@ #include #include +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) +#include +#endif + namespace { const char* EnumToString(Babylon::Polyfills::Console::LogLevel logLevel) @@ -334,6 +338,36 @@ TEST(AppRuntime, DestroyDoesNotDeadlock) testThread.join(); } +#if defined(JSRUNTIMEHOST_NAPI_ENGINE_V8) +TEST(AppRuntime, V8FinalizersDrainAfterDispatch) +{ + constexpr size_t ExternalCount{32}; + std::atomic finalized{}; + std::promise observed; + + Babylon::AppRuntime runtime{}; + runtime.Dispatch([&finalized](Napi::Env env) { + for (size_t index{}; index < ExternalCount; ++index) + { + Napi::External>::New( + env, + &finalized, + [](Napi::Env, std::atomic* count) { + count->fetch_add(1, std::memory_order_relaxed); + }); + } + }); + runtime.Dispatch([](Napi::Env env) { + Napi::GetContext(env)->GetIsolate()->LowMemoryNotification(); + }); + runtime.Dispatch([&finalized, &observed](Napi::Env) { + observed.set_value(finalized.load(std::memory_order_relaxed)); + }); + + EXPECT_EQ(observed.get_future().get(), ExternalCount); +} +#endif + // The V8JSI Node-API shim does not implement napi_create_dataview / // napi_get_dataview_info (its DataView::New throws "TODO"), so this native test // only builds on the Chakra, V8, and JavaScriptCore backends. The size_t-width From be360c6578f2b6fe0337b9991e267ece44d55e6b Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 29 Jul 2026 22:19:58 -0700 Subject: [PATCH 2/2] Bound deferred V8 finalizer work per dispatcher turn Run deferred V8 finalizers once after each dispatcher batch and cap each batch at eight milliseconds. This prevents large GC queues from monopolizing the runtime thread while preserving eventual finalization on later turns. Add a deterministic slow-finalizer regression test that proves the queue yields and then drains completely. --- Core/AppRuntime/Include/Babylon/AppRuntime.h | 4 ++ Core/AppRuntime/Source/AppRuntime.cpp | 5 +- Core/AppRuntime/Source/AppRuntime_Chakra.cpp | 4 ++ Core/AppRuntime/Source/AppRuntime_Hermes.cpp | 4 ++ Core/AppRuntime/Source/AppRuntime_JSI.cpp | 4 ++ .../Source/AppRuntime_JavaScriptCore.cpp | 4 ++ Core/AppRuntime/Source/AppRuntime_QuickJS.cpp | 4 ++ Core/AppRuntime/Source/AppRuntime_V8.cpp | 10 ++- Core/Node-API/Source/env_v8.cc | 16 ++++- Tests/UnitTests/Shared/Shared.cpp | 61 ++++++++++++++++++- 10 files changed, 109 insertions(+), 7 deletions(-) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index d6d024e5..48bc3552 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -85,6 +85,10 @@ namespace Babylon // queue explicitly (Napi::DrainJobs / JS_ExecutePendingJob). void DrainMicrotasks(Napi::Env env); + // Engine-specific maintenance that should run once after a complete + // dispatcher turn, rather than after each callback in that turn. + void DrainPostDispatchWork(Napi::Env env); + Internal::DelayedTaskScheduler& GetDelayedTaskScheduler(); Options m_options; diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index 78ebe2d9..ea141d65 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -90,7 +90,10 @@ namespace Babylon while (!m_impl->m_cancelSource.cancelled()) { - m_impl->m_dispatcher.blocking_tick(m_impl->m_cancelSource); + if (m_impl->m_dispatcher.blocking_tick(m_impl->m_cancelSource)) + { + DrainPostDispatchWork(env); + } } Napi::HandleScope scope{env}; diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 8fe33493..8589e0f5 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -82,4 +82,8 @@ namespace Babylon // JsSetPromiseContinuationCallback hook (see RunEnvironmentTier). // No explicit pump needed here. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Core/AppRuntime/Source/AppRuntime_Hermes.cpp b/Core/AppRuntime/Source/AppRuntime_Hermes.cpp index f9dc3d5a..c78755f4 100644 --- a/Core/AppRuntime/Source/AppRuntime_Hermes.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Hermes.cpp @@ -29,4 +29,8 @@ namespace Babylon // observes the same "between turns" semantics it gets on V8/Chakra. Napi::DrainJobs(env); } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Core/AppRuntime/Source/AppRuntime_JSI.cpp b/Core/AppRuntime/Source/AppRuntime_JSI.cpp index a81a3900..5c83053b 100644 --- a/Core/AppRuntime/Source/AppRuntime_JSI.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JSI.cpp @@ -54,4 +54,8 @@ namespace Babylon { // JSI/V8 backed JSI auto-drains microtasks per scope. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp index f2483f41..70672a6d 100644 --- a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -32,4 +32,8 @@ namespace Babylon { // JavaScriptCore drains microtasks automatically at script boundaries. } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp b/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp index 10505fea..36538b68 100644 --- a/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp +++ b/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp @@ -68,4 +68,8 @@ namespace Babylon { } } + + void AppRuntime::DrainPostDispatchWork(Napi::Env) + { + } } diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index dd0955d7..efe3f234 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -122,11 +122,15 @@ namespace Babylon Module::Instance().Platform().UnregisterHost(v8::Isolate::GetCurrent()); } - void AppRuntime::DrainMicrotasks(Napi::Env env) + void AppRuntime::DrainMicrotasks(Napi::Env) { // V8 auto-drains microtasks. Foreground tasks run on AppRuntime's dispatcher. - // N-API finalizers deferred by a V8 garbage collection still require a safe - // host callback boundary. + } + + void AppRuntime::DrainPostDispatchWork(Napi::Env env) + { + // N-API finalizers deferred by a V8 garbage collection require a safe + // host boundary, but should not monopolize a dispatcher turn. Napi::DrainFinalizers(env); } } diff --git a/Core/Node-API/Source/env_v8.cc b/Core/Node-API/Source/env_v8.cc index 596456e5..fd2575d9 100644 --- a/Core/Node-API/Source/env_v8.cc +++ b/Core/Node-API/Source/env_v8.cc @@ -2,8 +2,15 @@ #include #include "js_native_api_v8.h" +#include + namespace Napi { + namespace + { + constexpr auto FinalizerDrainBudget{std::chrono::milliseconds{8}}; + } + Env Attach(v8::Local isolate) { // second argument is module version @@ -19,13 +26,20 @@ namespace Napi void DrainFinalizers(Env env) { napi_env env_ptr{env}; + const auto start = std::chrono::steady_clock::now(); - // Finalizers can mutate this queue, so continue until it is empty. + // Finalizers can mutate this queue. Bound each dispatcher turn so a large + // batch cannot monopolize the runtime thread. while (!env_ptr->pending_finalizers.empty()) { auto* finalizer = *env_ptr->pending_finalizers.begin(); env_ptr->pending_finalizers.erase(finalizer); finalizer->Finalize(); + + if (std::chrono::steady_clock::now() - start >= FinalizerDrainBudget) + { + break; + } } } diff --git a/Tests/UnitTests/Shared/Shared.cpp b/Tests/UnitTests/Shared/Shared.cpp index 6d7b698f..0a226f96 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -343,10 +343,12 @@ TEST(AppRuntime, V8FinalizersDrainAfterDispatch) { constexpr size_t ExternalCount{32}; std::atomic finalized{}; + std::promise created; + std::promise collectionRequested; std::promise observed; Babylon::AppRuntime runtime{}; - runtime.Dispatch([&finalized](Napi::Env env) { + runtime.Dispatch([&finalized, &created](Napi::Env env) { for (size_t index{}; index < ExternalCount; ++index) { Napi::External>::New( @@ -356,16 +358,71 @@ TEST(AppRuntime, V8FinalizersDrainAfterDispatch) count->fetch_add(1, std::memory_order_relaxed); }); } + created.set_value(); }); - runtime.Dispatch([](Napi::Env env) { + created.get_future().wait(); + + runtime.Dispatch([&collectionRequested](Napi::Env env) { Napi::GetContext(env)->GetIsolate()->LowMemoryNotification(); + collectionRequested.set_value(); }); + collectionRequested.get_future().wait(); + runtime.Dispatch([&finalized, &observed](Napi::Env) { observed.set_value(finalized.load(std::memory_order_relaxed)); }); EXPECT_EQ(observed.get_future().get(), ExternalCount); } + +TEST(AppRuntime, V8FinalizerDrainYieldsBetweenDispatcherTurns) +{ + constexpr size_t ExternalCount{16}; + std::atomic finalized{}; + std::promise created; + std::promise collectionRequested; + + Babylon::AppRuntime runtime{}; + runtime.Dispatch([&](Napi::Env env) { + for (size_t index{}; index < ExternalCount; ++index) + { + Napi::External>::New( + env, + &finalized, + [](Napi::Env, std::atomic* count) { + std::this_thread::sleep_for(std::chrono::milliseconds{2}); + count->fetch_add(1, std::memory_order_relaxed); + }); + } + created.set_value(); + }); + created.get_future().wait(); + + runtime.Dispatch([&](Napi::Env env) { + Napi::GetContext(env)->GetIsolate()->LowMemoryNotification(); + collectionRequested.set_value(); + }); + collectionRequested.get_future().wait(); + + const auto observeFinalized = [&]() { + std::promise observed; + auto future = observed.get_future(); + runtime.Dispatch([&](Napi::Env) { + observed.set_value(finalized.load(std::memory_order_relaxed)); + }); + return future.get(); + }; + + auto observed = observeFinalized(); + EXPECT_GT(observed, 0u); + EXPECT_LT(observed, ExternalCount); + + for (size_t turn{}; turn < ExternalCount && observed < ExternalCount; ++turn) + { + observed = observeFinalized(); + } + EXPECT_EQ(observed, ExternalCount); +} #endif // The V8JSI Node-API shim does not implement napi_create_dataview /