Run one V8 fallback foreground task per event-loop checkpoint

include/unofficial_napi.h

@@ -141,20 +141,35 @@ NAPI_EXTENSION_WASMER_EXTERN napi_status unofficial_napi_set_prepare_stack_trace
     napi_value callback);
 
 typedef enum unofficial_napi_event_loop_checkpoint_mode {
-  // Drain promise/microtask work without admitting a host task turn.
+  // Drain promise/microtask work without suspending for a host task turn. A
+  // synchronous provider still reports a provider task it ran (see
+  // unofficial_napi_event_loop_checkpoint_state_host_tasks_admitted).
   unofficial_napi_event_loop_checkpoint_microtasks = 0,
   // Admit a host task turn as well as draining engine work. Host-JavaScript
   // providers suspend through JSPI. Synchronous providers report that no host
-  // task was admitted so the runtime can wait on its native event source.
+  // task was admitted, unless the checkpoint ran a provider task (see
+  // unofficial_napi_event_loop_checkpoint_state_host_tasks_admitted), so the
+  // runtime can wait on its native event source.
   unofficial_napi_event_loop_checkpoint_host_tasks = 1,
 } unofficial_napi_event_loop_checkpoint_mode;
 
 typedef enum unofficial_napi_event_loop_checkpoint_state {
   unofficial_napi_event_loop_checkpoint_state_none = 0,
   // The provider still owns work which can make JavaScript runnable.
   unofficial_napi_event_loop_checkpoint_state_pending_provider_work = 1 << 0,
-  // The checkpoint admitted a host task turn. When this bit is absent, the
-  // runtime remains responsible for waiting on its native event source.
+  // The checkpoint admitted a host task turn, so the runtime starts its next
+  // loop turn without waiting on its native event source.
+  //
+  // - Host-JavaScript providers set it after suspending through JSPI.
+  // - Synchronous providers set it, together with
+  //   unofficial_napi_event_loop_checkpoint_state_pending_provider_work, when
+  //   the checkpoint ran a provider task (for example a V8 foreground task),
+  //   in either mode. They run at most one such task per checkpoint, because
+  //   a task may queue its successor, so the event loop gets a turn between
+  //   them. A checkpoint that runs no task leaves the bit unset.
+  //
+  // When this bit is absent, the runtime remains responsible for waiting on
+  // its native event source.
   unofficial_napi_event_loop_checkpoint_state_host_tasks_admitted = 1 << 1,
 } unofficial_napi_event_loop_checkpoint_state;
 

tests/CMakeLists.txt

