From e65b6aedb702a78e623bc550284547cbab688ce4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 14 Sep 2017 13:05:48 +0200 Subject: [PATCH 1/2] src: prepare v8 platform for multi-isolate support This splits the task queue used for asynchronous tasks scheduled by V8 in per-isolate queues, so that multiple threads can be supported. PR-URL: https://github.com/ayojs/ayo/pull/89 Reviewed-By: Timothy Gu --- src/env-inl.h | 42 ++--------- src/env.cc | 54 ++++++++++++++ src/env.h | 10 ++- src/inspector_agent.cc | 2 +- src/node.cc | 41 ++++++++--- src/node.h | 24 ++++++- src/node_platform.cc | 120 +++++++++++++++++++++++++------- src/node_platform.h | 51 +++++++++++--- test/cctest/node_test_fixture.h | 4 +- test/cctest/test_environment.cc | 14 ++-- 10 files changed, 269 insertions(+), 93 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 83b87fadacc6fd..5c28b2334bd661 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -37,41 +37,9 @@ namespace node { -inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop, - uint32_t* zero_fill_field) : - -// Create string and private symbol properties as internalized one byte strings. -// -// Internalized because it makes property lookups a little faster and because -// the string is created in the old space straight away. It's going to end up -// in the old space sooner or later anyway but now it doesn't go through -// v8::Eternal's new space handling first. -// -// One byte because our strings are ASCII and we can safely skip V8's UTF-8 -// decoding step. It's a one-time cost, but why pay it when you don't have to? -#define V(PropertyName, StringValue) \ - PropertyName ## _( \ - isolate, \ - v8::Private::New( \ - isolate, \ - v8::String::NewFromOneByte( \ - isolate, \ - reinterpret_cast(StringValue), \ - v8::NewStringType::kInternalized, \ - sizeof(StringValue) - 1).ToLocalChecked())), - PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) -#undef V -#define V(PropertyName, StringValue) \ - PropertyName ## _( \ - isolate, \ - v8::String::NewFromOneByte( \ - isolate, \ - reinterpret_cast(StringValue), \ - v8::NewStringType::kInternalized, \ - sizeof(StringValue) - 1).ToLocalChecked()), - PER_ISOLATE_STRING_PROPERTIES(V) -#undef V - event_loop_(event_loop), zero_fill_field_(zero_fill_field) {} +inline v8::Isolate* IsolateData::isolate() const { + return isolate_; +} inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; @@ -81,6 +49,10 @@ inline uint32_t* IsolateData::zero_fill_field() const { return zero_fill_field_; } +inline MultiIsolatePlatform* IsolateData::platform() const { + return platform_; +} + inline Environment::AsyncHooks::AsyncHooks(v8::Isolate* isolate) : isolate_(isolate), fields_(isolate, kFieldsCount), diff --git a/src/env.cc b/src/env.cc index 9bed08cdb09175..bc58d7f08f1cbb 100644 --- a/src/env.cc +++ b/src/env.cc @@ -2,6 +2,8 @@ #include "async-wrap.h" #include "v8-profiler.h" #include "node_buffer.h" +#include "req-wrap-inl.h" +#include "node_platform.h" #if defined(_MSC_VER) #define getpid GetCurrentProcessId @@ -17,10 +19,62 @@ namespace node { using v8::Context; using v8::FunctionTemplate; using v8::HandleScope; +using v8::Isolate; using v8::Local; using v8::Message; +using v8::Private; using v8::StackFrame; using v8::StackTrace; +using v8::String; + +IsolateData::IsolateData(Isolate* isolate, + uv_loop_t* event_loop, + MultiIsolatePlatform* platform, + uint32_t* zero_fill_field) : + +// Create string and private symbol properties as internalized one byte strings. +// +// Internalized because it makes property lookups a little faster and because +// the string is created in the old space straight away. It's going to end up +// in the old space sooner or later anyway but now it doesn't go through +// v8::Eternal's new space handling first. +// +// One byte because our strings are ASCII and we can safely skip V8's UTF-8 +// decoding step. It's a one-time cost, but why pay it when you don't have to? +#define V(PropertyName, StringValue) \ + PropertyName ## _( \ + isolate, \ + Private::New( \ + isolate, \ + String::NewFromOneByte( \ + isolate, \ + reinterpret_cast(StringValue), \ + v8::NewStringType::kInternalized, \ + sizeof(StringValue) - 1).ToLocalChecked())), + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) +#undef V +#define V(PropertyName, StringValue) \ + PropertyName ## _( \ + isolate, \ + String::NewFromOneByte( \ + isolate, \ + reinterpret_cast(StringValue), \ + v8::NewStringType::kInternalized, \ + sizeof(StringValue) - 1).ToLocalChecked()), + PER_ISOLATE_STRING_PROPERTIES(V) +#undef V + isolate_(isolate), + event_loop_(event_loop), + zero_fill_field_(zero_fill_field), + platform_(platform) { + if (platform_ != nullptr) + platform_->RegisterIsolate(this, event_loop); +} + +IsolateData::~IsolateData() { + if (platform_ != nullptr) + platform_->UnregisterIsolate(this); +} void Environment::Start(int argc, const char* const* argv, diff --git a/src/env.h b/src/env.h index 787cc7eb19d064..2947af74ed7ecd 100644 --- a/src/env.h +++ b/src/env.h @@ -341,10 +341,13 @@ struct node_async_ids { class IsolateData { public: - inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop, - uint32_t* zero_fill_field = nullptr); + IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop, + MultiIsolatePlatform* platform = nullptr, + uint32_t* zero_fill_field = nullptr); + ~IsolateData(); inline uv_loop_t* event_loop() const; inline uint32_t* zero_fill_field() const; + inline MultiIsolatePlatform* platform() const; #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) @@ -357,6 +360,7 @@ class IsolateData { #undef VP std::unordered_map> http2_static_strs; + inline v8::Isolate* isolate() const; private: #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) @@ -369,8 +373,10 @@ class IsolateData { #undef VS #undef VP + v8::Isolate* const isolate_; uv_loop_t* const event_loop_; uint32_t* const zero_fill_field_; + MultiIsolatePlatform* platform_; DISALLOW_COPY_AND_ASSIGN(IsolateData); }; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index efc0ffc15c5b85..d5b3b43ec22979 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -309,7 +309,7 @@ class NodeInspectorClient : public V8InspectorClient { terminated_ = false; running_nested_loop_ = true; while (!terminated_ && channel_->waitForFrontendMessage()) { - platform_->FlushForegroundTasksInternal(); + platform_->FlushForegroundTasks(env_->isolate()); } terminated_ = false; running_nested_loop_ = false; diff --git a/src/node.cc b/src/node.cc index d304ca24c895cd..a9ae4cc3959d05 100644 --- a/src/node.cc +++ b/src/node.cc @@ -262,10 +262,10 @@ node::DebugOptions debug_options; static struct { #if NODE_USE_V8_PLATFORM - void Initialize(int thread_pool_size, uv_loop_t* loop) { + void Initialize(int thread_pool_size) { tracing_agent_ = trace_enabled ? new tracing::Agent() : nullptr; - platform_ = new NodePlatform(thread_pool_size, loop, + platform_ = new NodePlatform(thread_pool_size, trace_enabled ? tracing_agent_->GetTracingController() : nullptr); V8::InitializePlatform(platform_); tracing::TraceEventHelper::SetTracingController( @@ -280,8 +280,8 @@ static struct { tracing_agent_ = nullptr; } - void DrainVMTasks() { - platform_->DrainBackgroundTasks(); + void DrainVMTasks(Isolate* isolate) { + platform_->DrainBackgroundTasks(isolate); } #if HAVE_INSPECTOR @@ -306,12 +306,16 @@ static struct { tracing_agent_->Stop(); } + NodePlatform* Platform() { + return platform_; + } + tracing::Agent* tracing_agent_; NodePlatform* platform_; #else // !NODE_USE_V8_PLATFORM - void Initialize(int thread_pool_size, uv_loop_t* loop) {} + void Initialize(int thread_pool_size) {} void Dispose() {} - void DrainVMTasks() {} + void DrainVMTasks(Isolate* isolate) {} bool StartInspector(Environment *env, const char* script_path, const node::DebugOptions& options) { env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0"); @@ -323,6 +327,10 @@ static struct { "so event tracing is not available.\n"); } void StopTracingAgent() {} + + NodePlatform* Platform() { + return nullptr; + } #endif // !NODE_USE_V8_PLATFORM #if !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR @@ -4431,7 +4439,14 @@ int EmitExit(Environment* env) { IsolateData* CreateIsolateData(Isolate* isolate, uv_loop_t* loop) { - return new IsolateData(isolate, loop); + return new IsolateData(isolate, loop, nullptr); +} + +IsolateData* CreateIsolateData( + Isolate* isolate, + uv_loop_t* loop, + MultiIsolatePlatform* platform) { + return new IsolateData(isolate, loop, platform); } @@ -4516,7 +4531,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, do { uv_run(env.event_loop(), UV_RUN_DEFAULT); - v8_platform.DrainVMTasks(); + v8_platform.DrainVMTasks(isolate); more = uv_loop_alive(env.event_loop()); if (more) @@ -4537,7 +4552,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, RunAtExit(&env); uv_key_delete(&thread_local_env); - v8_platform.DrainVMTasks(); + v8_platform.DrainVMTasks(isolate); WaitForInspectorDisconnect(&env); #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); @@ -4580,7 +4595,11 @@ inline int Start(uv_loop_t* event_loop, Locker locker(isolate); Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); - IsolateData isolate_data(isolate, event_loop, allocator.zero_fill_field()); + IsolateData isolate_data( + isolate, + event_loop, + v8_platform.Platform(), + allocator.zero_fill_field()); exit_code = Start(isolate, &isolate_data, argc, argv, exec_argc, exec_argv); } @@ -4627,7 +4646,7 @@ int Start(int argc, char** argv) { V8::SetEntropySource(crypto::EntropySource); #endif // HAVE_OPENSSL - v8_platform.Initialize(v8_thread_pool_size, uv_default_loop()); + v8_platform.Initialize(v8_thread_pool_size); // Enable tracing when argv has --trace-events-enabled. if (trace_enabled) { fprintf(stderr, "Warning: Trace event is an experimental feature " diff --git a/src/node.h b/src/node.h index aa8738a567c63c..0656428c0b1f6b 100644 --- a/src/node.h +++ b/src/node.h @@ -61,6 +61,7 @@ #endif #include "v8.h" // NOLINT(build/include_order) +#include "v8-platform.h" // NOLINT(build/include_order) #include "node_version.h" // NODE_MODULE_VERSION #define NODE_MAKE_VERSION(major, minor, patch) \ @@ -209,8 +210,27 @@ NODE_EXTERN void Init(int* argc, class IsolateData; class Environment; -NODE_EXTERN IsolateData* CreateIsolateData(v8::Isolate* isolate, - struct uv_loop_s* loop); +class MultiIsolatePlatform : public v8::Platform { + public: + virtual ~MultiIsolatePlatform() { } + virtual void DrainBackgroundTasks(v8::Isolate* isolate) = 0; + + // These will be called by the `IsolateData` creation/destruction functions. + virtual void RegisterIsolate(IsolateData* isolate_data, + struct uv_loop_s* loop) = 0; + virtual void UnregisterIsolate(IsolateData* isolate_data) = 0; +}; + +// If `platform` is passed, it will be used to register new Worker instances. +// It can be `nullptr`, in which case creating new Workers inside of +// Environments that use this `IsolateData` will not work. +NODE_EXTERN IsolateData* CreateIsolateData( + v8::Isolate* isolate, + struct uv_loop_s* loop); +NODE_EXTERN IsolateData* CreateIsolateData( + v8::Isolate* isolate, + struct uv_loop_s* loop, + MultiIsolatePlatform* platform); NODE_EXTERN void FreeIsolateData(IsolateData* isolate_data); NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data, diff --git a/src/node_platform.cc b/src/node_platform.cc index 56b13b8437a0d2..ec2fca6c414d45 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -1,6 +1,8 @@ #include "node_platform.h" #include "node_internals.h" +#include "env.h" +#include "env-inl.h" #include "util.h" namespace node { @@ -13,11 +15,6 @@ using v8::Platform; using v8::Task; using v8::TracingController; -static void FlushTasks(uv_async_t* handle) { - NodePlatform* platform = static_cast(handle->data); - platform->FlushForegroundTasksInternal(); -} - static void BackgroundRunner(void* data) { TaskQueue* background_tasks = static_cast*>(data); while (Task* task = background_tasks->BlockingPop()) { @@ -27,12 +24,51 @@ static void BackgroundRunner(void* data) { } } -NodePlatform::NodePlatform(int thread_pool_size, uv_loop_t* loop, - TracingController* tracing_controller) - : loop_(loop) { - CHECK_EQ(0, uv_async_init(loop, &flush_tasks_, FlushTasks)); - flush_tasks_.data = static_cast(this); - uv_unref(reinterpret_cast(&flush_tasks_)); +PerIsolatePlatformData::PerIsolatePlatformData( + v8::Isolate* isolate, uv_loop_t* loop) + : isolate_(isolate), loop_(loop) { + flush_tasks_ = new uv_async_t(); + CHECK_EQ(0, uv_async_init(loop, flush_tasks_, FlushTasks)); + flush_tasks_->data = static_cast(this); + uv_unref(reinterpret_cast(flush_tasks_)); +} + +void PerIsolatePlatformData::FlushTasks(uv_async_t* handle) { + auto platform_data = static_cast(handle->data); + platform_data->FlushForegroundTasksInternal(); +} + +void PerIsolatePlatformData::CallOnForegroundThread(Task* task) { + foreground_tasks_.Push(task); + uv_async_send(flush_tasks_); +} + +void PerIsolatePlatformData::CallDelayedOnForegroundThread( + Task* task, double delay_in_seconds) { + auto pair = new std::pair(task, delay_in_seconds); + foreground_delayed_tasks_.Push(pair); + uv_async_send(flush_tasks_); +} + +PerIsolatePlatformData::~PerIsolatePlatformData() { + FlushForegroundTasksInternal(); + + uv_close(reinterpret_cast(flush_tasks_), + [](uv_handle_t* handle) { + delete reinterpret_cast(handle); + }); +} + +void PerIsolatePlatformData::ref() { + ref_count_++; +} + +int PerIsolatePlatformData::unref() { + return --ref_count_; +} + +NodePlatform::NodePlatform(int thread_pool_size, + TracingController* tracing_controller) { if (tracing_controller) { tracing_controller_.reset(tracing_controller); } else { @@ -49,18 +85,35 @@ NodePlatform::NodePlatform(int thread_pool_size, uv_loop_t* loop, } } +void NodePlatform::RegisterIsolate(IsolateData* isolate_data, uv_loop_t* loop) { + Isolate* isolate = isolate_data->isolate(); + Mutex::ScopedLock lock(per_isolate_mutex_); + PerIsolatePlatformData* existing = per_isolate_[isolate]; + if (existing != nullptr) + existing->ref(); + else + per_isolate_[isolate] = new PerIsolatePlatformData(isolate, loop); +} + +void NodePlatform::UnregisterIsolate(IsolateData* isolate_data) { + Isolate* isolate = isolate_data->isolate(); + Mutex::ScopedLock lock(per_isolate_mutex_); + PerIsolatePlatformData* existing = per_isolate_[isolate]; + CHECK_NE(existing, nullptr); + if (existing->unref() == 0) { + delete existing; + per_isolate_.erase(isolate); + } +} + void NodePlatform::Shutdown() { background_tasks_.Stop(); for (size_t i = 0; i < threads_.size(); i++) { CHECK_EQ(0, uv_thread_join(threads_[i].get())); } - // uv_run cannot be called from the time before the beforeExit callback - // runs until the program exits unless the event loop has any referenced - // handles after beforeExit terminates. This prevents unrefed timers - // that happen to terminate during shutdown from being run unsafely. - // Since uv_run cannot be called, this handle will never be fully cleaned - // up. - uv_close(reinterpret_cast(&flush_tasks_), nullptr); + Mutex::ScopedLock lock(per_isolate_mutex_); + for (const auto& pair : per_isolate_) + delete pair.second; } size_t NodePlatform::NumberOfAvailableBackgroundThreads() { @@ -85,13 +138,19 @@ static void RunForegroundTask(uv_timer_t* handle) { }); } -void NodePlatform::DrainBackgroundTasks() { +void NodePlatform::DrainBackgroundTasks(Isolate* isolate) { + PerIsolatePlatformData* per_isolate = ForIsolate(isolate); + do { + // Right now, there is no way to drain only background tasks associated with + // a specific isolate, so this sometimes does more work than necessary. + // In the long run, that functionality is probably going to be available + // anyway, though. background_tasks_.BlockingDrain(); - } while (FlushForegroundTasksInternal()); + } while (per_isolate->FlushForegroundTasksInternal()); } -bool NodePlatform::FlushForegroundTasksInternal() { +bool PerIsolatePlatformData::FlushForegroundTasksInternal() { bool did_work = false; while (auto delayed = foreground_delayed_tasks_.Pop()) { did_work = true; @@ -118,17 +177,26 @@ void NodePlatform::CallOnBackgroundThread(Task* task, background_tasks_.Push(task); } +PerIsolatePlatformData* NodePlatform::ForIsolate(Isolate* isolate) { + Mutex::ScopedLock lock(per_isolate_mutex_); + PerIsolatePlatformData* data = per_isolate_[isolate]; + CHECK_NE(data, nullptr); + return data; +} + void NodePlatform::CallOnForegroundThread(Isolate* isolate, Task* task) { - foreground_tasks_.Push(task); - uv_async_send(&flush_tasks_); + ForIsolate(isolate)->CallOnForegroundThread(task); } void NodePlatform::CallDelayedOnForegroundThread(Isolate* isolate, Task* task, double delay_in_seconds) { - auto pair = new std::pair(task, delay_in_seconds); - foreground_delayed_tasks_.Push(pair); - uv_async_send(&flush_tasks_); + ForIsolate(isolate)->CallDelayedOnForegroundThread(task, + delay_in_seconds); +} + +void NodePlatform::FlushForegroundTasks(v8::Isolate* isolate) { + ForIsolate(isolate)->FlushForegroundTasksInternal(); } bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; } diff --git a/src/node_platform.h b/src/node_platform.h index 04927fccc3df66..aa9bf327d7471f 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -2,14 +2,19 @@ #define SRC_NODE_PLATFORM_H_ #include +#include #include #include "libplatform/libplatform.h" +#include "node.h" #include "node_mutex.h" #include "uv.h" namespace node { +class NodePlatform; +class IsolateData; + template class TaskQueue { public: @@ -32,15 +37,38 @@ class TaskQueue { std::queue task_queue_; }; -class NodePlatform : public v8::Platform { +class PerIsolatePlatformData { public: - NodePlatform(int thread_pool_size, uv_loop_t* loop, - v8::TracingController* tracing_controller); - virtual ~NodePlatform() {} + PerIsolatePlatformData(v8::Isolate* isolate, uv_loop_t* loop); + ~PerIsolatePlatformData(); + + void CallOnForegroundThread(v8::Task* task); + void CallDelayedOnForegroundThread(v8::Task* task, double delay_in_seconds); + + void Shutdown(); + + void ref(); + int unref(); - void DrainBackgroundTasks(); // Returns true iff work was dispatched or executed. bool FlushForegroundTasksInternal(); + private: + static void FlushTasks(uv_async_t* handle); + + 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_; +}; + +class NodePlatform : public MultiIsolatePlatform { + public: + NodePlatform(int thread_pool_size, v8::TracingController* tracing_controller); + virtual ~NodePlatform() {} + + void DrainBackgroundTasks(v8::Isolate* isolate) override; void Shutdown(); // v8::Platform implementation. @@ -54,11 +82,16 @@ class NodePlatform : public v8::Platform { double MonotonicallyIncreasingTime() override; v8::TracingController* GetTracingController() override; + void FlushForegroundTasks(v8::Isolate* isolate); + + void RegisterIsolate(IsolateData* isolate_data, uv_loop_t* loop) override; + void UnregisterIsolate(IsolateData* isolate_data) override; + private: - uv_loop_t* const loop_; - uv_async_t flush_tasks_; - TaskQueue foreground_tasks_; - TaskQueue> foreground_delayed_tasks_; + PerIsolatePlatformData* ForIsolate(v8::Isolate* isolate); + + Mutex per_isolate_mutex_; + std::unordered_map per_isolate_; TaskQueue background_tasks_; std::vector> threads_; diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 263f7b96f9daec..890fe9049994e9 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -73,6 +73,8 @@ class NodeTestFixture : public ::testing::Test { public: static uv_loop_t* CurrentLoop() { return ¤t_loop; } + node::MultiIsolatePlatform* Platform() const { return platform_; } + protected: v8::Isolate::CreateParams params_; ArrayBufferAllocator allocator_; @@ -84,7 +86,7 @@ class NodeTestFixture : public ::testing::Test { virtual void SetUp() { CHECK_EQ(0, uv_loop_init(¤t_loop)); - platform_ = new node::NodePlatform(8, ¤t_loop, nullptr); + platform_ = new node::NodePlatform(8, nullptr); v8::V8::InitializePlatform(platform_); v8::V8::Initialize(); params_.array_buffer_allocator = &allocator_; diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 8beacfa95ece7e..704efd7a88358f 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -26,11 +26,13 @@ class EnvironmentTest : public NodeTestFixture { public: Env(const v8::HandleScope& handle_scope, v8::Isolate* isolate, - const Argv& argv) { + const Argv& argv, + NodeTestFixture* test_fixture) { context_ = v8::Context::New(isolate); CHECK(!context_.IsEmpty()); isolate_data_ = CreateIsolateData(isolate, - NodeTestFixture::CurrentLoop()); + NodeTestFixture::CurrentLoop(), + test_fixture->Platform()); CHECK_NE(nullptr, isolate_data_); environment_ = CreateEnvironment(isolate_data_, context_, @@ -66,7 +68,7 @@ class EnvironmentTest : public NodeTestFixture { TEST_F(EnvironmentTest, AtExitWithEnvironment) { const v8::HandleScope handle_scope(isolate_); const Argv argv; - Env env {handle_scope, isolate_, argv}; + Env env {handle_scope, isolate_, argv, this}; AtExit(*env, at_exit_callback1); RunAtExit(*env); @@ -76,7 +78,7 @@ TEST_F(EnvironmentTest, AtExitWithEnvironment) { TEST_F(EnvironmentTest, AtExitWithArgument) { const v8::HandleScope handle_scope(isolate_); const Argv argv; - Env env {handle_scope, isolate_, argv}; + Env env {handle_scope, isolate_, argv, this}; std::string arg{"some args"}; AtExit(*env, at_exit_callback1, static_cast(&arg)); @@ -87,8 +89,8 @@ TEST_F(EnvironmentTest, AtExitWithArgument) { TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { const v8::HandleScope handle_scope(isolate_); const Argv argv; - Env env1 {handle_scope, isolate_, argv}; - Env env2 {handle_scope, isolate_, argv}; + Env env1 {handle_scope, isolate_, argv, this}; + Env env2 {handle_scope, isolate_, argv, this}; AtExit(*env1, at_exit_callback1); AtExit(*env2, at_exit_callback2); From ca75a4cd0196e9e0152b1b9ed8a51275a48b59c0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 23 Oct 2017 00:52:55 +0200 Subject: [PATCH 2/2] 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/env.cc | 1 - src/node.cc | 6 +++++ src/node.h | 1 + src/node_platform.cc | 54 ++++++++++++++++++++++++++++++++------------ src/node_platform.h | 16 ++++++++++++- 5 files changed, 61 insertions(+), 17 deletions(-) diff --git a/src/env.cc b/src/env.cc index bc58d7f08f1cbb..86ea8aacbd4e9d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -2,7 +2,6 @@ #include "async-wrap.h" #include "v8-profiler.h" #include "node_buffer.h" -#include "req-wrap-inl.h" #include "node_platform.h" #if defined(_MSC_VER) diff --git a/src/node.cc b/src/node.cc index a9ae4cc3959d05..aab9d9fd8fe339 100644 --- a/src/node.cc +++ b/src/node.cc @@ -284,6 +284,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) { @@ -316,6 +320,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"); @@ -4553,6 +4558,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 0656428c0b1f6b..3cf5692f6b2975 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 ec2fca6c414d45..7cec43cbf44509 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 aa9bf327d7471f..73c2509e1a0052 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.