From d3edf2fcde0c37206dc339aab7333212d20aa0f1 Mon Sep 17 00:00:00 2001 From: Ulan Degenbaev Date: Thu, 12 Apr 2018 22:01:11 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/19987 Fixes: https://github.com/nodejs/node/issues/19937 Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Yang Guo Reviewed-By: Colin Ihrig Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Ben Noordhuis Reviewed-By: Anatoli Papirovski Reviewed-By: Khaidi Chu --- node.gyp | 1 + src/inspector_agent.cc | 2 +- src/node_platform.cc | 22 ++++++++++++--- src/node_platform.h | 10 +++++-- test/cctest/test_platform.cc | 55 ++++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 test/cctest/test_platform.cc diff --git a/node.gyp b/node.gyp index 8347beb18245ca..abed8ad4c48c10 100644 --- a/node.gyp +++ b/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' ], diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index e143d316d2e6fa..fffaecdef3ddfa 100644 --- a/src/inspector_agent.cc +++ b/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; diff --git a/src/node_platform.cc b/src/node_platform.cc index d96db086925c01..b8f1344727cfd6 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -95,7 +95,7 @@ void PerIsolatePlatformData::PostDelayedTask( } PerIsolatePlatformData::~PerIsolatePlatformData() { - FlushForegroundTasksInternal(); + while (FlushForegroundTasksInternal()) {} CancelPendingDelayedTasks(); uv_close(reinterpret_cast(flush_tasks_), @@ -223,7 +223,13 @@ bool PerIsolatePlatformData::FlushForegroundTasksInternal() { }); }); } - while (std::unique_ptr 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> tasks = foreground_tasks_.PopAll(); + while (!tasks.empty()) { + std::unique_ptr 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), delay_in_seconds); } -void NodePlatform::FlushForegroundTasks(v8::Isolate* isolate) { - ForIsolate(isolate)->FlushForegroundTasksInternal(); +bool NodePlatform::FlushForegroundTasks(v8::Isolate* isolate) { + return ForIsolate(isolate)->FlushForegroundTasksInternal(); } void NodePlatform::CancelPendingDelayedTasks(v8::Isolate* isolate) { @@ -348,4 +354,12 @@ void TaskQueue::Stop() { tasks_available_.Broadcast(scoped_lock); } +template +std::queue> TaskQueue::PopAll() { + Mutex::ScopedLock scoped_lock(lock_); + std::queue> result; + result.swap(task_queue_); + return result; +} + } // namespace node diff --git a/src/node_platform.h b/src/node_platform.h index b7546057871a1d..8f6ff89f491fe3 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -26,6 +26,7 @@ class TaskQueue { void Push(std::unique_ptr task); std::unique_ptr Pop(); std::unique_ptr BlockingPop(); + std::queue> PopAll(); void NotifyOfCompletion(); void BlockingDrain(); void Stop(); @@ -65,7 +66,9 @@ class PerIsolatePlatformData : void ref(); int unref(); - // 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. bool FlushForegroundTasksInternal(); void CancelPendingDelayedTasks(); @@ -130,7 +133,10 @@ class NodePlatform : public MultiIsolatePlatform { double CurrentClockTimeMillis() override; v8::TracingController* GetTracingController() override; - void FlushForegroundTasks(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. + bool FlushForegroundTasks(v8::Isolate* isolate); void RegisterIsolate(IsolateData* isolate_data, uv_loop_t* loop) override; void UnregisterIsolate(IsolateData* isolate_data) override; diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc new file mode 100644 index 00000000000000..876547480b7032 --- /dev/null +++ b/test/cctest/test_platform.cc @@ -0,0 +1,55 @@ +#include "node_internals.h" +#include "libplatform/libplatform.h" + +#include +#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. +class RepostingTask : public v8::Task { + public: + explicit RepostingTask(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 + void Run() final { + ++*run_count_; + if (repost_count_ > 0) { + --repost_count_; + platform_->CallOnForegroundThread(isolate_, + new RepostingTask(repost_count_, run_count_, isolate_, platform_)); + } + } + + private: + int repost_count_; + int* run_count_; + v8::Isolate* isolate_; + node::NodePlatform* platform_; +}; + +class PlatformTest : public EnvironmentTestFixture {}; + +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_, new RepostingTask(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_)); +}