src: limit foreground tasks draining loop

Foreground tasks that repost themselves can force the draining loop
to run indefinitely long without giving other tasks chance to run.
This limits the foreground task draining loop to run only the tasks
that were in the tasks queue at the beginning of the loop.
PR-URL: #19987Fixes: #19937
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Khaidi Chu <i@2333.moe>

Author: ulan

node.gyp

@@ -961,6 +961,7 @@
 'test/cctest/test_base64.cc',
 'test/cctest/test_node_postmortem_metadata.cc',
 'test/cctest/test_environment.cc',
+'test/cctest/test_platform.cc',
 'test/cctest/test_util.cc',
 'test/cctest/test_url.cc'
  ],

src/inspector_agent.cc

@@ -316,7 +316,7 @@ class NodeInspectorClient : public V8InspectorClient {
  terminated_ = false;
  running_nested_loop_ = true;
 while (!terminated_ && channel_->waitForFrontendMessage()) {
- platform_->FlushForegroundTasks(env_->isolate());
+while (platform_->FlushForegroundTasks(env_->isolate())) {}
  }
  terminated_ = false;
  running_nested_loop_ = false;

src/node_platform.cc

@@ -95,7 +95,7 @@ void PerIsolatePlatformData::PostDelayedTask(
 }
 
 PerIsolatePlatformData::~PerIsolatePlatformData() {
-FlushForegroundTasksInternal();
+while (FlushForegroundTasksInternal()) {}
 CancelPendingDelayedTasks();
 
 uv_close(reinterpret_cast<uv_handle_t*>(flush_tasks_),
@@ -223,7 +223,13 @@ bool PerIsolatePlatformData::FlushForegroundTasksInternal() {
  });
  });
  }
-while (std::unique_ptr<Task> task = foreground_tasks_.Pop()) {
+// Move all foreground tasks into a separate queue and flush that queue.
+// This way tasks that are posted while flushing the queue will be run on the
+// next call of FlushForegroundTasksInternal.
+ std::queue<std::unique_ptr<Task>> tasks = foreground_tasks_.PopAll();
+while (!tasks.empty()) {
+ std::unique_ptr<Task> task = std::move(tasks.front());
+ tasks.pop();
  did_work = true;
 RunForegroundTask(std::move(task));
  }
@@ -254,8 +260,8 @@ void NodePlatform::CallDelayedOnForegroundThread(Isolate* isolate,
  std::unique_ptr<Task>(task), delay_in_seconds);
 }
 
-voidNodePlatform::FlushForegroundTasks(v8::Isolate* isolate) {
-ForIsolate(isolate)->FlushForegroundTasksInternal();
+boolNodePlatform::FlushForegroundTasks(v8::Isolate* isolate) {
+returnForIsolate(isolate)->FlushForegroundTasksInternal();
 }
 
 voidNodePlatform::CancelPendingDelayedTasks(v8::Isolate* isolate) {
@@ -348,4 +354,12 @@ void TaskQueue<T>::Stop() {
  tasks_available_.Broadcast(scoped_lock);
 }
 
+template <classT>
+std::queue<std::unique_ptr<T>> TaskQueue<T>::PopAll() {
+ Mutex::ScopedLock scoped_lock(lock_);
+ std::queue<std::unique_ptr<T>> result;
+ result.swap(task_queue_);
+return result;
+}
+
 } // namespace node

src/node_platform.h

@@ -26,6 +26,7 @@ class TaskQueue {
 voidPush(std::unique_ptr<T> task);
  std::unique_ptr<T> Pop();
  std::unique_ptr<T> BlockingPop();
+ std::queue<std::unique_ptr<T>> PopAll();
 voidNotifyOfCompletion();
 voidBlockingDrain();
 voidStop();
@@ -65,7 +66,9 @@ class PerIsolatePlatformData :
 voidref();
 intunref();
 
-// Returns true iff work was dispatched or executed.
+// Returns true if work was dispatched or executed. New tasks that are
+// posted during flushing of the queue are postponed until the next
+// flushing.
 boolFlushForegroundTasksInternal();
 voidCancelPendingDelayedTasks();
 
@@ -130,7 +133,10 @@ class NodePlatform : public MultiIsolatePlatform {
 doubleCurrentClockTimeMillis() override;
  v8::TracingController* GetTracingController() override;
 
-voidFlushForegroundTasks(v8::Isolate* isolate);
+// Returns true if work was dispatched or executed. New tasks that are
+// posted during flushing of the queue are postponed until the next
+// flushing.
+boolFlushForegroundTasks(v8::Isolate* isolate);
 
 voidRegisterIsolate(IsolateData* isolate_data, uv_loop_t* loop) override;
 voidUnregisterIsolate(IsolateData* isolate_data) override;

test/cctest/test_platform.cc

@@ -0,0 +1,55 @@
+#include"node_internals.h"
+#include"libplatform/libplatform.h"
+
+#include<string>
+#include"gtest/gtest.h"
+#include"node_test_fixture.h"
+
+// This task increments the given run counter and reposts itself until the
+// repost counter reaches zero.
+classRepostingTask : publicv8::Task {
+public:
+explicitRepostingTask(int repost_count,
+int* run_count,
+ v8::Isolate* isolate,
+ node::NodePlatform* platform)
+ : repost_count_(repost_count),
+ run_count_(run_count),
+ isolate_(isolate),
+ platform_(platform) {}
+
+// v8::Task implementation
+voidRun() final {
+ ++*run_count_;
+if (repost_count_ > 0) {
+ --repost_count_;
+ platform_->CallOnForegroundThread(isolate_,
+newRepostingTask(repost_count_, run_count_, isolate_, platform_));
+ }
+ }
+
+private:
+int repost_count_;
+int* run_count_;
+ v8::Isolate* isolate_;
+ node::NodePlatform* platform_;
+};
+
+classPlatformTest : publicEnvironmentTestFixture {};
+
+TEST_F(PlatformTest, SkipNewTasksInFlushForegroundTasks) {
+ v8::Isolate::Scope isolate_scope(isolate_);
+const v8::HandleScope handle_scope(isolate_);
+const Argv argv;
+ Env env {handle_scope, argv};
+int run_count = 0;
+ platform->CallOnForegroundThread(
+ isolate_, newRepostingTask(2, &run_count, isolate_, platform.get()));
+EXPECT_TRUE(platform->FlushForegroundTasks(isolate_));
+EXPECT_EQ(1, run_count);
+EXPECT_TRUE(platform->FlushForegroundTasks(isolate_));
+EXPECT_EQ(2, run_count);
+EXPECT_TRUE(platform->FlushForegroundTasks(isolate_));
+EXPECT_EQ(3, run_count);
+EXPECT_FALSE(platform->FlushForegroundTasks(isolate_));
+}