From 9590bcb02dd164ff5dacb1de1e06a786a55c50ab Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 23 Oct 2017 00:52:55 +0200 Subject: [PATCH] src: cancel pending delayed platform tasks on exit Worker threads need an event loop without active libuv handles in order to shut down. One source of handles that was previously not accounted for were delayed V8 tasks; these create timers that would be standing in the way of clearing the event loop. To solve this, keep track of the scheduled tasks in a list and close their timer handles before the corresponding isolate/loop is removed from the platform. It is not clear from the V8 documentation what the expectation is with respect to pending background tasks at the end of the isolate lifetime; however, an alternative approach of executing these scheduled tasks when flushing them led to an infinite loop of tasks scheduling each other; so it seems safe to assume that the behaviour implemented in this patch is at least acceptable. PR-URL: https://github.com/ayojs/ayo/pull/120 Reviewed-By: Stephen Belanger --- src/node.cc | 6 +++++ src/node.h | 1 + src/node_platform.cc | 54 ++++++++++++++++++++++++++++++++------------ src/node_platform.h | 16 ++++++++++++- src/node_worker.cc | 1 + 5 files changed, 62 insertions(+), 16 deletions(-) diff --git a/src/node.cc b/src/node.cc index 640fbdf8a8..5ab020b213 100644 --- a/src/node.cc +++ b/src/node.cc @@ -289,6 +289,10 @@ static struct { platform_->DrainBackgroundTasks(isolate); } + void CancelVMTasks(Isolate* isolate) { + platform_->CancelPendingDelayedTasks(isolate); + } + #if HAVE_INSPECTOR bool StartInspector(Environment *env, const char* script_path, const node::DebugOptions& options) { @@ -321,6 +325,7 @@ static struct { void Initialize(int thread_pool_size) {} void Dispose() {} void DrainVMTasks(Isolate* isolate) {} + void CancelVMTasks(Isolate* isolate) {} bool StartInspector(Environment *env, const char* script_path, const node::DebugOptions& options) { env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0"); @@ -4924,6 +4929,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, uv_key_delete(&thread_local_env); v8_platform.DrainVMTasks(isolate); + v8_platform.CancelVMTasks(isolate); WaitForInspectorDisconnect(&env); #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); diff --git a/src/node.h b/src/node.h index 7531c97a33..511dcb6242 100644 --- a/src/node.h +++ b/src/node.h @@ -214,6 +214,7 @@ class MultiIsolatePlatform : public v8::Platform { public: virtual ~MultiIsolatePlatform() { } virtual void DrainBackgroundTasks(v8::Isolate* isolate) = 0; + virtual void CancelPendingDelayedTasks(v8::Isolate* isolate) = 0; // These will be called by the `IsolateData` creation/destruction functions. virtual void RegisterIsolate(IsolateData* isolate_data, diff --git a/src/node_platform.cc b/src/node_platform.cc index ec2fca6c41..7cec43cbf4 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -4,6 +4,7 @@ #include "env.h" #include "env-inl.h" #include "util.h" +#include namespace node { @@ -45,13 +46,17 @@ void PerIsolatePlatformData::CallOnForegroundThread(Task* task) { void PerIsolatePlatformData::CallDelayedOnForegroundThread( Task* task, double delay_in_seconds) { - auto pair = new std::pair(task, delay_in_seconds); - foreground_delayed_tasks_.Push(pair); + auto delayed = new DelayedTask(); + delayed->task = task; + delayed->platform_data = this; + delayed->timeout = delay_in_seconds; + foreground_delayed_tasks_.Push(delayed); uv_async_send(flush_tasks_); } PerIsolatePlatformData::~PerIsolatePlatformData() { FlushForegroundTasksInternal(); + CancelPendingDelayedTasks(); uv_close(reinterpret_cast(flush_tasks_), [](uv_handle_t* handle) { @@ -120,7 +125,7 @@ size_t NodePlatform::NumberOfAvailableBackgroundThreads() { return threads_.size(); } -static void RunForegroundTask(Task* task) { +void PerIsolatePlatformData::RunForegroundTask(Task* task) { Isolate* isolate = Isolate::GetCurrent(); HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); @@ -130,14 +135,29 @@ static void RunForegroundTask(Task* task) { delete task; } -static void RunForegroundTask(uv_timer_t* handle) { - Task* task = static_cast(handle->data); - RunForegroundTask(task); - uv_close(reinterpret_cast(handle), [](uv_handle_t* handle) { - delete reinterpret_cast(handle); +void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { + DelayedTask* delayed = static_cast(handle->data); + auto& tasklist = delayed->platform_data->scheduled_delayed_tasks_; + auto it = std::find(tasklist.begin(), tasklist.end(), delayed); + CHECK_NE(it, tasklist.end()); + tasklist.erase(it); + RunForegroundTask(delayed->task); + uv_close(reinterpret_cast(&delayed->timer), + [](uv_handle_t* handle) { + delete static_cast(handle->data); }); } +void PerIsolatePlatformData::CancelPendingDelayedTasks() { + for (auto delayed : scheduled_delayed_tasks_) { + uv_close(reinterpret_cast(&delayed->timer), + [](uv_handle_t* handle) { + delete static_cast(handle->data); + }); + } + scheduled_delayed_tasks_.clear(); +} + void NodePlatform::DrainBackgroundTasks(Isolate* isolate) { PerIsolatePlatformData* per_isolate = ForIsolate(isolate); @@ -152,18 +172,18 @@ void NodePlatform::DrainBackgroundTasks(Isolate* isolate) { bool PerIsolatePlatformData::FlushForegroundTasksInternal() { bool did_work = false; + while (auto delayed = foreground_delayed_tasks_.Pop()) { did_work = true; uint64_t delay_millis = - static_cast(delayed->second + 0.5) * 1000; - uv_timer_t* handle = new uv_timer_t(); - handle->data = static_cast(delayed->first); - uv_timer_init(loop_, handle); + static_cast(delayed->timeout + 0.5) * 1000; + delayed->timer.data = static_cast(delayed); + uv_timer_init(loop_, &delayed->timer); // Timers may not guarantee queue ordering of events with the same delay if // the delay is non-zero. This should not be a problem in practice. - uv_timer_start(handle, RunForegroundTask, delay_millis, 0); - uv_unref(reinterpret_cast(handle)); - delete delayed; + uv_timer_start(&delayed->timer, RunForegroundTask, delay_millis, 0); + uv_unref(reinterpret_cast(&delayed->timer)); + scheduled_delayed_tasks_.push_back(delayed); } while (Task* task = foreground_tasks_.Pop()) { did_work = true; @@ -199,6 +219,10 @@ void NodePlatform::FlushForegroundTasks(v8::Isolate* isolate) { ForIsolate(isolate)->FlushForegroundTasksInternal(); } +void NodePlatform::CancelPendingDelayedTasks(v8::Isolate* isolate) { + ForIsolate(isolate)->CancelPendingDelayedTasks(); +} + bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; } double NodePlatform::MonotonicallyIncreasingTime() { diff --git a/src/node_platform.h b/src/node_platform.h index aa9bf327d7..73c2509e1a 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -14,6 +14,7 @@ namespace node { class NodePlatform; class IsolateData; +class PerIsolatePlatformData; template class TaskQueue { @@ -37,6 +38,13 @@ class TaskQueue { std::queue task_queue_; }; +struct DelayedTask { + v8::Task* task; + uv_timer_t timer; + double timeout; + PerIsolatePlatformData* platform_data; +}; + class PerIsolatePlatformData { public: PerIsolatePlatformData(v8::Isolate* isolate, uv_loop_t* loop); @@ -52,15 +60,20 @@ class PerIsolatePlatformData { // Returns true iff work was dispatched or executed. bool FlushForegroundTasksInternal(); + void CancelPendingDelayedTasks(); + private: static void FlushTasks(uv_async_t* handle); + static void RunForegroundTask(v8::Task* task); + static void RunForegroundTask(uv_timer_t* timer); int ref_count_ = 1; v8::Isolate* isolate_; uv_loop_t* const loop_; uv_async_t* flush_tasks_ = nullptr; TaskQueue foreground_tasks_; - TaskQueue> foreground_delayed_tasks_; + TaskQueue foreground_delayed_tasks_; + std::vector scheduled_delayed_tasks_; }; class NodePlatform : public MultiIsolatePlatform { @@ -69,6 +82,7 @@ class NodePlatform : public MultiIsolatePlatform { virtual ~NodePlatform() {} void DrainBackgroundTasks(v8::Isolate* isolate) override; + void CancelPendingDelayedTasks(v8::Isolate* isolate) override; void Shutdown(); // v8::Platform implementation. diff --git a/src/node_worker.cc b/src/node_worker.cc index 03c9f59aae..759e2a8d20 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -300,6 +300,7 @@ void Worker::Run() { Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); platform->DrainBackgroundTasks(isolate_); + platform->CancelPendingDelayedTasks(isolate_); // Grab the parent-to-child channel and render is unusable. MessagePort* child_port;