From 66a7421bf71426ed2212cfaac3cb2dca1070c1de Mon Sep 17 00:00:00 2001 From: Ulan Degenbaev Date: Thu, 12 Apr 2018 22:01:11 +0200 Subject: [PATCH 1/5] 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. Fixes: https://github.com/nodejs/node/issues/19937 --- node.gyp | 1 + src/node_platform.cc | 18 ++++++++++-- src/node_platform.h | 2 ++ test/cctest/test_platform.cc | 54 ++++++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 test/cctest/test_platform.cc diff --git a/node.gyp b/node.gyp index e2e6842c4f38c6..26b8d5ff916f59 100644 --- a/node.gyp +++ b/node.gyp @@ -937,6 +937,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/node_platform.cc b/src/node_platform.cc index d96db086925c01..a2350b09adc604 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)); } @@ -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..af8c3205a99561 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(); @@ -66,6 +67,7 @@ class PerIsolatePlatformData : int unref(); // Returns true iff work was dispatched or executed. + // New tasks that are posted during flushing of the queue are not run. bool FlushForegroundTasksInternal(); void CancelPendingDelayedTasks(); diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc new file mode 100644 index 00000000000000..c918fe99c6977e --- /dev/null +++ b/test/cctest/test_platform.cc @@ -0,0 +1,54 @@ +#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(3, &run_count, isolate_, platform.get())); + platform->FlushForegroundTasks(isolate_); + EXPECT_EQ(1, run_count); + platform->FlushForegroundTasks(isolate_); + EXPECT_EQ(2, run_count); + platform->FlushForegroundTasks(isolate_); + EXPECT_EQ(3, run_count); +} From 4dbc4aa77fb33c1cb51f1580d3935328c128c799 Mon Sep 17 00:00:00 2001 From: Ulan Degenbaev Date: Fri, 13 Apr 2018 10:34:15 +0200 Subject: [PATCH 2/5] [squash] clarify the comment of flushing --- src/node_platform.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_platform.h b/src/node_platform.h index af8c3205a99561..d6bba03cd813c9 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -67,7 +67,8 @@ class PerIsolatePlatformData : int unref(); // Returns true iff work was dispatched or executed. - // New tasks that are posted during flushing of the queue are not run. + // New tasks that are posted during flushing of the queue are postponed until + // the next flushing. bool FlushForegroundTasksInternal(); void CancelPendingDelayedTasks(); From c4354012708843ff722425936c62747ef5d12b5b Mon Sep 17 00:00:00 2001 From: Ulan Degenbaev Date: Mon, 16 Apr 2018 15:26:34 +0200 Subject: [PATCH 3/5] [squash] fix the repost count in the new test. --- test/cctest/test_platform.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index c918fe99c6977e..11543c9ca7ab67 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -44,7 +44,7 @@ TEST_F(PlatformTest, SkipNewTasksInFlushForegroundTasks) { Env env {handle_scope, argv}; int run_count = 0; platform->CallOnForegroundThread( - isolate_, new RepostingTask(3, &run_count, isolate_, platform.get())); + isolate_, new RepostingTask(2, &run_count, isolate_, platform.get())); platform->FlushForegroundTasks(isolate_); EXPECT_EQ(1, run_count); platform->FlushForegroundTasks(isolate_); From 167ae0ee0a4cca7495ff4d3f2be9922d022b1bc6 Mon Sep 17 00:00:00 2001 From: Ulan Degenbaev Date: Fri, 20 Apr 2018 15:55:48 +0200 Subject: [PATCH 4/5] [squash] drain all tasks in inspector-agent. --- src/inspector_agent.cc | 2 +- src/node_platform.cc | 4 ++-- src/node_platform.h | 11 +++++++---- test/cctest/test_platform.cc | 7 ++++--- 4 files changed, 14 insertions(+), 10 deletions(-) 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 a2350b09adc604..b8f1344727cfd6 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -260,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) { diff --git a/src/node_platform.h b/src/node_platform.h index d6bba03cd813c9..271a808a5a886a 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -66,9 +66,9 @@ class PerIsolatePlatformData : void ref(); int unref(); - // Returns true iff work was dispatched or executed. - // New tasks that are posted during flushing of the queue are postponed until - // the next flushing. + // Returns true iff 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(); @@ -133,7 +133,10 @@ class NodePlatform : public MultiIsolatePlatform { double CurrentClockTimeMillis() override; v8::TracingController* GetTracingController() override; - void FlushForegroundTasks(v8::Isolate* isolate); + // Returns true iff 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 index 11543c9ca7ab67..876547480b7032 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -45,10 +45,11 @@ TEST_F(PlatformTest, SkipNewTasksInFlushForegroundTasks) { int run_count = 0; platform->CallOnForegroundThread( isolate_, new RepostingTask(2, &run_count, isolate_, platform.get())); - platform->FlushForegroundTasks(isolate_); + EXPECT_TRUE(platform->FlushForegroundTasks(isolate_)); EXPECT_EQ(1, run_count); - platform->FlushForegroundTasks(isolate_); + EXPECT_TRUE(platform->FlushForegroundTasks(isolate_)); EXPECT_EQ(2, run_count); - platform->FlushForegroundTasks(isolate_); + EXPECT_TRUE(platform->FlushForegroundTasks(isolate_)); EXPECT_EQ(3, run_count); + EXPECT_FALSE(platform->FlushForegroundTasks(isolate_)); } From f46763bffee2e42407b72f9fc50b5caf335ac908 Mon Sep 17 00:00:00 2001 From: Ulan Degenbaev Date: Fri, 20 Apr 2018 17:15:08 +0200 Subject: [PATCH 5/5] [squash] fix comment s/iff/if/. --- src/node_platform.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_platform.h b/src/node_platform.h index 271a808a5a886a..8f6ff89f491fe3 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -66,7 +66,7 @@ class PerIsolatePlatformData : void ref(); int unref(); - // Returns true iff work was dispatched or executed. New tasks that are + // 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(); @@ -133,7 +133,7 @@ class NodePlatform : public MultiIsolatePlatform { double CurrentClockTimeMillis() override; v8::TracingController* GetTracingController() override; - // Returns true iff work was dispatched or executed. New tasks that are + // 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);