From 053ac98353144ed412f97af1f67fcf5a30f80664 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 | 6 ++-- 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(+), 2 deletions(-) diff --git a/Core/AppRuntime/Source/AppRuntime_V8.cpp b/Core/AppRuntime/Source/AppRuntime_V8.cpp index 89928dcf..713312ab 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -113,9 +113,11 @@ namespace Babylon isolate->Dispose(); } - void AppRuntime::DrainMicrotasks(Napi::Env) + void AppRuntime::DrainMicrotasks(Napi::Env env) { // V8 auto-drains microtasks at the end of each script/callback when - // using the default MicrotasksPolicy. No explicit pump needed. + // using the default MicrotasksPolicy. 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 0af5caa8..0946623d 100644 --- a/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt +++ b/Tests/UnitTests/Android/app/src/main/cpp/CMakeLists.txt @@ -23,6 +23,9 @@ add_library(UnitTestsJNI SHARED target_compile_definitions(UnitTestsJNI PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIMEHOST_PLATFORM}") 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 2dbc7619..2de2122d 100644 --- a/Tests/UnitTests/CMakeLists.txt +++ b/Tests/UnitTests/CMakeLists.txt @@ -51,6 +51,9 @@ target_compile_definitions(UnitTests PRIVATE JSRUNTIMEHOST_PLATFORM="${JSRUNTIME 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 a920fa1f..9a9844e5 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) @@ -279,6 +283,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 3744e0465fd2ba9fa0b3610fb076db11713d1d1f 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 | 12 ++-- Core/Node-API/Source/env_v8.cc | 16 ++++- Tests/UnitTests/Shared/Shared.cpp | 61 ++++++++++++++++++- 10 files changed, 110 insertions(+), 8 deletions(-) diff --git a/Core/AppRuntime/Include/Babylon/AppRuntime.h b/Core/AppRuntime/Include/Babylon/AppRuntime.h index e001fa32..a7371e6b 100644 --- a/Core/AppRuntime/Include/Babylon/AppRuntime.h +++ b/Core/AppRuntime/Include/Babylon/AppRuntime.h @@ -76,6 +76,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); + Options m_options; class Impl; diff --git a/Core/AppRuntime/Source/AppRuntime.cpp b/Core/AppRuntime/Source/AppRuntime.cpp index 176bc849..9a8b49a5 100644 --- a/Core/AppRuntime/Source/AppRuntime.cpp +++ b/Core/AppRuntime/Source/AppRuntime.cpp @@ -84,7 +84,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); + } } // The dispatcher can be non-empty if something is dispatched after cancellation. diff --git a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp index 315954c2..bf36a3b7 100644 --- a/Core/AppRuntime/Source/AppRuntime_Chakra.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Chakra.cpp @@ -78,4 +78,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 12debfcb..02b5b941 100644 --- a/Core/AppRuntime/Source/AppRuntime_Hermes.cpp +++ b/Core/AppRuntime/Source/AppRuntime_Hermes.cpp @@ -25,4 +25,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 a7c809c7..f35fa314 100644 --- a/Core/AppRuntime/Source/AppRuntime_JSI.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JSI.cpp @@ -50,4 +50,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 b1334c22..1d8f1c25 100644 --- a/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp +++ b/Core/AppRuntime/Source/AppRuntime_JavaScriptCore.cpp @@ -28,4 +28,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 a3bf75da..a7ffb621 100644 --- a/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp +++ b/Core/AppRuntime/Source/AppRuntime_QuickJS.cpp @@ -64,4 +64,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 713312ab..933ccd1c 100644 --- a/Core/AppRuntime/Source/AppRuntime_V8.cpp +++ b/Core/AppRuntime/Source/AppRuntime_V8.cpp @@ -113,11 +113,15 @@ namespace Babylon isolate->Dispose(); } - void AppRuntime::DrainMicrotasks(Napi::Env env) + void AppRuntime::DrainMicrotasks(Napi::Env) { - // V8 auto-drains microtasks at the end of each script/callback when - // using the default MicrotasksPolicy. N-API finalizers deferred by a - // V8 garbage collection still require a safe host callback boundary. + // V8 auto-drains microtasks at the end of each script/callback. + } + + 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 9a9844e5..34e7bf26 100644 --- a/Tests/UnitTests/Shared/Shared.cpp +++ b/Tests/UnitTests/Shared/Shared.cpp @@ -288,10 +288,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( @@ -301,16 +303,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 /