@@ -325,3 +325,20 @@ add_fixture_test(${_napi_test_prefix}_test_65_unofficial_contextify
 add_fixture_test(${_napi_test_prefix}_test_allocator
   src/napi_allocator_test.cc
 )
+
+if(_napi_test_engine STREQUAL "v8")
+  add_executable(napi_v8_foreground_checkpoint_test src/v8_foreground_checkpoint_test.cc)
+  target_include_directories(napi_v8_foreground_checkpoint_test PRIVATE
+    "${NAPI_INCLUDE_ROOT}"
+    "${${_napi_engine_include_var}}"
+  )
+  target_link_libraries(napi_v8_foreground_checkpoint_test PRIVATE
+    ${_napi_test_engine_target}
+    "${${_napi_engine_library_var}}"
+  )
+  if(NAPI_V8_DEFINES)
+    target_compile_definitions(napi_v8_foreground_checkpoint_test PRIVATE ${NAPI_V8_DEFINES})
+  endif()
+  add_test(NAME napi_v8.foreground_checkpoint_fairness
+    COMMAND napi_v8_foreground_checkpoint_test)
+endif()

tests/src/v8_foreground_checkpoint_test.cc

@@ -0,0 +1,66 @@
+#include "../../v8/src/edge_v8_platform.h"
+
+#include <cstdio>
+#include <memory>
+#include <utility>
+
+#include <v8.h>
+
+namespace {
+class RepostingForegroundTask final : public v8::Task {
+ public:
+  RepostingForegroundTask(std::shared_ptr<v8::TaskRunner> runner, int* ran, int remaining)
+      : runner_(std::move(runner)), ran_(ran), remaining_(remaining) {}
+
+  void Run() override {
+    ++*ran_;
+    if (remaining_ > 1) {
+      runner_->PostTask(std::make_unique<RepostingForegroundTask>(runner_, ran_, remaining_ - 1));
+    }
+  }
+
+ private:
+  std::shared_ptr<v8::TaskRunner> runner_;
+  int* ran_;
+  int remaining_;
+};
+}  // namespace
+
+// Own V8 initialization and disposal in a separate process so this platform
+// cannot interfere with the environments of other N-API test fixtures.
+int main() {
+  EdgeV8Platform::SetWorkerThreadCount(1);
+  auto platform = EdgeV8Platform::Create();
+  if (!platform) return 1;
+  v8::V8::InitializePlatform(platform.get());
+  if (!v8::V8::Initialize()) return 1;
+  auto allocator = std::unique_ptr<v8::ArrayBuffer::Allocator>(
+      v8::ArrayBuffer::Allocator::NewDefaultAllocator());
+  v8::Isolate::CreateParams params;
+  params.array_buffer_allocator = allocator.get();
+  v8::Isolate* isolate = v8::Isolate::New(params);
+  if (!isolate) return 1;
+  bool passed = true;
+  {
+    v8::Isolate::Scope isolate_scope(isolate);
+    v8::HandleScope handle_scope(isolate);
+    auto runner = platform->GetForegroundTaskRunner(isolate, v8::TaskPriority::kUserVisible);
+    // Clear bootstrap tasks before introducing the deterministic task chain.
+    for (int turn = 0; turn < 32; ++turn) platform->PumpPendingForegroundTasks(isolate);
+    int ran = 0;
+    runner->PostTask(std::make_unique<RepostingForegroundTask>(runner, &ran, 4));
+    for (int turn = 1; turn <= 4; ++turn) {
+      platform->PumpPendingForegroundTasks(isolate);
+      if (ran != turn) {
+        std::fprintf(stderr, "foreground turn %d ran %d tasks\n", turn, ran);
+        passed = false;
+      }
+    }
+    platform->PumpPendingForegroundTasks(isolate);
+    passed = passed && ran == 4;
+  }
+  platform->DisposeIsolate(isolate);
+  v8::V8::Dispose();
+  v8::V8::DisposePlatform();
+  return passed ? 0 : 1;
+}

v8/src/edge_v8_platform.cc

@@ -510,18 +510,19 @@ void EdgeV8Platform::ClearForegroundTaskTarget(v8::Isolate* isolate, napi_env en
   state->runner->ClearTarget(env);
 }
 
-void EdgeV8Platform::PumpPendingForegroundTasks(v8::Isolate* isolate) {
-  if (fallback_ == nullptr || isolate == nullptr) return;
+bool EdgeV8Platform::PumpPendingForegroundTasks(v8::Isolate* isolate) {
+  if (fallback_ == nullptr || isolate == nullptr) return false;
   // ForegroundTaskRunner::PostTaskCommon forwards to the guest's bound
   // enqueue callback when one is set, but falls back to fallback_'s own
   // GetForegroundTaskRunner() when no guest target is bound (e.g. the guest
   // drives everything through the provider event-loop checkpoint and never
   // calls BindForegroundTaskTarget). Nothing else pumps that fallback
   // runner's queue, so tasks routed there -- including V8-internal work like
   // Heap::PostFinalizationRegistryCleanupTaskIfNeeded's cleanup task -- would
-  // otherwise be posted and never run. Drain it explicitly.
-  while (v8::platform::PumpMessageLoop(fallback_.get(), isolate)) {
-  }
+  // otherwise be posted and never run. Admit one task per checkpoint so
+  // self-rescheduling GC work cannot monopolize the guest's event-loop turn.
+  // The caller reports progress to keep the loop runnable for the next task.
+  return v8::platform::PumpMessageLoop(fallback_.get(), isolate);
 }
 
 int EdgeV8Platform::NumberOfWorkerThreads() {

v8/src/edge_v8_platform.h

@@ -43,7 +43,9 @@ class EdgeV8Platform final : public v8::Platform {
   void ClearForegroundTaskTarget(v8::Isolate* isolate, napi_env env);
   void AddPendingForegroundTask(const std::shared_ptr<IsolateState>& state);
   void CompletePendingForegroundTask(const std::shared_ptr<IsolateState>& state);
-  void PumpPendingForegroundTasks(v8::Isolate* isolate);
+  // Run at most one ready fallback task. Returns true when a task ran so
+  // the embedder can admit another loop turn before continuing provider work.
+  bool PumpPendingForegroundTasks(v8::Isolate* isolate);
 
   int NumberOfWorkerThreads() override;
   std::shared_ptr<v8::TaskRunner> GetForegroundTaskRunner(

v8/src/unofficial_napi.cc

@@ -2121,34 +2121,34 @@ napi_status NAPI_CDECL unofficial_napi_set_prepare_stack_trace_callback(
 // FinalizationRegistry callbacks never fire. Pump it here, at the same point
 // microtasks are already checkpointed, so this deferred work always gets a
 // chance to run regardless of whether the guest wired up its own hook.
-void PumpPlatformForegroundTasks(napi_env env) {
-  if (env == nullptr || env->isolate == nullptr) return;
+bool PumpPlatformForegroundTasks(napi_env env) {
+  if (env == nullptr || env->isolate == nullptr) return false;
   EdgeV8Platform* platform = nullptr;
   {
     std::lock_guard<std::mutex> lock(g_runtime_mu);
     platform = g_runtime.platform.get();
   }
   if (platform != nullptr) {
-    platform->PumpPendingForegroundTasks(env->isolate);
+    return platform->PumpPendingForegroundTasks(env->isolate);
   }
+  return false;
 }
 
-void DrainMicrotasksForEnv(napi_env env) {
-  if (env == nullptr || env->isolate == nullptr) return;
+bool DrainMicrotasksForEnv(napi_env env) {
+  if (env == nullptr || env->isolate == nullptr) return false;
   env->DrainFinalizerQueue();
   v8::Local<v8::Context> context = env->context();
   if (!context.IsEmpty()) {
     v8::MicrotaskQueue* queue = context->GetMicrotaskQueue();
     if (queue != nullptr) {
       queue->PerformCheckpoint(env->isolate);
       env->DrainFinalizerQueue();
-      PumpPlatformForegroundTasks(env);
-      return;
+      return PumpPlatformForegroundTasks(env);
     }
   }
   env->isolate->PerformMicrotaskCheckpoint();
   env->DrainFinalizerQueue();
-  PumpPlatformForegroundTasks(env);
+  return PumpPlatformForegroundTasks(env);
 }
 
 napi_status NAPI_CDECL unofficial_napi_event_loop_checkpoint(
@@ -2162,11 +2162,17 @@ napi_status NAPI_CDECL unofficial_napi_event_loop_checkpoint(
       mode != unofficial_napi_event_loop_checkpoint_host_tasks) {
     return napi_invalid_arg;
   }
-  DrainMicrotasksForEnv(env);
+  const bool host_task_admitted = DrainMicrotasksForEnv(env);
   if (state_out != nullptr) {
     *state_out = NapiV8HasPendingProviderWork(env)
                      ? unofficial_napi_event_loop_checkpoint_state_pending_provider_work
                      : unofficial_napi_event_loop_checkpoint_state_none;
+    if (host_task_admitted) {
+      // A task may have posted its successor. Conservatively request another
+      // turn; an empty pump clears this signal without inspecting delayed work.
+      *state_out |= unofficial_napi_event_loop_checkpoint_state_host_tasks_admitted |
+                    unofficial_napi_event_loop_checkpoint_state_pending_provider_work;
+    }
   }
   return napi_ok;
 }