From c712fb7cd6222018cf615fd0071998bde6f16da9 Mon Sep 17 00:00:00 2001 From: Marcel Laverdet Date: Tue, 12 Nov 2019 14:26:08 -0800 Subject: [PATCH] src: add abstract `IsolatePlatformDelegate` Adds a new abstract class for module authors and embedders to register arbitrary isolates with `node::MultiIsolatePlatform`. PR-URL: https://github.com/nodejs/node/pull/30324 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis Reviewed-By: Joyee Cheung --- src/node.h | 12 ++++++ src/node_platform.cc | 66 +++++++++++++++++++++++--------- src/node_platform.h | 13 +++++-- test/cctest/node_test_fixture.cc | 10 ++--- test/cctest/node_test_fixture.h | 16 ++++++-- test/cctest/test_platform.cc | 47 +++++++++++++++++++++++ 6 files changed, 134 insertions(+), 30 deletions(-) diff --git a/src/node.h b/src/node.h index c80e6266857921..f3da4574019254 100644 --- a/src/node.h +++ b/src/node.h @@ -262,6 +262,12 @@ class NODE_EXTERN ArrayBufferAllocator : public v8::ArrayBuffer::Allocator { NODE_EXTERN ArrayBufferAllocator* CreateArrayBufferAllocator(); NODE_EXTERN void FreeArrayBufferAllocator(ArrayBufferAllocator* allocator); +class NODE_EXTERN IsolatePlatformDelegate { + public: + virtual std::shared_ptr GetForegroundTaskRunner() = 0; + virtual bool IdleTasksEnabled() = 0; +}; + class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { public: ~MultiIsolatePlatform() override = default; @@ -283,6 +289,12 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { // This function may only be called once per `Isolate`. virtual void RegisterIsolate(v8::Isolate* isolate, struct uv_loop_s* loop) = 0; + // This method can be used when an application handles task scheduling on its + // own through `IsolatePlatformDelegate`. Upon registering an isolate with + // this overload any other method in this class with the exception of + // `UnregisterIsolate` *must not* be used on that isolate. + virtual void RegisterIsolate(v8::Isolate* isolate, + IsolatePlatformDelegate* delegate) = 0; // This function may only be called once per `Isolate`, and discard any // pending delayed tasks scheduled for that isolate. diff --git a/src/node_platform.cc b/src/node_platform.cc index 6e3b4abfee7287..383f67ce0f3bad 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -230,6 +230,11 @@ PerIsolatePlatformData::PerIsolatePlatformData( uv_unref(reinterpret_cast(flush_tasks_)); } +std::shared_ptr +PerIsolatePlatformData::GetForegroundTaskRunner() { + return shared_from_this(); +} + void PerIsolatePlatformData::FlushTasks(uv_async_t* handle) { auto platform_data = static_cast(handle->data); platform_data->FlushForegroundTasksInternal(); @@ -267,7 +272,7 @@ void PerIsolatePlatformData::PostNonNestableDelayedTask( } PerIsolatePlatformData::~PerIsolatePlatformData() { - Shutdown(); + CHECK(!flush_tasks_); } void PerIsolatePlatformData::AddShutdownCallback(void (*callback)(void*), @@ -325,18 +330,32 @@ NodePlatform::NodePlatform(int thread_pool_size, void NodePlatform::RegisterIsolate(Isolate* isolate, uv_loop_t* loop) { Mutex::ScopedLock lock(per_isolate_mutex_); - std::shared_ptr existing = per_isolate_[isolate]; - CHECK(!existing); - per_isolate_[isolate] = - std::make_shared(isolate, loop); + auto delegate = std::make_shared(isolate, loop); + IsolatePlatformDelegate* ptr = delegate.get(); + auto insertion = per_isolate_.emplace( + isolate, + std::make_pair(ptr, std::move(delegate))); + CHECK(insertion.second); +} + +void NodePlatform::RegisterIsolate(Isolate* isolate, + IsolatePlatformDelegate* delegate) { + Mutex::ScopedLock lock(per_isolate_mutex_); + auto insertion = per_isolate_.emplace( + isolate, + std::make_pair(delegate, std::shared_ptr{})); + CHECK(insertion.second); } void NodePlatform::UnregisterIsolate(Isolate* isolate) { Mutex::ScopedLock lock(per_isolate_mutex_); - std::shared_ptr existing = per_isolate_[isolate]; - CHECK(existing); - existing->Shutdown(); - per_isolate_.erase(isolate); + auto existing_it = per_isolate_.find(isolate); + CHECK_NE(existing_it, per_isolate_.end()); + auto& existing = existing_it->second; + if (existing.second) { + existing.second->Shutdown(); + } + per_isolate_.erase(existing_it); } void NodePlatform::AddIsolateFinishedCallback(Isolate* isolate, @@ -344,11 +363,11 @@ void NodePlatform::AddIsolateFinishedCallback(Isolate* isolate, Mutex::ScopedLock lock(per_isolate_mutex_); auto it = per_isolate_.find(isolate); if (it == per_isolate_.end()) { - CHECK(it->second); cb(data); return; } - it->second->AddShutdownCallback(cb, data); + CHECK(it->second.second); + it->second.second->AddShutdownCallback(cb, data); } void NodePlatform::Shutdown() { @@ -394,7 +413,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { } void NodePlatform::DrainTasks(Isolate* isolate) { - std::shared_ptr per_isolate = ForIsolate(isolate); + std::shared_ptr per_isolate = ForNodeIsolate(isolate); do { // Worker tasks aren't associated with an Isolate. @@ -452,23 +471,32 @@ void NodePlatform::CallDelayedOnWorkerThread(std::unique_ptr task, } +IsolatePlatformDelegate* NodePlatform::ForIsolate(Isolate* isolate) { + Mutex::ScopedLock lock(per_isolate_mutex_); + auto data = per_isolate_[isolate]; + CHECK_NOT_NULL(data.first); + return data.first; +} + std::shared_ptr -NodePlatform::ForIsolate(Isolate* isolate) { +NodePlatform::ForNodeIsolate(Isolate* isolate) { Mutex::ScopedLock lock(per_isolate_mutex_); - std::shared_ptr data = per_isolate_[isolate]; - CHECK(data); - return data; + auto data = per_isolate_[isolate]; + CHECK(data.second); + return data.second; } bool NodePlatform::FlushForegroundTasks(Isolate* isolate) { - return ForIsolate(isolate)->FlushForegroundTasksInternal(); + return ForNodeIsolate(isolate)->FlushForegroundTasksInternal(); } -bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; } +bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { + return ForIsolate(isolate)->IdleTasksEnabled(); +} std::shared_ptr NodePlatform::GetForegroundTaskRunner(Isolate* isolate) { - return ForIsolate(isolate); + return ForIsolate(isolate)->GetForegroundTaskRunner(); } double NodePlatform::MonotonicallyIncreasingTime() { diff --git a/src/node_platform.h b/src/node_platform.h index 24f7b337bb8fd7..e47d72b3662f92 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -52,12 +52,14 @@ struct DelayedTask { // This acts as the foreground task runner for a given Isolate. class PerIsolatePlatformData : + public IsolatePlatformDelegate, public v8::TaskRunner, public std::enable_shared_from_this { public: PerIsolatePlatformData(v8::Isolate* isolate, uv_loop_t* loop); ~PerIsolatePlatformData() override; + std::shared_ptr GetForegroundTaskRunner() override; void PostTask(std::unique_ptr task) override; void PostIdleTask(std::unique_ptr task) override; void PostDelayedTask(std::unique_ptr task, @@ -162,6 +164,9 @@ class NodePlatform : public MultiIsolatePlatform { bool FlushForegroundTasks(v8::Isolate* isolate) override; void RegisterIsolate(v8::Isolate* isolate, uv_loop_t* loop) override; + void RegisterIsolate(v8::Isolate* isolate, + IsolatePlatformDelegate* delegate) override; + void UnregisterIsolate(v8::Isolate* isolate) override; void AddIsolateFinishedCallback(v8::Isolate* isolate, void (*callback)(void*), void* data) override; @@ -170,11 +175,13 @@ class NodePlatform : public MultiIsolatePlatform { v8::Isolate* isolate) override; private: - std::shared_ptr ForIsolate(v8::Isolate* isolate); + IsolatePlatformDelegate* ForIsolate(v8::Isolate* isolate); + std::shared_ptr ForNodeIsolate(v8::Isolate* isolate); Mutex per_isolate_mutex_; - std::unordered_map> per_isolate_; + using DelegatePair = std::pair< + IsolatePlatformDelegate*, std::shared_ptr>; + std::unordered_map per_isolate_; node::tracing::TracingController* tracing_controller_; std::shared_ptr worker_thread_task_runner_; diff --git a/test/cctest/node_test_fixture.cc b/test/cctest/node_test_fixture.cc index 1a50e58dd9e8fa..b6f4af6b9e70e0 100644 --- a/test/cctest/node_test_fixture.cc +++ b/test/cctest/node_test_fixture.cc @@ -1,7 +1,7 @@ #include "node_test_fixture.h" -ArrayBufferUniquePtr NodeTestFixture::allocator{nullptr, nullptr}; -uv_loop_t NodeTestFixture::current_loop; -NodePlatformUniquePtr NodeTestFixture::platform; -TracingAgentUniquePtr NodeTestFixture::tracing_agent; -bool NodeTestFixture::node_initialized = false; +ArrayBufferUniquePtr NodeZeroIsolateTestFixture::allocator{nullptr, nullptr}; +uv_loop_t NodeZeroIsolateTestFixture::current_loop; +NodePlatformUniquePtr NodeZeroIsolateTestFixture::platform; +TracingAgentUniquePtr NodeZeroIsolateTestFixture::tracing_agent; +bool NodeZeroIsolateTestFixture::node_initialized = false; diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index ac0701d0942666..c8cb9232978821 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -60,14 +60,13 @@ using ArrayBufferUniquePtr = std::unique_ptr; using NodePlatformUniquePtr = std::unique_ptr; -class NodeTestFixture : public ::testing::Test { +class NodeZeroIsolateTestFixture : public ::testing::Test { protected: static ArrayBufferUniquePtr allocator; static TracingAgentUniquePtr tracing_agent; static NodePlatformUniquePtr platform; static uv_loop_t current_loop; static bool node_initialized; - v8::Isolate* isolate_; static void SetUpTestCase() { if (!node_initialized) { @@ -99,8 +98,18 @@ class NodeTestFixture : public ::testing::Test { void SetUp() override { allocator = ArrayBufferUniquePtr(node::CreateArrayBufferAllocator(), &node::FreeArrayBufferAllocator); + } +}; + + +class NodeTestFixture : public NodeZeroIsolateTestFixture { + protected: + v8::Isolate* isolate_; + + void SetUp() override { + NodeZeroIsolateTestFixture::SetUp(); isolate_ = NewIsolate(allocator.get(), ¤t_loop); - CHECK_NE(isolate_, nullptr); + CHECK_NOT_NULL(isolate_); isolate_->Enter(); } @@ -110,6 +119,7 @@ class NodeTestFixture : public ::testing::Test { isolate_->Dispose(); platform->UnregisterIsolate(isolate_); isolate_ = nullptr; + NodeZeroIsolateTestFixture::TearDown(); } }; diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index 5420502124d6da..73057bb2d8c03a 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -57,3 +57,50 @@ TEST_F(PlatformTest, SkipNewTasksInFlushForegroundTasks) { EXPECT_EQ(3, run_count); EXPECT_FALSE(platform->FlushForegroundTasks(isolate_)); } + +// Tests the registration of an abstract `IsolatePlatformDelegate` instance as +// opposed to the more common `uv_loop_s*` version of `RegisterIsolate`. +TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { + // Allocate isolate + v8::Isolate::CreateParams create_params; + create_params.array_buffer_allocator = allocator.get(); + auto isolate = v8::Isolate::Allocate(); + CHECK_NOT_NULL(isolate); + + // Register *first*, then initialize + auto delegate = std::make_shared( + isolate, + ¤t_loop); + platform->RegisterIsolate(isolate, delegate.get()); + v8::Isolate::Initialize(isolate, create_params); + + // Try creating Context + IsolateData + Environment + { + v8::Isolate::Scope isolate_scope(isolate); + v8::HandleScope handle_scope(isolate); + + auto context = node::NewContext(isolate); + CHECK(!context.IsEmpty()); + v8::Context::Scope context_scope(context); + + std::unique_ptr + isolate_data{node::CreateIsolateData(isolate, + ¤t_loop, + platform.get()), + node::FreeIsolateData}; + CHECK(isolate_data); + + std::unique_ptr + environment{node::CreateEnvironment(isolate_data.get(), + context, + 0, nullptr, + 0, nullptr), + node::FreeEnvironment}; + CHECK(environment); + } + + // Graceful shutdown + delegate->Shutdown(); + isolate->Dispose(); + platform->UnregisterIsolate(isolate); +}