From ebdf84a8bd2637ef29cc0193981c76c870724d07 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Nov 2019 19:47:03 +0000 Subject: [PATCH 01/37] src: make `FreeEnvironment()` perform all necessary cleanup Make the calls `stop_sub_worker_contexts()`, `RunCleanup()` part of the public API for easier embedding. (Note that calling `RunAtExit()` is idempotent because the at-exit callback queue is cleared after each call.) PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 18 +++++++++++++++++- src/node_main_instance.cc | 18 ++++++------------ src/node_main_instance.h | 3 ++- src/node_platform.cc | 5 ++++- src/node_worker.cc | 26 +++++++++----------------- 5 files changed, 38 insertions(+), 32 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 97d25cba579570..d4fa856b8d59ea 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -342,7 +342,23 @@ Environment* CreateEnvironment(IsolateData* isolate_data, } void FreeEnvironment(Environment* env) { - env->RunCleanup(); + { + // TODO(addaleax): This should maybe rather be in a SealHandleScope. + HandleScope handle_scope(env->isolate()); + Context::Scope context_scope(env->context()); + env->set_stopping(true); + env->stop_sub_worker_contexts(); + env->RunCleanup(); + RunAtExit(env); + } + + // This call needs to be made while the `Environment` is still alive + // because we assume that it is available for async tracking in the + // NodePlatform implementation. + MultiIsolatePlatform* platform = env->isolate_data()->platform(); + if (platform != nullptr) + platform->DrainTasks(env->isolate()); + delete env; } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 91bb30cb4e3ee6..c6533321bd6b64 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -110,7 +110,8 @@ int NodeMainInstance::Run() { HandleScope handle_scope(isolate_); int exit_code = 0; - std::unique_ptr env = CreateMainEnvironment(&exit_code); + DeleteFnPtr env = + CreateMainEnvironment(&exit_code); CHECK_NOT_NULL(env); Context::Scope context_scope(env->context()); @@ -149,10 +150,7 @@ int NodeMainInstance::Run() { exit_code = EmitExit(env.get()); } - env->set_can_call_into_js(false); - env->stop_sub_worker_contexts(); ResetStdio(); - env->RunCleanup(); // TODO(addaleax): Neither NODE_SHARED_MODE nor HAVE_INSPECTOR really // make sense here. @@ -167,10 +165,6 @@ int NodeMainInstance::Run() { } #endif - RunAtExit(env.get()); - - per_process::v8_platform.DrainVMTasks(isolate_); - #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); #endif @@ -180,8 +174,8 @@ int NodeMainInstance::Run() { // TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h // and the environment creation routine in workers somehow. -std::unique_ptr NodeMainInstance::CreateMainEnvironment( - int* exit_code) { +DeleteFnPtr +NodeMainInstance::CreateMainEnvironment(int* exit_code) { *exit_code = 0; // Reset the exit code to 0 HandleScope handle_scope(isolate_); @@ -205,14 +199,14 @@ std::unique_ptr NodeMainInstance::CreateMainEnvironment( CHECK(!context.IsEmpty()); Context::Scope context_scope(context); - std::unique_ptr env = std::make_unique( + DeleteFnPtr env { new Environment( isolate_data_.get(), context, args_, exec_args_, static_cast(Environment::kIsMainThread | Environment::kOwnsProcessState | - Environment::kOwnsInspector)); + Environment::kOwnsInspector)) }; env->InitializeLibuv(per_process::v8_is_profiling); env->InitializeDiagnostics(); diff --git a/src/node_main_instance.h b/src/node_main_instance.h index 5bc18cb3de63c0..cc9f50b9222de3 100644 --- a/src/node_main_instance.h +++ b/src/node_main_instance.h @@ -61,7 +61,8 @@ class NodeMainInstance { // TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h // and the environment creation routine in workers somehow. - std::unique_ptr CreateMainEnvironment(int* exit_code); + DeleteFnPtr CreateMainEnvironment( + int* exit_code); // If nullptr is returned, the binary is not built with embedded // snapshot. diff --git a/src/node_platform.cc b/src/node_platform.cc index 5b878f886a1320..e6e880b87b5380 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -402,6 +402,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { void NodePlatform::DrainTasks(Isolate* isolate) { std::shared_ptr per_isolate = ForIsolate(isolate); + if (!per_isolate) return; do { // Worker tasks aren't associated with an Isolate. @@ -468,7 +469,9 @@ NodePlatform::ForIsolate(Isolate* isolate) { } bool NodePlatform::FlushForegroundTasks(Isolate* isolate) { - return ForIsolate(isolate)->FlushForegroundTasksInternal(); + std::shared_ptr per_isolate = ForIsolate(isolate); + if (!per_isolate) return false; + return per_isolate->FlushForegroundTasksInternal(); } bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; } diff --git a/src/node_worker.cc b/src/node_worker.cc index 440f09f7b66e96..ec0fdf1cc0fad1 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -280,26 +280,18 @@ void Worker::Run() { if (!env_) return; env_->set_can_call_into_js(false); - Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, - Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); { - Context::Scope context_scope(env_->context()); - { - Mutex::ScopedLock lock(mutex_); - stopped_ = true; - this->env_ = nullptr; - } - env_->set_stopping(true); - env_->stop_sub_worker_contexts(); - env_->RunCleanup(); - RunAtExit(env_.get()); - - // This call needs to be made while the `Environment` is still alive - // because we assume that it is available for async tracking in the - // NodePlatform implementation. - platform_->DrainTasks(isolate_); + Mutex::ScopedLock lock(mutex_); + stopped_ = true; + this->env_ = nullptr; } + + // TODO(addaleax): Try moving DisallowJavascriptExecutionScope into + // FreeEnvironment(). + Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, + Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); + env_.reset(); }); if (is_stopped()) return; From 3f9fa28ee0da2e6c297df5f4e1b376a754d164f6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 11 Nov 2019 12:29:07 +0000 Subject: [PATCH 02/37] src: fix memory leak in CreateEnvironment when bootstrap fails PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index d4fa856b8d59ea..ad496a6621d10c 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -336,8 +336,10 @@ Environment* CreateEnvironment(IsolateData* isolate_data, Environment::kOwnsProcessState | Environment::kOwnsInspector)); env->InitializeLibuv(per_process::v8_is_profiling); - if (env->RunBootstrapping().IsEmpty()) + if (env->RunBootstrapping().IsEmpty()) { + FreeEnvironment(env); return nullptr; + } return env; } From 459973ca3226ea57538b26b025c1173609b748ce Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 18:57:10 +0000 Subject: [PATCH 03/37] src: move worker_context from Environment to IsolateData Workers are fully in control of their Isolates, and this helps avoid a problem with later changes to `CreateEnvironment()` because now we can run the boostrapping code inside the latter. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/env-inl.h | 16 ++++++++++------ src/env.cc | 6 +++--- src/env.h | 7 ++++--- src/node_worker.cc | 2 +- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index acb939ce72b56c..93b71c3194fa56 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -69,6 +69,15 @@ inline v8::Local IsolateData::async_wrap_provider(int index) const { return async_wrap_providers_[index].Get(isolate_); } +inline void IsolateData::set_worker_context(worker::Worker* context) { + CHECK_NULL(worker_context_); // Should be set only once. + worker_context_ = context; +} + +inline worker::Worker* IsolateData::worker_context() const { + return worker_context_; +} + inline AsyncHooks::AsyncHooks() : async_ids_stack_(env()->isolate(), 16 * 2), fields_(env()->isolate(), kFieldsCount), @@ -861,12 +870,7 @@ inline uint64_t Environment::thread_id() const { } inline worker::Worker* Environment::worker_context() const { - return worker_context_; -} - -inline void Environment::set_worker_context(worker::Worker* context) { - CHECK_NULL(worker_context_); // Should be set only once. - worker_context_ = context; + return isolate_data()->worker_context(); } inline void Environment::add_sub_worker_context(worker::Worker* context) { diff --git a/src/env.cc b/src/env.cc index a0d98637a27b79..653f2e3f9e8753 100644 --- a/src/env.cc +++ b/src/env.cc @@ -973,7 +973,7 @@ void Environment::Exit(int exit_code) { DisposePlatform(); exit(exit_code); } else { - worker_context_->Exit(exit_code); + worker_context()->Exit(exit_code); } } @@ -987,8 +987,8 @@ void Environment::stop_sub_worker_contexts() { } Environment* Environment::worker_parent_env() const { - if (worker_context_ == nullptr) return nullptr; - return worker_context_->env(); + if (worker_context() == nullptr) return nullptr; + return worker_context()->env(); } void Environment::BuildEmbedderGraph(Isolate* isolate, diff --git a/src/env.h b/src/env.h index 0632a96260f4d6..72381c6cf4ad4e 100644 --- a/src/env.h +++ b/src/env.h @@ -504,6 +504,9 @@ class IsolateData : public MemoryRetainer { inline v8::ArrayBuffer::Allocator* allocator() const; inline NodeArrayBufferAllocator* node_allocator() const; + inline worker::Worker* worker_context() const; + inline void set_worker_context(worker::Worker* context); + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) @@ -552,6 +555,7 @@ class IsolateData : public MemoryRetainer { const bool uses_node_allocator_; MultiIsolatePlatform* platform_; std::shared_ptr options_; + worker::Worker* worker_context_ = nullptr; }; struct ContextInfo { @@ -1074,7 +1078,6 @@ class Environment : public MemoryRetainer { inline uint64_t thread_id() const; inline worker::Worker* worker_context() const; Environment* worker_parent_env() const; - inline void set_worker_context(worker::Worker* context); inline void add_sub_worker_context(worker::Worker* context); inline void remove_sub_worker_context(worker::Worker* context); void stop_sub_worker_contexts(); @@ -1390,8 +1393,6 @@ class Environment : public MemoryRetainer { std::vector> file_handle_read_wrap_freelist_; - worker::Worker* worker_context_ = nullptr; - std::list extra_linked_bindings_; Mutex extra_linked_bindings_mutex_; diff --git a/src/node_worker.cc b/src/node_worker.cc index ec0fdf1cc0fad1..5cac7d0a975de5 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -185,6 +185,7 @@ class WorkerThreadData { CHECK(isolate_data_); if (w_->per_isolate_opts_) isolate_data_->set_options(std::move(w_->per_isolate_opts_)); + isolate_data_->set_worker_context(w_); } Mutex::ScopedLock lock(w_->mutex_); @@ -327,7 +328,6 @@ void Worker::Run() { CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); env_->set_abort_on_uncaught_exception(false); - env_->set_worker_context(this); env_->InitializeLibuv(start_profiler_idle_notifier_); } From 2c87b179151a2d886fffda62df0d9325608fcd0c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 19:36:07 +0000 Subject: [PATCH 04/37] src: associate is_main_thread() with worker_context() In our codebase, the assumption generally is that `!is_main_thread()` means that the current Environment belongs to a Node.js Worker thread. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 3 +-- src/env-inl.h | 2 +- src/env.h | 1 - src/node_main_instance.cc | 3 +-- 4 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index ad496a6621d10c..ee7732c93b8563 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -332,8 +332,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data, context, args, exec_args, - static_cast(Environment::kIsMainThread | - Environment::kOwnsProcessState | + static_cast(Environment::kOwnsProcessState | Environment::kOwnsInspector)); env->InitializeLibuv(per_process::v8_is_profiling); if (env->RunBootstrapping().IsEmpty()) { diff --git a/src/env-inl.h b/src/env-inl.h index 93b71c3194fa56..b82ac88fc7fafc 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -854,7 +854,7 @@ inline void Environment::set_has_serialized_options(bool value) { } inline bool Environment::is_main_thread() const { - return flags_ & kIsMainThread; + return worker_context() == nullptr; } inline bool Environment::owns_process_state() const { diff --git a/src/env.h b/src/env.h index 72381c6cf4ad4e..5e9c7c0495366f 100644 --- a/src/env.h +++ b/src/env.h @@ -876,7 +876,6 @@ class Environment : public MemoryRetainer { enum Flags { kNoFlags = 0, - kIsMainThread = 1 << 0, kOwnsProcessState = 1 << 1, kOwnsInspector = 1 << 2, }; diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index c6533321bd6b64..cbbff1ad742006 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -204,8 +204,7 @@ NodeMainInstance::CreateMainEnvironment(int* exit_code) { context, args_, exec_args_, - static_cast(Environment::kIsMainThread | - Environment::kOwnsProcessState | + static_cast(Environment::kOwnsProcessState | Environment::kOwnsInspector)) }; env->InitializeLibuv(per_process::v8_is_profiling); env->InitializeDiagnostics(); From 0e9f4a95ffbe2127fb28aaf3e6b9de5580239cf2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 11 Nov 2019 14:24:13 +0000 Subject: [PATCH 05/37] src: align worker and main thread code with embedder API This addresses some long-standing TODOs by Joyee and me about making the embedder API more powerful and us less reliant on internal APIs for creating the main thread and Workers. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 88 ++++++++++++++++++++++++++++++++++++--- src/env-inl.h | 11 +++-- src/env.cc | 47 +++++++++++++++------ src/env.h | 20 ++++----- src/node.cc | 41 +++++++++++++----- src/node.h | 52 ++++++++++++++++++++++- src/node_internals.h | 1 + src/node_main_instance.cc | 16 ++----- src/node_main_instance.h | 2 - src/node_worker.cc | 81 +++++++++++++++-------------------- src/node_worker.h | 7 +--- 11 files changed, 251 insertions(+), 115 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index ee7732c93b8563..7404400297d5e1 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -7,6 +7,10 @@ #include "node_v8_platform-inl.h" #include "uv.h" +#if HAVE_INSPECTOR +#include "inspector/worker_inspector.h" // ParentInspectorHandle +#endif + namespace node { using errors::TryCatchScope; using v8::Array; @@ -319,26 +323,40 @@ Environment* CreateEnvironment(IsolateData* isolate_data, const char* const* argv, int exec_argc, const char* const* exec_argv) { + return CreateEnvironment( + isolate_data, context, + std::vector(argv, argv + argc), + std::vector(exec_argv, exec_argv + exec_argc)); +} + +Environment* CreateEnvironment( + IsolateData* isolate_data, + Local context, + const std::vector& args, + const std::vector& exec_args, + EnvironmentFlags::Flags flags, + ThreadId thread_id) { Isolate* isolate = context->GetIsolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(context); // TODO(addaleax): This is a much better place for parsing per-Environment // options than the global parse call. - std::vector args(argv, argv + argc); - std::vector exec_args(exec_argv, exec_argv + exec_argc); - // TODO(addaleax): Provide more sensible flags, in an embedder-accessible way. Environment* env = new Environment( isolate_data, context, args, exec_args, - static_cast(Environment::kOwnsProcessState | - Environment::kOwnsInspector)); - env->InitializeLibuv(per_process::v8_is_profiling); + flags, + thread_id); + if (flags & EnvironmentFlags::kOwnsProcessState) { + env->set_abort_on_uncaught_exception(false); + } + if (env->RunBootstrapping().IsEmpty()) { FreeEnvironment(env); return nullptr; } + return env; } @@ -363,6 +381,58 @@ void FreeEnvironment(Environment* env) { delete env; } +InspectorParentHandle::~InspectorParentHandle() {} + +// Hide the internal handle class from the public API. +#if HAVE_INSPECTOR +struct InspectorParentHandleImpl : public InspectorParentHandle { + std::unique_ptr impl; + + explicit InspectorParentHandleImpl( + std::unique_ptr&& impl) + : impl(std::move(impl)) {} +}; +#endif + +NODE_EXTERN std::unique_ptr GetInspectorParentHandle( + Environment* env, + ThreadId thread_id, + const char* url) { + CHECK_NOT_NULL(env); + CHECK_NE(thread_id.id, static_cast(-1)); +#if HAVE_INSPECTOR + return std::make_unique( + env->inspector_agent()->GetParentHandle(thread_id.id, url)); +#else + return {}; +#endif +} + +void LoadEnvironment(Environment* env) { + USE(LoadEnvironment(env, {})); +} + +MaybeLocal LoadEnvironment( + Environment* env, + std::unique_ptr inspector_parent_handle) { + env->InitializeLibuv(per_process::v8_is_profiling); + env->InitializeDiagnostics(); + +#if HAVE_INSPECTOR + if (inspector_parent_handle) { + env->InitializeInspector( + std::move(static_cast( + inspector_parent_handle.get())->impl)); + } else { + env->InitializeInspector({}); + } +#endif + + // TODO(joyeecheung): Allow embedders to customize the entry + // point more directly without using _third_party_main.js + return StartExecution(env); +} + Environment* GetCurrentEnvironment(Local context) { return Environment::GetCurrent(context); } @@ -579,4 +649,10 @@ void AddLinkedBinding(Environment* env, AddLinkedBinding(env, mod); } +static std::atomic next_thread_id{0}; + +ThreadId AllocateEnvironmentThreadId() { + return ThreadId { next_thread_id++ }; +} + } // namespace node diff --git a/src/env-inl.h b/src/env-inl.h index b82ac88fc7fafc..598b84c07ca8d2 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -813,8 +813,9 @@ void Environment::SetImmediateThreadsafe(Fn&& cb, CallbackFlags::Flags flags) { { Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); native_immediates_threadsafe_.Push(std::move(callback)); + if (task_queues_async_initialized_) + uv_async_send(&task_queues_async_); } - uv_async_send(&task_queues_async_); } template @@ -824,8 +825,9 @@ void Environment::RequestInterrupt(Fn&& cb) { { Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); native_immediates_interrupts_.Push(std::move(callback)); + if (task_queues_async_initialized_) + uv_async_send(&task_queues_async_); } - uv_async_send(&task_queues_async_); RequestInterruptFromV8(); } @@ -858,11 +860,11 @@ inline bool Environment::is_main_thread() const { } inline bool Environment::owns_process_state() const { - return flags_ & kOwnsProcessState; + return flags_ & EnvironmentFlags::kOwnsProcessState; } inline bool Environment::owns_inspector() const { - return flags_ & kOwnsInspector; + return flags_ & EnvironmentFlags::kOwnsInspector; } inline uint64_t Environment::thread_id() const { @@ -1176,6 +1178,7 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) { inline void Environment::RegisterFinalizationGroupForCleanup( v8::Local group) { cleanup_finalization_groups_.emplace_back(isolate(), group); + DCHECK(task_queues_async_initialized_); uv_async_send(&task_queues_async_); } diff --git a/src/env.cc b/src/env.cc index 653f2e3f9e8753..f0b8b521f70363 100644 --- a/src/env.cc +++ b/src/env.cc @@ -257,12 +257,6 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() { USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args)); } -static std::atomic next_thread_id{0}; - -uint64_t Environment::AllocateThreadId() { - return next_thread_id++; -} - void Environment::CreateProperties() { HandleScope handle_scope(isolate_); Local ctx = context(); @@ -319,8 +313,8 @@ Environment::Environment(IsolateData* isolate_data, Local context, const std::vector& args, const std::vector& exec_args, - Flags flags, - uint64_t thread_id) + EnvironmentFlags::Flags flags, + ThreadId thread_id) : isolate_(context->GetIsolate()), isolate_data_(isolate_data), immediate_info_(context->GetIsolate()), @@ -332,7 +326,8 @@ Environment::Environment(IsolateData* isolate_data, should_abort_on_uncaught_toggle_(isolate_, 1), stream_base_state_(isolate_, StreamBase::kNumStreamBaseStateFields), flags_(flags), - thread_id_(thread_id == kNoThreadId ? AllocateThreadId() : thread_id), + thread_id_(thread_id.id == static_cast(-1) ? + AllocateEnvironmentThreadId().id : thread_id.id), fs_stats_field_array_(isolate_, kFsStatsBufferLength), fs_stats_field_bigint_array_(isolate_, kFsStatsBufferLength), context_(context->GetIsolate(), context) { @@ -340,6 +335,14 @@ Environment::Environment(IsolateData* isolate_data, HandleScope handle_scope(isolate()); Context::Scope context_scope(context); + // Set some flags if only kDefaultFlags was passed. This can make API version + // transitions easier for embedders. + if (flags_ & EnvironmentFlags::kDefaultFlags) { + flags_ = flags_ | + EnvironmentFlags::kOwnsProcessState | + EnvironmentFlags::kOwnsInspector; + } + set_env_vars(per_process::system_environment); enabled_debug_list_.Parse(this); @@ -358,6 +361,10 @@ Environment::Environment(IsolateData* isolate_data, AssignToContext(context, ContextInfo("")); + static uv_once_t init_once = UV_ONCE_INIT; + uv_once(&init_once, InitThreadLocalOnce); + uv_key_set(&thread_local_env, this); + if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) { trace_state_observer_ = std::make_unique(this); if (TracingController* tracing_controller = writer->GetTracingController()) @@ -407,6 +414,9 @@ Environment::Environment(IsolateData* isolate_data, Environment::~Environment() { if (interrupt_data_ != nullptr) *interrupt_data_ = nullptr; + // FreeEnvironment() should have set this. + CHECK(is_stopping()); + isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback( BuildEmbedderGraph, this); @@ -493,6 +503,15 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { uv_unref(reinterpret_cast(&idle_check_handle_)); uv_unref(reinterpret_cast(&task_queues_async_)); + { + Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); + task_queues_async_initialized_ = true; + if (native_immediates_threadsafe_.size() > 0 || + native_immediates_interrupts_.size() > 0) { + uv_async_send(&task_queues_async_); + } + } + // Register clean-up cb to be called to clean up the handles // when the environment is freed, note that they are not cleaned in // the one environment per process setup, but will be called in @@ -502,10 +521,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { if (start_profiler_idle_notifier) { StartProfilerIdleNotifier(); } - - static uv_once_t init_once = UV_ONCE_INIT; - uv_once(&init_once, InitThreadLocalOnce); - uv_key_set(&thread_local_env, this); } void Environment::ExitEnv() { @@ -539,6 +554,11 @@ void Environment::RegisterHandleCleanups() { } void Environment::CleanupHandles() { + { + Mutex::ScopedLock lock(native_immediates_threadsafe_mutex_); + task_queues_async_initialized_ = false; + } + Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(), Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); @@ -1103,6 +1123,7 @@ void Environment::CleanupFinalizationGroups() { if (try_catch.HasCaught() && !try_catch.HasTerminated()) errors::TriggerUncaughtException(isolate(), try_catch); // Re-schedule the execution of the remainder of the queue. + CHECK(task_queues_async_initialized_); uv_async_send(&task_queues_async_); return; } diff --git a/src/env.h b/src/env.h index 5e9c7c0495366f..8331a79dbc9918 100644 --- a/src/env.h +++ b/src/env.h @@ -874,12 +874,6 @@ class Environment : public MemoryRetainer { inline void PushAsyncCallbackScope(); inline void PopAsyncCallbackScope(); - enum Flags { - kNoFlags = 0, - kOwnsProcessState = 1 << 1, - kOwnsInspector = 1 << 2, - }; - static inline Environment* GetCurrent(v8::Isolate* isolate); static inline Environment* GetCurrent(v8::Local context); static inline Environment* GetCurrent( @@ -898,8 +892,8 @@ class Environment : public MemoryRetainer { v8::Local context, const std::vector& args, const std::vector& exec_args, - Flags flags = Flags(), - uint64_t thread_id = kNoThreadId); + EnvironmentFlags::Flags flags, + ThreadId thread_id); ~Environment() override; void InitializeLibuv(bool start_profiler_idle_notifier); @@ -1068,9 +1062,6 @@ class Environment : public MemoryRetainer { inline bool has_serialized_options() const; inline void set_has_serialized_options(bool has_serialized_options); - static uint64_t AllocateThreadId(); - static constexpr uint64_t kNoThreadId = -1; - inline bool is_main_thread() const; inline bool owns_process_state() const; inline bool owns_inspector() const; @@ -1350,7 +1341,7 @@ class Environment : public MemoryRetainer { bool has_serialized_options_ = false; std::atomic_bool can_call_into_js_ { true }; - Flags flags_; + uint64_t flags_; uint64_t thread_id_; std::unordered_set sub_worker_contexts_; @@ -1409,6 +1400,11 @@ class Environment : public MemoryRetainer { Mutex native_immediates_threadsafe_mutex_; NativeImmediateQueue native_immediates_threadsafe_; NativeImmediateQueue native_immediates_interrupts_; + // Also guarded by native_immediates_threadsafe_mutex_. This can be used when + // trying to post tasks from other threads to an Environment, as the libuv + // handle for the immediate queues (task_queues_async_) may not be initialized + // yet or already have been destroyed. + bool task_queues_async_initialized_ = false; void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); diff --git a/src/node.cc b/src/node.cc index e93ddecb872f56..c34a875768315c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -190,8 +190,8 @@ MaybeLocal ExecuteBootstrapper(Environment* env, int Environment::InitializeInspector( std::unique_ptr parent_handle) { std::string inspector_path; + bool is_main = !parent_handle; if (parent_handle) { - DCHECK(!is_main_thread()); inspector_path = parent_handle->url(); inspector_agent_->SetParentHandle(std::move(parent_handle)); } else { @@ -205,7 +205,7 @@ int Environment::InitializeInspector( inspector_agent_->Start(inspector_path, options_->debug_options(), inspector_host_port(), - is_main_thread()); + is_main); if (options_->debug_options().inspector_enabled && !inspector_agent_->IsListening()) { return 12; // Signal internal error @@ -394,7 +394,7 @@ MaybeLocal StartExecution(Environment* env, const char* main_script_id) { ExecuteBootstrapper(env, main_script_id, ¶meters, &arguments)); } -MaybeLocal StartMainThreadExecution(Environment* env) { +MaybeLocal StartExecution(Environment* env) { // To allow people to extend Node in different ways, this hook allows // one to drop a file lib/_third_party_main.js into the build // directory which will be executed instead of Node's normal loading. @@ -402,6 +402,10 @@ MaybeLocal StartMainThreadExecution(Environment* env) { return StartExecution(env, "internal/main/run_third_party_main"); } + if (env->worker_context() != nullptr) { + return StartExecution(env, "internal/main/worker_thread"); + } + std::string first_argv; if (env->argv().size() > 1) { first_argv = env->argv()[1]; @@ -440,15 +444,30 @@ MaybeLocal StartMainThreadExecution(Environment* env) { return StartExecution(env, "internal/main/eval_stdin"); } -void LoadEnvironment(Environment* env) { - CHECK(env->is_main_thread()); - // TODO(joyeecheung): Not all of the execution modes in - // StartMainThreadExecution() make sense for embedders. Pick the - // useful ones out, and allow embedders to customize the entry - // point more directly without using _third_party_main.js - USE(StartMainThreadExecution(env)); +#ifdef __POSIX__ +typedef void (*sigaction_cb)(int signo, siginfo_t* info, void* ucontext); +#endif +#if NODE_USE_V8_WASM_TRAP_HANDLER +static std::atomic previous_sigsegv_action; + +void TrapWebAssemblyOrContinue(int signo, siginfo_t* info, void* ucontext) { + if (!v8::TryHandleWebAssemblyTrapPosix(signo, info, ucontext)) { + sigaction_cb prev = previous_sigsegv_action.load(); + if (prev != nullptr) { + prev(signo, info, ucontext); + } else { + // Reset to the default signal handler, i.e. cause a hard crash. + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = SIG_DFL; + CHECK_EQ(sigaction(signo, &sa, nullptr), 0); + + ResetStdio(); + raise(signo); + } + } } - +#endif // NODE_USE_V8_WASM_TRAP_HANDLER #ifdef __POSIX__ void RegisterSignalHandler(int signal, diff --git a/src/node.h b/src/node.h index e9cf73ede5373a..4357291de79cec 100644 --- a/src/node.h +++ b/src/node.h @@ -363,18 +363,66 @@ NODE_EXTERN IsolateData* CreateIsolateData( ArrayBufferAllocator* allocator = nullptr); NODE_EXTERN void FreeIsolateData(IsolateData* isolate_data); -// TODO(addaleax): Add an official variant using STL containers, and move -// per-Environment options parsing here. +struct ThreadId { + uint64_t id = static_cast(-1); +}; +NODE_EXTERN ThreadId AllocateEnvironmentThreadId(); + +namespace EnvironmentFlags { +enum Flags : uint64_t { + kNoFlags = 0, + // Use the default behaviour for Node.js instances. + kDefaultFlags = 1 << 0, + // Controls whether this Environment is allowed to affect per-process state + // (e.g. cwd, process title, uid, etc.). + // This is set when using kDefaultFlags. + kOwnsProcessState = 1 << 1, + // Set if this Environment instance is associated with the global inspector + // handling code (i.e. listening on SIGUSR1). + // This is set when using kDefaultFlags. + kOwnsInspector = 1 << 2 +}; +} // namespace EnvironmentFlags + +// TODO(addaleax): Maybe move per-Environment options parsing here. // Returns nullptr when the Environment cannot be created e.g. there are // pending JavaScript exceptions. +// It is recommended to use the second variant taking a flags argument. NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data, v8::Local context, int argc, const char* const* argv, int exec_argc, const char* const* exec_argv); +NODE_EXTERN Environment* CreateEnvironment( + IsolateData* isolate_data, + v8::Local context, + const std::vector& args, + const std::vector& exec_args, + EnvironmentFlags::Flags flags = EnvironmentFlags::kDefaultFlags, + ThreadId thread_id = {} /* allocates a thread id automatically */); +struct InspectorParentHandle { + virtual ~InspectorParentHandle(); +}; +// Returns a handle that can be passed to `LoadEnvironment()`, making the +// child Environment accessible to the inspector as if it were a Node.js Worker. +// `child_thread_id` can be created using `AllocateEnvironmentThreadId()` +// and then later passed on to `CreateEnvironment()` to create the child +// Environment. +// This method should not be called while the parent Environment is active +// on another thread. +NODE_EXTERN std::unique_ptr GetInspectorParentHandle( + Environment* parent_env, + ThreadId child_thread_id, + const char* child_url); + +// TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload +// and provide a more flexible approach than third_party_main. NODE_EXTERN void LoadEnvironment(Environment* env); +NODE_EXTERN v8::MaybeLocal LoadEnvironment( + Environment* env, + std::unique_ptr inspector_parent_handle); NODE_EXTERN void FreeEnvironment(Environment* env); // This may return nullptr if context is not associated with a Node instance. diff --git a/src/node_internals.h b/src/node_internals.h index c1555b312e2f22..38de262e05e8d9 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -301,6 +301,7 @@ void DefineZlibConstants(v8::Local target); v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, uv_loop_t* event_loop, MultiIsolatePlatform* platform); +v8::MaybeLocal StartExecution(Environment* env); v8::MaybeLocal StartExecution(Environment* env, const char* main_script_id); v8::MaybeLocal GetPerContextExports(v8::Local context); diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index cbbff1ad742006..f638e26dba5a04 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -172,8 +172,6 @@ int NodeMainInstance::Run() { return exit_code; } -// TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h -// and the environment creation routine in workers somehow. DeleteFnPtr NodeMainInstance::CreateMainEnvironment(int* exit_code) { *exit_code = 0; // Reset the exit code to 0 @@ -199,26 +197,18 @@ NodeMainInstance::CreateMainEnvironment(int* exit_code) { CHECK(!context.IsEmpty()); Context::Scope context_scope(context); - DeleteFnPtr env { new Environment( + DeleteFnPtr env { CreateEnvironment( isolate_data_.get(), context, args_, exec_args_, - static_cast(Environment::kOwnsProcessState | - Environment::kOwnsInspector)) }; - env->InitializeLibuv(per_process::v8_is_profiling); - env->InitializeDiagnostics(); + EnvironmentFlags::kDefaultFlags) }; - // TODO(joyeecheung): when we snapshot the bootstrapped context, - // the inspector and diagnostics setup should after after deserialization. -#if HAVE_INSPECTOR - *exit_code = env->InitializeInspector({}); -#endif if (*exit_code != 0) { return env; } - if (env->RunBootstrapping().IsEmpty()) { + if (env == nullptr) { *exit_code = 1; } diff --git a/src/node_main_instance.h b/src/node_main_instance.h index cc9f50b9222de3..b8178c2774e795 100644 --- a/src/node_main_instance.h +++ b/src/node_main_instance.h @@ -59,8 +59,6 @@ class NodeMainInstance { IsolateData* isolate_data() { return isolate_data_.get(); } - // TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h - // and the environment creation routine in workers somehow. DeleteFnPtr CreateMainEnvironment( int* exit_code); diff --git a/src/node_worker.cc b/src/node_worker.cc index 5cac7d0a975de5..dcce464cf80f88 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -8,10 +8,6 @@ #include "util-inl.h" #include "async_wrap-inl.h" -#if HAVE_INSPECTOR -#include "inspector/worker_inspector.h" // ParentInspectorHandle -#endif - #include #include #include @@ -54,10 +50,10 @@ Worker::Worker(Environment* env, exec_argv_(exec_argv), platform_(env->isolate_data()->platform()), array_buffer_allocator_(ArrayBufferAllocator::Create()), - start_profiler_idle_notifier_(env->profiler_idle_notifier_started()), - thread_id_(Environment::AllocateThreadId()), + thread_id_(AllocateEnvironmentThreadId()), env_vars_(env_vars) { - Debug(this, "Creating new worker instance with thread id %llu", thread_id_); + Debug(this, "Creating new worker instance with thread id %llu", + thread_id_.id); // Set up everything that needs to be set up in the parent environment. parent_port_ = MessagePort::New(env, env->context()); @@ -75,19 +71,17 @@ Worker::Worker(Environment* env, object()->Set(env->context(), env->thread_id_string(), - Number::New(env->isolate(), static_cast(thread_id_))) + Number::New(env->isolate(), static_cast(thread_id_.id))) .Check(); -#if HAVE_INSPECTOR - inspector_parent_handle_ = - env->inspector_agent()->GetParentHandle(thread_id_, url); -#endif + inspector_parent_handle_ = GetInspectorParentHandle( + env, thread_id_, url.c_str()); argv_ = std::vector{env->argv()[0]}; // Mark this Worker object as weak until we actually start the thread. MakeWeak(); - Debug(this, "Preparation for worker %llu finished", thread_id_); + Debug(this, "Preparation for worker %llu finished", thread_id_.id); } bool Worker::is_stopped() const { @@ -193,7 +187,7 @@ class WorkerThreadData { } ~WorkerThreadData() { - Debug(w_, "Worker %llu dispose isolate", w_->thread_id_); + Debug(w_, "Worker %llu dispose isolate", w_->thread_id_.id); Isolate* isolate; { Mutex::ScopedLock lock(w_->mutex_); @@ -254,19 +248,19 @@ size_t Worker::NearHeapLimit(void* data, size_t current_heap_limit, void Worker::Run() { std::string name = "WorkerThread "; - name += std::to_string(thread_id_); + name += std::to_string(thread_id_.id); TRACE_EVENT_METADATA1( "__metadata", "thread_name", "name", TRACE_STR_COPY(name.c_str())); CHECK_NOT_NULL(platform_); - Debug(this, "Creating isolate for worker with id %llu", thread_id_); + Debug(this, "Creating isolate for worker with id %llu", thread_id_.id); WorkerThreadData data(this); if (isolate_ == nullptr) return; CHECK(data.loop_is_usable()); - Debug(this, "Starting worker with id %llu", thread_id_); + Debug(this, "Starting worker with id %llu", thread_id_.id); { Locker locker(isolate_); Isolate::Scope isolate_scope(isolate_); @@ -317,42 +311,34 @@ void Worker::Run() { CHECK(!context.IsEmpty()); Context::Scope context_scope(context); { - // TODO(addaleax): Use CreateEnvironment(), or generally another - // public API. - env_.reset(new Environment(data.isolate_data_.get(), - context, - std::move(argv_), - std::move(exec_argv_), - Environment::kNoFlags, - thread_id_)); + env_.reset(CreateEnvironment( + data.isolate_data_.get(), + context, + std::move(argv_), + std::move(exec_argv_), + EnvironmentFlags::kNoFlags, + thread_id_)); + if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); - env_->set_abort_on_uncaught_exception(false); - - env_->InitializeLibuv(start_profiler_idle_notifier_); } { Mutex::ScopedLock lock(mutex_); if (stopped_) return; this->env_ = env_.get(); } - Debug(this, "Created Environment for worker with id %llu", thread_id_); + Debug(this, "Created Environment for worker with id %llu", thread_id_.id); if (is_stopped()) return; { - env_->InitializeDiagnostics(); -#if HAVE_INSPECTOR - env_->InitializeInspector(std::move(inspector_parent_handle_)); -#endif - HandleScope handle_scope(isolate_); - - if (!env_->RunBootstrapping().IsEmpty()) { - CreateEnvMessagePort(env_.get()); - if (is_stopped()) return; - Debug(this, "Created message port for worker %llu", thread_id_); - USE(StartExecution(env_.get(), "internal/main/worker_thread")); + CreateEnvMessagePort(env_.get()); + Debug(this, "Created message port for worker %llu", thread_id_.id); + if (LoadEnvironment(env_.get(), + std::move(inspector_parent_handle_)) + .IsEmpty()) { + return; } - Debug(this, "Loaded environment for worker %llu", thread_id_); + Debug(this, "Loaded environment for worker %llu", thread_id_.id); } if (is_stopped()) return; @@ -392,11 +378,11 @@ void Worker::Run() { exit_code_ = exit_code; Debug(this, "Exiting thread for worker %llu with exit code %d", - thread_id_, exit_code_); + thread_id_.id, exit_code_); } } - Debug(this, "Worker %llu thread stops", thread_id_); + Debug(this, "Worker %llu thread stops", thread_id_.id); } void Worker::CreateEnvMessagePort(Environment* env) { @@ -455,7 +441,7 @@ Worker::~Worker() { CHECK_NULL(env_); CHECK(thread_joined_); - Debug(this, "Worker %llu destroyed", thread_id_); + Debug(this, "Worker %llu destroyed", thread_id_.id); } void Worker::New(const FunctionCallbackInfo& args) { @@ -663,7 +649,7 @@ void Worker::StopThread(const FunctionCallbackInfo& args) { Worker* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); - Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_); + Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_.id); w->Exit(1); } @@ -699,7 +685,7 @@ Local Worker::GetResourceLimits(Isolate* isolate) const { void Worker::Exit(int code) { Mutex::ScopedLock lock(mutex_); - Debug(this, "Worker %llu called Exit(%d)", thread_id_, code); + Debug(this, "Worker %llu called Exit(%d)", thread_id_.id, code); if (env_ != nullptr) { exit_code_ = code; Stop(env_); @@ -726,7 +712,7 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo& args) { Worker* w; ASSIGN_OR_RETURN_UNWRAP(&w, args.This()); - Debug(w, "Worker %llu taking heap snapshot", w->thread_id_); + Debug(w, "Worker %llu taking heap snapshot", w->thread_id_.id); Environment* env = w->env(); AsyncHooks::DefaultTriggerAsyncIdScope trigger_id_scope(w); @@ -766,6 +752,7 @@ namespace { void GetEnvMessagePort(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Local port = env->message_port(); + CHECK_IMPLIES(!env->is_main_thread(), !port.IsEmpty()); if (!port.IsEmpty()) { CHECK_EQ(port->CreationContext()->GetIsolate(), args.GetIsolate()); args.GetReturnValue().Set(port); diff --git a/src/node_worker.h b/src/node_worker.h index 3611a8536fe040..a7d02914826563 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -75,12 +75,9 @@ class Worker : public AsyncWrap { MultiIsolatePlatform* platform_; std::shared_ptr array_buffer_allocator_; v8::Isolate* isolate_ = nullptr; - bool start_profiler_idle_notifier_; uv_thread_t tid_; -#if HAVE_INSPECTOR - std::unique_ptr inspector_parent_handle_; -#endif + std::unique_ptr inspector_parent_handle_; // This mutex protects access to all variables listed below it. mutable Mutex mutex_; @@ -89,7 +86,7 @@ class Worker : public AsyncWrap { const char* custom_error_ = nullptr; std::string custom_error_str_; int exit_code_ = 0; - uint64_t thread_id_ = -1; + ThreadId thread_id_; uintptr_t stack_base_ = 0; // Custom resource constraints: From c694f0862a5e71a41ba6298eea55066ce638e040 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 13 Nov 2019 15:54:57 +0000 Subject: [PATCH 06/37] src: provide a variant of LoadEnvironment taking a callback This allows embedders to flexibly control how they start JS code rather than using `third_party_main`. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 7 ++-- src/node.cc | 29 +++++++++++++---- src/node.h | 15 +++++++-- src/node_internals.h | 5 +-- src/node_worker.cc | 1 + test/cctest/test_environment.cc | 58 +++++++++++++++++++++++++++++++++ 6 files changed, 99 insertions(+), 16 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 7404400297d5e1..7a9e608d42f45f 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -409,11 +409,12 @@ NODE_EXTERN std::unique_ptr GetInspectorParentHandle( } void LoadEnvironment(Environment* env) { - USE(LoadEnvironment(env, {})); + USE(LoadEnvironment(env, nullptr, {})); } MaybeLocal LoadEnvironment( Environment* env, + StartExecutionCallback cb, std::unique_ptr inspector_parent_handle) { env->InitializeLibuv(per_process::v8_is_profiling); env->InitializeDiagnostics(); @@ -428,9 +429,7 @@ MaybeLocal LoadEnvironment( } #endif - // TODO(joyeecheung): Allow embedders to customize the entry - // point more directly without using _third_party_main.js - return StartExecution(env); + return StartExecution(env, cb); } Environment* GetCurrentEnvironment(Local context) { diff --git a/src/node.cc b/src/node.cc index c34a875768315c..46e8f74cc286f7 100644 --- a/src/node.cc +++ b/src/node.cc @@ -364,6 +364,7 @@ void MarkBootstrapComplete(const FunctionCallbackInfo& args) { performance::NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE); } +static MaybeLocal StartExecution(Environment* env, const char* main_script_id) { EscapableHandleScope scope(env->isolate()); CHECK_NOT_NULL(main_script_id); @@ -384,17 +385,31 @@ MaybeLocal StartExecution(Environment* env, const char* main_script_id) { ->GetFunction(env->context()) .ToLocalChecked()}; - InternalCallbackScope callback_scope( - env, - Object::New(env->isolate()), - { 1, 0 }, - InternalCallbackScope::kSkipAsyncHooks); - return scope.EscapeMaybe( ExecuteBootstrapper(env, main_script_id, ¶meters, &arguments)); } -MaybeLocal StartExecution(Environment* env) { +MaybeLocal StartExecution(Environment* env, StartExecutionCallback cb) { + InternalCallbackScope callback_scope( + env, + Object::New(env->isolate()), + { 1, 0 }, + InternalCallbackScope::kSkipAsyncHooks); + + if (cb != nullptr) { + EscapableHandleScope scope(env->isolate()); + + if (StartExecution(env, "internal/bootstrap/environment").IsEmpty()) + return {}; + + StartExecutionCallbackInfo info = { + env->process_object(), + env->native_module_require(), + }; + + return scope.EscapeMaybe(cb(info)); + } + // To allow people to extend Node in different ways, this hook allows // one to drop a file lib/_third_party_main.js into the build // directory which will be executed instead of Node's normal loading. diff --git a/src/node.h b/src/node.h index 4357291de79cec..e511579b346809 100644 --- a/src/node.h +++ b/src/node.h @@ -73,6 +73,7 @@ #include "node_version.h" // NODE_MODULE_VERSION #include +#include #define NODE_MAKE_VERSION(major, minor, patch) \ ((major) * 0x1000 + (minor) * 0x100 + (patch)) @@ -417,12 +418,20 @@ NODE_EXTERN std::unique_ptr GetInspectorParentHandle( ThreadId child_thread_id, const char* child_url); -// TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload -// and provide a more flexible approach than third_party_main. +struct StartExecutionCallbackInfo { + v8::Local process_object; + v8::Local native_require; +}; + +using StartExecutionCallback = + std::function(const StartExecutionCallbackInfo&)>; + +// TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload. NODE_EXTERN void LoadEnvironment(Environment* env); NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, - std::unique_ptr inspector_parent_handle); + StartExecutionCallback cb, + std::unique_ptr inspector_parent_handle = {}); NODE_EXTERN void FreeEnvironment(Environment* env); // This may return nullptr if context is not associated with a Node instance. diff --git a/src/node_internals.h b/src/node_internals.h index 38de262e05e8d9..25d279e615109b 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -301,9 +301,10 @@ void DefineZlibConstants(v8::Local target); v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params, uv_loop_t* event_loop, MultiIsolatePlatform* platform); -v8::MaybeLocal StartExecution(Environment* env); +// This overload automatically picks the right 'main_script_id' if no callback +// was provided by the embedder. v8::MaybeLocal StartExecution(Environment* env, - const char* main_script_id); + StartExecutionCallback cb = nullptr); v8::MaybeLocal GetPerContextExports(v8::Local context); v8::MaybeLocal ExecuteBootstrapper( Environment* env, diff --git a/src/node_worker.cc b/src/node_worker.cc index dcce464cf80f88..846f4c82c4d26c 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -333,6 +333,7 @@ void Worker::Run() { CreateEnvMessagePort(env_.get()); Debug(this, "Created message port for worker %llu", thread_id_.id); if (LoadEnvironment(env_.get(), + nullptr, std::move(inspector_parent_handle_)) .IsEmpty()) { return; diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 75aa4b7d840e12..ae11f945d777a8 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -51,6 +51,35 @@ class EnvironmentTest : public EnvironmentTestFixture { // CHECK(result->IsString()); // } +TEST_F(EnvironmentTest, LoadEnvironmentWithCallback) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv}; + + v8::Local context = isolate_->GetCurrentContext(); + bool called_cb = false; + node::LoadEnvironment(*env, + [&](const node::StartExecutionCallbackInfo& info) + -> v8::MaybeLocal { + called_cb = true; + + CHECK(info.process_object->IsObject()); + CHECK(info.native_require->IsFunction()); + + v8::Local argv0 = info.process_object->Get( + context, + v8::String::NewFromOneByte( + isolate_, + reinterpret_cast("argv0"), + v8::NewStringType::kNormal).ToLocalChecked()).ToLocalChecked(); + CHECK(argv0->IsString()); + + return info.process_object; + }); + + CHECK(called_cb); +} + TEST_F(EnvironmentTest, AtExitWithEnvironment) { const v8::HandleScope handle_scope(isolate_); const Argv argv; @@ -188,6 +217,35 @@ static void at_exit_js(void* arg) { called_at_exit_js = true; } +TEST_F(EnvironmentTest, SetImmediateCleanup) { + int called = 0; + int called_unref = 0; + + { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv}; + + node::LoadEnvironment(*env, + [&](const node::StartExecutionCallbackInfo& info) + -> v8::MaybeLocal { + return v8::Object::New(isolate_); + }); + + (*env)->SetImmediate([&](node::Environment* env_arg) { + EXPECT_EQ(env_arg, *env); + called++; + }); + (*env)->SetUnrefImmediate([&](node::Environment* env_arg) { + EXPECT_EQ(env_arg, *env); + called_unref++; + }); + } + + EXPECT_EQ(called, 1); + EXPECT_EQ(called_unref, 0); +} + static char hello[] = "hello"; TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) { From 8a1255cd3089b8473794f342180f30590351c344 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 15:42:09 +0100 Subject: [PATCH 07/37] src: add LoadEnvironment() variant taking a string Allow passing a string as the main module rather than using the callback variant. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 36 ++++++++++++++++++++++++++++++++- src/env-inl.h | 5 +++++ src/env.h | 7 +++++++ src/node.h | 4 ++++ src/node_native_module.cc | 8 ++++++++ src/node_native_module.h | 2 ++ src/node_native_module_env.cc | 4 ++++ src/node_native_module_env.h | 1 + src/node_worker.cc | 2 +- test/cctest/test_environment.cc | 27 +++++++++++++++++++++++++ 10 files changed, 94 insertions(+), 2 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 7a9e608d42f45f..b43b43f1163b8d 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -409,7 +409,9 @@ NODE_EXTERN std::unique_ptr GetInspectorParentHandle( } void LoadEnvironment(Environment* env) { - USE(LoadEnvironment(env, nullptr, {})); + USE(LoadEnvironment(env, + StartExecutionCallback{}, + {})); } MaybeLocal LoadEnvironment( @@ -432,6 +434,38 @@ MaybeLocal LoadEnvironment( return StartExecution(env, cb); } +MaybeLocal LoadEnvironment( + Environment* env, + const char* main_script_source_utf8, + std::unique_ptr inspector_parent_handle) { + CHECK_NOT_NULL(main_script_source_utf8); + return LoadEnvironment( + env, + [&](const StartExecutionCallbackInfo& info) -> MaybeLocal { + // This is a slightly hacky way to convert UTF-8 to UTF-16. + Local str = + String::NewFromUtf8(env->isolate(), + main_script_source_utf8, + v8::NewStringType::kNormal).ToLocalChecked(); + auto main_utf16 = std::make_unique(env->isolate(), str); + + // TODO(addaleax): Avoid having a global table for all scripts. + std::string name = "embedder_main_" + std::to_string(env->thread_id()); + native_module::NativeModuleEnv::Add( + name.c_str(), + UnionBytes(**main_utf16, main_utf16->length())); + env->set_main_utf16(std::move(main_utf16)); + std::vector> params = { + env->process_string(), + env->require_string()}; + std::vector> args = { + env->process_object(), + env->native_module_require()}; + return ExecuteBootstrapper(env, name.c_str(), ¶ms, &args); + }, + std::move(inspector_parent_handle)); +} + Environment* GetCurrentEnvironment(Local context) { return Environment::GetCurrent(context); } diff --git a/src/env-inl.h b/src/env-inl.h index 598b84c07ca8d2..cb10b4d9554e23 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1216,6 +1216,11 @@ int64_t Environment::base_object_count() const { return base_object_count_; } +void Environment::set_main_utf16(std::unique_ptr str) { + CHECK(!main_utf16_); + main_utf16_ = std::move(str); +} + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) diff --git a/src/env.h b/src/env.h index 8331a79dbc9918..0566c2d26dc4f2 100644 --- a/src/env.h +++ b/src/env.h @@ -1269,6 +1269,8 @@ class Environment : public MemoryRetainer { void AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose( std::shared_ptr); + inline void set_main_utf16(std::unique_ptr); + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1435,6 +1437,11 @@ class Environment : public MemoryRetainer { #undef V v8::Global context_; + + // Keeps the main script source alive is one was passed to LoadEnvironment(). + // We should probably find a way to just use plain `v8::String`s created from + // the source passed to LoadEnvironment() directly instead. + std::unique_ptr main_utf16_; }; } // namespace node diff --git a/src/node.h b/src/node.h index e511579b346809..2e00d043c13ff6 100644 --- a/src/node.h +++ b/src/node.h @@ -432,6 +432,10 @@ NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, StartExecutionCallback cb, std::unique_ptr inspector_parent_handle = {}); +NODE_EXTERN v8::MaybeLocal LoadEnvironment( + Environment* env, + const char* main_script_source_utf8, + std::unique_ptr inspector_parent_handle = {}); NODE_EXTERN void FreeEnvironment(Environment* env); // This may return nullptr if context is not associated with a Node instance. diff --git a/src/node_native_module.cc b/src/node_native_module.cc index 7362207412efa4..74729c412674be 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -30,6 +30,14 @@ bool NativeModuleLoader::Exists(const char* id) { return source_.find(id) != source_.end(); } +bool NativeModuleLoader::Add(const char* id, const UnionBytes& source) { + if (Exists(id)) { + return false; + } + source_.emplace(id, source); + return true; +} + Local NativeModuleLoader::GetSourceObject(Local context) { Isolate* isolate = context->GetIsolate(); Local out = Object::New(isolate); diff --git a/src/node_native_module.h b/src/node_native_module.h index c0bce3bce42c84..3be3f2364dd252 100644 --- a/src/node_native_module.h +++ b/src/node_native_module.h @@ -47,6 +47,8 @@ class NativeModuleLoader { UnionBytes GetConfig(); // Return data for config.gypi bool Exists(const char* id); + bool Add(const char* id, const UnionBytes& source); + v8::Local GetSourceObject(v8::Local context); v8::Local GetConfigString(v8::Isolate* isolate); std::vector GetModuleIds(); diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc index 9a6ccf99313cf8..cdd98e8220eb46 100644 --- a/src/node_native_module_env.cc +++ b/src/node_native_module_env.cc @@ -32,6 +32,10 @@ Local ToJsSet(Local context, const std::set& in) { return out; } +bool NativeModuleEnv::Add(const char* id, const UnionBytes& source) { + return NativeModuleLoader::GetInstance()->Add(id, source); +} + bool NativeModuleEnv::Exists(const char* id) { return NativeModuleLoader::GetInstance()->Exists(id); } diff --git a/src/node_native_module_env.h b/src/node_native_module_env.h index f662c67be50d40..bc36be75109639 100644 --- a/src/node_native_module_env.h +++ b/src/node_native_module_env.h @@ -29,6 +29,7 @@ class NativeModuleEnv { // Returns config.gypi as a JSON string static v8::Local GetConfigString(v8::Isolate* isolate); static bool Exists(const char* id); + static bool Add(const char* id, const UnionBytes& source); // Loads data into NativeModuleLoader::.instance.code_cache_ // Generated by mkcodecache as node_code_cache.cc when diff --git a/src/node_worker.cc b/src/node_worker.cc index 846f4c82c4d26c..a61bf51f1c9a02 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -333,7 +333,7 @@ void Worker::Run() { CreateEnvMessagePort(env_.get()); Debug(this, "Created message port for worker %llu", thread_id_.id); if (LoadEnvironment(env_.get(), - nullptr, + StartExecutionCallback{}, std::move(inspector_parent_handle_)) .IsEmpty()) { return; diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index ae11f945d777a8..57308096f4dc22 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -80,6 +80,33 @@ TEST_F(EnvironmentTest, LoadEnvironmentWithCallback) { CHECK(called_cb); } +TEST_F(EnvironmentTest, LoadEnvironmentWithSource) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv}; + + v8::Local context = isolate_->GetCurrentContext(); + v8::Local main_ret = + node::LoadEnvironment(*env, + "return { process, require };").ToLocalChecked(); + + CHECK(main_ret->IsObject()); + CHECK(main_ret.As()->Get( + context, + v8::String::NewFromOneByte( + isolate_, + reinterpret_cast("process"), + v8::NewStringType::kNormal).ToLocalChecked()) + .ToLocalChecked()->IsObject()); + CHECK(main_ret.As()->Get( + context, + v8::String::NewFromOneByte( + isolate_, + reinterpret_cast("require"), + v8::NewStringType::kNormal).ToLocalChecked()) + .ToLocalChecked()->IsFunction()); +} + TEST_F(EnvironmentTest, AtExitWithEnvironment) { const v8::HandleScope handle_scope(isolate_); const Argv argv; From bb689cf55d18981a6883193e2623ba6dd7c9b9c7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 14:20:00 -0800 Subject: [PATCH 08/37] test: re-enable cctest that was commented out Refs: https://github.com/nodejs/node/pull/31910 PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- test/cctest/test_environment.cc | 39 ++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 57308096f4dc22..7f6dbb8d9d9cfd 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -32,24 +32,27 @@ class EnvironmentTest : public EnvironmentTestFixture { } }; -// TODO(codebytere): re-enable this test. -// TEST_F(EnvironmentTest, PreExeuctionPreparation) { -// const v8::HandleScope handle_scope(isolate_); -// const Argv argv; -// Env env {handle_scope, argv}; - -// v8::Local context = isolate_->GetCurrentContext(); - -// const char* run_script = "process.argv0"; -// v8::Local script = v8::Script::Compile( -// context, -// v8::String::NewFromOneByte(isolate_, -// reinterpret_cast(run_script), -// v8::NewStringType::kNormal).ToLocalChecked()) -// .ToLocalChecked(); -// v8::Local result = script->Run(context).ToLocalChecked(); -// CHECK(result->IsString()); -// } +TEST_F(EnvironmentTest, PreExecutionPreparation) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv}; + + node::LoadEnvironment(*env, [&](const node::StartExecutionCallbackInfo& info) + -> v8::MaybeLocal { + return v8::Null(isolate_); + }); + + v8::Local context = isolate_->GetCurrentContext(); + const char* run_script = "process.argv0"; + v8::Local script = v8::Script::Compile( + context, + v8::String::NewFromOneByte(isolate_, + reinterpret_cast(run_script), + v8::NewStringType::kNormal).ToLocalChecked()) + .ToLocalChecked(); + v8::Local result = script->Run(context).ToLocalChecked(); + CHECK(result->IsString()); +} TEST_F(EnvironmentTest, LoadEnvironmentWithCallback) { const v8::HandleScope handle_scope(isolate_); From ad95b853864a3be5e5d26ef0cf5f027634003acb Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 17:11:35 -0800 Subject: [PATCH 09/37] src: add unique_ptr equivalent of CreatePlatform This makes this bit of the embedder situation a bit easier to use. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 9 ++++++++- src/node.h | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index b43b43f1163b8d..79603ee726c6e7 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -477,13 +477,20 @@ MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform() { MultiIsolatePlatform* CreatePlatform( int thread_pool_size, node::tracing::TracingController* tracing_controller) { - return new NodePlatform(thread_pool_size, tracing_controller); + return MultiIsolatePlatform::Create(thread_pool_size, tracing_controller) + .release(); } void FreePlatform(MultiIsolatePlatform* platform) { delete platform; } +std::unique_ptr MultiIsolatePlatform::Create( + int thread_pool_size, + node::tracing::TracingController* tracing_controller) { + return std::make_unique(thread_pool_size, tracing_controller); +} + MaybeLocal GetPerContextExports(Local context) { Isolate* isolate = context->GetIsolate(); EscapableHandleScope handle_scope(isolate); diff --git a/src/node.h b/src/node.h index 2e00d043c13ff6..10a64744450bc8 100644 --- a/src/node.h +++ b/src/node.h @@ -298,6 +298,10 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { virtual void AddIsolateFinishedCallback(v8::Isolate* isolate, void (*callback)(void*), void* data) = 0; + + static std::unique_ptr Create( + int thread_pool_size, + node::tracing::TracingController* tracing_controller = nullptr); }; enum IsolateSettingsFlags { @@ -446,6 +450,7 @@ NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local context); // it returns nullptr. NODE_EXTERN MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform(); +// Legacy variants of MultiIsolatePlatform::Create(). NODE_EXTERN MultiIsolatePlatform* CreatePlatform( int thread_pool_size, node::tracing::TracingController* tracing_controller); From 33ecd3b92217b442044b8171321aaa84f5425261 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 17:40:54 -0800 Subject: [PATCH 10/37] src: make InitializeNodeWithArgs() official public API This is a decent replacement for the to-be-deprecated Init() API. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/node.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/node.h b/src/node.h index 10a64744450bc8..7a8709f7d624ea 100644 --- a/src/node.h +++ b/src/node.h @@ -223,10 +223,20 @@ NODE_EXTERN int Stop(Environment* env); // TODO(addaleax): Officially deprecate this and replace it with something // better suited for a public embedder API. +// It is recommended to use InitializeNodeWithArgs() instead as an embedder. +// Init() calls InitializeNodeWithArgs() and exits the process with the exit +// code returned from it. NODE_EXTERN void Init(int* argc, const char** argv, int* exec_argc, const char*** exec_argv); +// Set up per-process state needed to run Node.js. This will consume arguments +// from argv, fill exec_argv, and possibly add errors resulting from parsing +// the arguments to `errors`. The return value is a suggested exit code for the +// program; If it is 0, then initializing Node.js succeeded. +NODE_EXTERN int InitializeNodeWithArgs(std::vector* argv, + std::vector* exec_argv, + std::vector* errors); enum OptionEnvvarSettings { kAllowedInEnvironment, From 31528f34f29aa4076488bed82cfbd96f9ef15cb9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 21:13:59 -0800 Subject: [PATCH 11/37] src: add ability to look up platform based on `Environment*` This should eventually remove any necessity to use the global-state `GetMainThreadMultiIsolatePlatform()`. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 8 ++++++++ src/node.h | 7 ++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 79603ee726c6e7..21a79297a037e1 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -474,6 +474,14 @@ MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform() { return per_process::v8_platform.Platform(); } +MultiIsolatePlatform* GetMultiIsolatePlatform(Environment* env) { + return GetMultiIsolatePlatform(env->isolate_data()); +} + +MultiIsolatePlatform* GetMultiIsolatePlatform(IsolateData* env) { + return env->platform(); +} + MultiIsolatePlatform* CreatePlatform( int thread_pool_size, node::tracing::TracingController* tracing_controller) { diff --git a/src/node.h b/src/node.h index 7a8709f7d624ea..023e60dafd615e 100644 --- a/src/node.h +++ b/src/node.h @@ -456,9 +456,14 @@ NODE_EXTERN void FreeEnvironment(Environment* env); NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local context); // This returns the MultiIsolatePlatform used in the main thread of Node.js. -// If NODE_USE_V8_PLATFORM haven't been defined when Node.js was built, +// If NODE_USE_V8_PLATFORM has not been defined when Node.js was built, // it returns nullptr. +// TODO(addaleax): Deprecate in favour of GetMultiIsolatePlatform(). NODE_EXTERN MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform(); +// This returns the MultiIsolatePlatform used for an Environment or IsolateData +// instance, if one exists. +NODE_EXTERN MultiIsolatePlatform* GetMultiIsolatePlatform(Environment* env); +NODE_EXTERN MultiIsolatePlatform* GetMultiIsolatePlatform(IsolateData* env); // Legacy variants of MultiIsolatePlatform::Create(). NODE_EXTERN MultiIsolatePlatform* CreatePlatform( From eca25d045ec135fe48c6adcee9549d6463ea28bf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 21:43:02 -0800 Subject: [PATCH 12/37] src: allow non-Node.js TracingControllers We do not need a Node.js-provided `v8::TracingController`, generally. Loosen that restriction in order to make it easier for embedders to provide their own subclass of `v8::TracingController`, or none at all. Refs: https://github.com/electron/electron/commit/9c36576dddfaecde1298ff3e089d21a6e54fe67f PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 10 +++++++++- src/node.h | 6 +++++- src/node_platform.cc | 13 ++++++++----- src/node_platform.h | 6 +++--- src/tracing/agent.cc | 5 +++-- src/tracing/trace_event.cc | 10 ++++++++-- src/tracing/trace_event.h | 11 +++++++---- 7 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 21a79297a037e1..354a979c201e09 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -485,6 +485,14 @@ MultiIsolatePlatform* GetMultiIsolatePlatform(IsolateData* env) { MultiIsolatePlatform* CreatePlatform( int thread_pool_size, node::tracing::TracingController* tracing_controller) { + return CreatePlatform( + thread_pool_size, + static_cast(tracing_controller)); +} + +MultiIsolatePlatform* CreatePlatform( + int thread_pool_size, + v8::TracingController* tracing_controller) { return MultiIsolatePlatform::Create(thread_pool_size, tracing_controller) .release(); } @@ -495,7 +503,7 @@ void FreePlatform(MultiIsolatePlatform* platform) { std::unique_ptr MultiIsolatePlatform::Create( int thread_pool_size, - node::tracing::TracingController* tracing_controller) { + v8::TracingController* tracing_controller) { return std::make_unique(thread_pool_size, tracing_controller); } diff --git a/src/node.h b/src/node.h index 023e60dafd615e..cb3ff139278ecd 100644 --- a/src/node.h +++ b/src/node.h @@ -311,7 +311,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform { static std::unique_ptr Create( int thread_pool_size, - node::tracing::TracingController* tracing_controller = nullptr); + v8::TracingController* tracing_controller = nullptr); }; enum IsolateSettingsFlags { @@ -466,9 +466,13 @@ NODE_EXTERN MultiIsolatePlatform* GetMultiIsolatePlatform(Environment* env); NODE_EXTERN MultiIsolatePlatform* GetMultiIsolatePlatform(IsolateData* env); // Legacy variants of MultiIsolatePlatform::Create(). +// TODO(addaleax): Deprecate in favour of the v8::TracingController variant. NODE_EXTERN MultiIsolatePlatform* CreatePlatform( int thread_pool_size, node::tracing::TracingController* tracing_controller); +NODE_EXTERN MultiIsolatePlatform* CreatePlatform( + int thread_pool_size, + v8::TracingController* tracing_controller); NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform); NODE_EXTERN void EmitBeforeExit(Environment* env); diff --git a/src/node_platform.cc b/src/node_platform.cc index e6e880b87b5380..35444d45506823 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -13,7 +13,6 @@ using v8::Isolate; using v8::Object; using v8::Platform; using v8::Task; -using node::tracing::TracingController; namespace { @@ -320,12 +319,16 @@ void PerIsolatePlatformData::DecreaseHandleCount() { } NodePlatform::NodePlatform(int thread_pool_size, - TracingController* tracing_controller) { - if (tracing_controller) { + v8::TracingController* tracing_controller) { + if (tracing_controller != nullptr) { tracing_controller_ = tracing_controller; } else { - tracing_controller_ = new TracingController(); + tracing_controller_ = new v8::TracingController(); } + // TODO(addaleax): It's a bit icky that we use global state here, but we can't + // really do anything about it unless V8 starts exposing a way to access the + // current v8::Platform instance. + tracing::TraceEventHelper::SetTracingController(tracing_controller_); worker_thread_task_runner_ = std::make_shared(thread_pool_size); } @@ -490,7 +493,7 @@ double NodePlatform::CurrentClockTimeMillis() { return SystemClockTimeMillis(); } -TracingController* NodePlatform::GetTracingController() { +v8::TracingController* NodePlatform::GetTracingController() { CHECK_NOT_NULL(tracing_controller_); return tracing_controller_; } diff --git a/src/node_platform.h b/src/node_platform.h index 533ae1bcca8248..3cf526ae3da124 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -137,7 +137,7 @@ class WorkerThreadsTaskRunner { class NodePlatform : public MultiIsolatePlatform { public: NodePlatform(int thread_pool_size, - node::tracing::TracingController* tracing_controller); + v8::TracingController* tracing_controller); ~NodePlatform() override = default; void DrainTasks(v8::Isolate* isolate) override; @@ -159,7 +159,7 @@ class NodePlatform : public MultiIsolatePlatform { bool IdleTasksEnabled(v8::Isolate* isolate) override; double MonotonicallyIncreasingTime() override; double CurrentClockTimeMillis() override; - node::tracing::TracingController* GetTracingController() override; + v8::TracingController* GetTracingController() override; bool FlushForegroundTasks(v8::Isolate* isolate) override; void RegisterIsolate(v8::Isolate* isolate, uv_loop_t* loop) override; @@ -179,7 +179,7 @@ class NodePlatform : public MultiIsolatePlatform { std::unordered_map> per_isolate_; - node::tracing::TracingController* tracing_controller_; + v8::TracingController* tracing_controller_; std::shared_ptr worker_thread_task_runner_; }; diff --git a/src/tracing/agent.cc b/src/tracing/agent.cc index 7d265dcb0c4c3b..7ce59674356f97 100644 --- a/src/tracing/agent.cc +++ b/src/tracing/agent.cc @@ -242,8 +242,9 @@ void TracingController::AddMetadataEvent( TRACE_EVENT_FLAG_NONE, CurrentTimestampMicroseconds(), CurrentCpuTimestampMicroseconds()); - node::tracing::TraceEventHelper::GetAgent()->AddMetadataEvent( - std::move(trace_event)); + Agent* node_agent = node::tracing::TraceEventHelper::GetAgent(); + if (node_agent != nullptr) + node_agent->AddMetadataEvent(std::move(trace_event)); } } // namespace tracing diff --git a/src/tracing/trace_event.cc b/src/tracing/trace_event.cc index 9232c34c4ca322..da562c1bf7f8ff 100644 --- a/src/tracing/trace_event.cc +++ b/src/tracing/trace_event.cc @@ -4,17 +4,23 @@ namespace node { namespace tracing { Agent* g_agent = nullptr; +v8::TracingController* g_controller = nullptr; void TraceEventHelper::SetAgent(Agent* agent) { g_agent = agent; + g_controller = agent->GetTracingController(); } Agent* TraceEventHelper::GetAgent() { return g_agent; } -TracingController* TraceEventHelper::GetTracingController() { - return g_agent->GetTracingController(); +v8::TracingController* TraceEventHelper::GetTracingController() { + return g_controller; +} + +void TraceEventHelper::SetTracingController(v8::TracingController* controller) { + g_controller = controller; } } // namespace tracing diff --git a/src/tracing/trace_event.h b/src/tracing/trace_event.h index c11544dd42cdd0..2a79c5bc05b501 100644 --- a/src/tracing/trace_event.h +++ b/src/tracing/trace_event.h @@ -314,7 +314,9 @@ const uint64_t kNoId = 0; // Refs: https://github.com/nodejs/node/pull/28724 class NODE_EXTERN TraceEventHelper { public: - static TracingController* GetTracingController(); + static v8::TracingController* GetTracingController(); + static void SetTracingController(v8::TracingController* controller); + static Agent* GetAgent(); static void SetAgent(Agent* agent); @@ -514,9 +516,10 @@ static V8_INLINE void AddMetadataEventImpl( arg_convertibles[1].reset(reinterpret_cast( static_cast(arg_values[1]))); } - node::tracing::TracingController* controller = - node::tracing::TraceEventHelper::GetTracingController(); - return controller->AddMetadataEvent( + node::tracing::Agent* agent = + node::tracing::TraceEventHelper::GetAgent(); + if (agent == nullptr) return; + return agent->GetTracingController()->AddMetadataEvent( category_group_enabled, name, num_args, arg_names, arg_types, arg_values, arg_convertibles, flags); } From 691a5755936d127c36ce7703a43b89e3c4662cfe Mon Sep 17 00:00:00 2001 From: Jichan Date: Wed, 8 Jan 2020 21:36:05 +0900 Subject: [PATCH 13/37] src: fix what a dispose without checking If created platform with CreatePlatform, the crash occurs because it does not check if it was initialized to v8_platform when DisposePlatform was called. Refs: https://github.com/nodejs/node/pull/31260 Co-authored-by: Anna Henningsen PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/node_v8_platform-inl.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index 0f4a98a98551ba..ce6d5dd223d2cb 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -12,6 +12,7 @@ #include "tracing/node_trace_writer.h" #include "tracing/trace_event.h" #include "tracing/traced_value.h" +#include "util.h" namespace node { @@ -79,8 +80,12 @@ class NodeTraceStateObserver }; struct V8Platform { + bool initialized_ = false; + #if NODE_USE_V8_PLATFORM inline void Initialize(int thread_pool_size) { + CHECK(!initialized_); + initialized_ = true; tracing_agent_ = std::make_unique(); node::tracing::TraceEventHelper::SetAgent(tracing_agent_.get()); node::tracing::TracingController* controller = @@ -99,6 +104,10 @@ struct V8Platform { } inline void Dispose() { + if (!initialized_) + return; + initialized_ = false; + StopTracingAgent(); platform_->Shutdown(); delete platform_; From 1c5d55adb03a0161b363a9dc6441155bae452ca7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 22:02:12 -0800 Subject: [PATCH 14/37] src: shutdown platform from FreePlatform() There is currently no way to properly do this. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/node_platform.cc | 6 ++++++ src/node_platform.h | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/node_platform.cc b/src/node_platform.cc index 35444d45506823..f7eefc93f8fece 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -333,6 +333,10 @@ NodePlatform::NodePlatform(int thread_pool_size, std::make_shared(thread_pool_size); } +NodePlatform::~NodePlatform() { + Shutdown(); +} + void NodePlatform::RegisterIsolate(Isolate* isolate, uv_loop_t* loop) { Mutex::ScopedLock lock(per_isolate_mutex_); std::shared_ptr existing = per_isolate_[isolate]; @@ -362,6 +366,8 @@ void NodePlatform::AddIsolateFinishedCallback(Isolate* isolate, } void NodePlatform::Shutdown() { + if (has_shut_down_) return; + has_shut_down_ = true; worker_thread_task_runner_->Shutdown(); { diff --git a/src/node_platform.h b/src/node_platform.h index 3cf526ae3da124..48ced3eac5b5de 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -138,7 +138,7 @@ class NodePlatform : public MultiIsolatePlatform { public: NodePlatform(int thread_pool_size, v8::TracingController* tracing_controller); - ~NodePlatform() override = default; + ~NodePlatform() override; void DrainTasks(v8::Isolate* isolate) override; void Shutdown(); @@ -181,6 +181,7 @@ class NodePlatform : public MultiIsolatePlatform { v8::TracingController* tracing_controller_; std::shared_ptr worker_thread_task_runner_; + bool has_shut_down_ = false; }; } // namespace node From a02c5d058c3a64689225d4313dda7e2c8d2a9dc3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 18:54:13 -0800 Subject: [PATCH 15/37] src,test: add full-featured embedder API test PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- Makefile | 10 ++- node.gyp | 56 +++++++++++++++ src/node_code_cache_stub.cc | 4 ++ src/node_snapshot_stub.cc | 4 ++ test/embedding/embedtest.cc | 135 ++++++++++++++++++++++++++++++++++++ test/embedding/test.js | 19 +++++ 6 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 test/embedding/embedtest.cc create mode 100644 test/embedding/test.js diff --git a/Makefile b/Makefile index 68e222656eb980..35eeeb618b7097 100644 --- a/Makefile +++ b/Makefile @@ -212,6 +212,8 @@ coverage-clean: $(RM) out/$(BUILDTYPE)/obj.target/node/src/tracing/*.gcno $(RM) out/$(BUILDTYPE)/obj.target/cctest/src/*.gcno $(RM) out/$(BUILDTYPE)/obj.target/cctest/test/cctest/*.gcno + $(RM) out/$(BUILDTYPE)/obj.target/embedtest/src/*.gcno + $(RM) out/$(BUILDTYPE)/obj.target/embedtest/test/embedding/*.gcno .PHONY: coverage # Build and test with code coverage reporting. Leave the lib directory @@ -250,8 +252,8 @@ coverage-test: coverage-build TEST_CI_ARGS="$(TEST_CI_ARGS) --type=coverage" $(MAKE) $(COVTESTS) $(MAKE) coverage-report-js -(cd out && "../gcovr/scripts/gcovr" \ - --gcov-exclude='.*\b(deps|usr|out|cctest)\b' -v -r Release/obj.target \ - --html --html-detail -o ../coverage/cxxcoverage.html \ + --gcov-exclude='.*\b(deps|usr|out|cctest|embedding)\b' -v \ + -r Release/obj.target --html --html-detail -o ../coverage/cxxcoverage.html \ --gcov-executable="$(GCOV)") @echo -n "Javascript coverage %: " @grep -B1 Lines coverage/index.html | head -n1 \ @@ -276,6 +278,7 @@ coverage-report-js: # Runs the C++ tests using the built `cctest` executable. cctest: all @out/$(BUILDTYPE)/$@ --gtest_filter=$(GTEST_FILTER) + @out/$(BUILDTYPE)/embedtest "require('./test/embedding/test.js')" .PHONY: list-gtests list-gtests: @@ -531,6 +534,7 @@ test-ci: | clear-stalled bench-addons-build build-addons build-js-native-api-tes $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC) + out/Release/embedtest 'require("./test/embedding/test.js")' @echo "Clean up any leftover processes, error if found." ps awwx | grep Release/node | grep -v grep | cat @PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \ @@ -1274,6 +1278,8 @@ LINT_CPP_FILES = $(filter-out $(LINT_CPP_EXCLUDE), $(wildcard \ test/addons/*/*.h \ test/cctest/*.cc \ test/cctest/*.h \ + test/embedding/*.cc \ + test/embedding/*.h \ test/js-native-api/*/*.cc \ test/js-native-api/*/*.h \ test/node-api/*/*.cc \ diff --git a/node.gyp b/node.gyp index 8173a1afdda8ee..c03623846a2b6e 100644 --- a/node.gyp +++ b/node.gyp @@ -1236,6 +1236,62 @@ ], }, # cctest + { + 'target_name': 'embedtest', + 'type': 'executable', + + 'dependencies': [ + '<(node_lib_target_name)', + 'deps/histogram/histogram.gyp:histogram', + 'deps/uvwasi/uvwasi.gyp:uvwasi', + 'node_dtrace_header', + 'node_dtrace_ustack', + 'node_dtrace_provider', + ], + + 'includes': [ + 'node.gypi' + ], + + 'include_dirs': [ + 'src', + 'tools/msvs/genfiles', + 'deps/v8/include', + 'deps/cares/include', + 'deps/uv/include', + 'deps/uvwasi/include', + 'test/embedding', + ], + + 'sources': [ + 'src/node_snapshot_stub.cc', + 'src/node_code_cache_stub.cc', + 'test/embedding/embedtest.cc', + ], + + 'conditions': [ + ['OS=="solaris"', { + 'ldflags': [ '-I<(SHARED_INTERMEDIATE_DIR)' ] + }], + # Skip cctest while building shared lib node for Windows + [ 'OS=="win" and node_shared=="true"', { + 'type': 'none', + }], + [ 'node_shared=="true"', { + 'xcode_settings': { + 'OTHER_LDFLAGS': [ '-Wl,-rpath,@loader_path', ], + }, + }], + ['OS=="win"', { + 'libraries': [ + 'Dbghelp.lib', + 'winmm.lib', + 'Ws2_32.lib', + ], + }], + ], + }, # embedtest + # TODO(joyeecheung): do not depend on node_lib, # instead create a smaller static library node_lib_base that does # just enough for node_native_module.cc and the cache builder to diff --git a/src/node_code_cache_stub.cc b/src/node_code_cache_stub.cc index 3851508170f812..9d9901738f63b0 100644 --- a/src/node_code_cache_stub.cc +++ b/src/node_code_cache_stub.cc @@ -1,3 +1,7 @@ +// This file is part of the embedder test, which is intentionally built without +// NODE_WANT_INTERNALS, so we define it here manually. +#define NODE_WANT_INTERNALS 1 + #include "node_native_module_env.h" // The stub here is used when configure is run without `--code-cache-path`. diff --git a/src/node_snapshot_stub.cc b/src/node_snapshot_stub.cc index 91bc37121d61fe..fac03b0c87af5d 100644 --- a/src/node_snapshot_stub.cc +++ b/src/node_snapshot_stub.cc @@ -1,3 +1,7 @@ +// This file is part of the embedder test, which is intentionally built without +// NODE_WANT_INTERNALS, so we define it here manually. +#define NODE_WANT_INTERNALS 1 + #include "node_main_instance.h" namespace node { diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc new file mode 100644 index 00000000000000..14e2dfb802d22c --- /dev/null +++ b/test/embedding/embedtest.cc @@ -0,0 +1,135 @@ +#include "node.h" +#include "uv.h" +#include + +// Note: This file is being referred to from doc/api/embedding.md, and excerpts +// from it are included in the documentation. Try to keep these in sync. + +using node::ArrayBufferAllocator; +using node::Environment; +using node::IsolateData; +using node::MultiIsolatePlatform; +using v8::Context; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Locker; +using v8::MaybeLocal; +using v8::SealHandleScope; +using v8::Value; +using v8::V8; + +static int RunNodeInstance(MultiIsolatePlatform* platform, + const std::vector& args, + const std::vector& exec_args); + +int main(int argc, char** argv) { + std::vector args(argv, argv + argc); + std::vector exec_args; + std::vector errors; + int exit_code = node::InitializeNodeWithArgs(&args, &exec_args, &errors); + for (const std::string& error : errors) + fprintf(stderr, "%s: %s\n", args[0].c_str(), error.c_str()); + if (exit_code != 0) { + return exit_code; + } + + std::unique_ptr platform = + MultiIsolatePlatform::Create(4); + V8::InitializePlatform(platform.get()); + V8::Initialize(); + + int ret = RunNodeInstance(platform.get(), args, exec_args); + + V8::Dispose(); + V8::ShutdownPlatform(); + return ret; +} + +int RunNodeInstance(MultiIsolatePlatform* platform, + const std::vector& args, + const std::vector& exec_args) { + int exit_code = 0; + uv_loop_t loop; + int ret = uv_loop_init(&loop); + if (ret != 0) { + fprintf(stderr, "%s: Failed to initialize loop: %s\n", + args[0].c_str(), + uv_err_name(ret)); + return 1; + } + + std::shared_ptr allocator = + ArrayBufferAllocator::Create(); + + Isolate* isolate = NewIsolate(allocator, &loop, platform); + if (isolate == nullptr) { + fprintf(stderr, "%s: Failed to initialize V8 Isolate\n", args[0].c_str()); + return 1; + } + + { + Locker locker(isolate); + Isolate::Scope isolate_scope(isolate); + + std::unique_ptr isolate_data( + node::CreateIsolateData(isolate, &loop, platform, allocator.get()), + node::FreeIsolateData); + + HandleScope handle_scope(isolate); + Local context = node::NewContext(isolate); + if (context.IsEmpty()) { + fprintf(stderr, "%s: Failed to initialize V8 Context\n", args[0].c_str()); + return 1; + } + + Context::Scope context_scope(context); + std::unique_ptr env( + node::CreateEnvironment(isolate_data.get(), context, args, exec_args), + node::FreeEnvironment); + + MaybeLocal loadenv_ret = node::LoadEnvironment( + env.get(), + "const publicRequire =" + " require('module').createRequire(process.cwd() + '/');" + "globalThis.require = publicRequire;" + "require('vm').runInThisContext(process.argv[1]);"); + + if (loadenv_ret.IsEmpty()) // There has been a JS exception. + return 1; + + { + SealHandleScope seal(isolate); + bool more; + do { + uv_run(&loop, UV_RUN_DEFAULT); + + platform->DrainTasks(isolate); + more = uv_loop_alive(&loop); + if (more) continue; + + node::EmitBeforeExit(env.get()); + more = uv_loop_alive(&loop); + } while (more == true); + } + + exit_code = node::EmitExit(env.get()); + + node::Stop(env.get()); + } + + bool platform_finished = false; + platform->AddIsolateFinishedCallback(isolate, [](void* data) { + *static_cast(data) = true; + }, &platform_finished); + platform->UnregisterIsolate(isolate); + isolate->Dispose(); + + // Wait until the platform has cleaned up all relevant resources. + while (!platform_finished) + uv_run(&loop, UV_RUN_ONCE); + int err = uv_loop_close(&loop); + assert(err == 0); + + return exit_code; +} diff --git a/test/embedding/test.js b/test/embedding/test.js new file mode 100644 index 00000000000000..a802de1849021f --- /dev/null +++ b/test/embedding/test.js @@ -0,0 +1,19 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const child_process = require('child_process'); + +common.allowGlobals(global.require); + +assert.strictEqual( + child_process.spawnSync(process.execPath, ['console.log(42)']) + .stdout.toString().trim(), + '42'); + +assert.strictEqual( + child_process.spawnSync(process.execPath, ['throw new Error()']).status, + 1); + +assert.strictEqual( + child_process.spawnSync(process.execPath, ['process.exitCode = 8']).status, + 8); From 0547551b435742a38b80f5c3887b532d1637749b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 23:14:33 -0800 Subject: [PATCH 16/37] doc: add basic embedding example documentation PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- doc/api/embedding.md | 226 +++++++++++++++++++++++++++++++++++++++++++ doc/api/index.md | 7 +- 2 files changed, 230 insertions(+), 3 deletions(-) create mode 100644 doc/api/embedding.md diff --git a/doc/api/embedding.md b/doc/api/embedding.md new file mode 100644 index 00000000000000..7db24b325ec7ef --- /dev/null +++ b/doc/api/embedding.md @@ -0,0 +1,226 @@ +# C++ Embedder API + + + +Node.js provides a number of C++ APIs that can be used to execute JavaScript +in a Node.js environment from other C++ software. + +The documentation for these APIs can be found in [src/node.h][] in the Node.js +source tree. In addition to the APIs exposed by Node.js, some required concepts +are provided by the V8 embedder API. + +Because using Node.js as an embedded library is different from writing code +that is executed by Node.js, breaking changes do not follow typical Node.js +[deprecation policy][] and may occur on each semver-major release without prior +warning. + +## Example embedding application + +The following sections will provide an overview over how to use these APIs +to create an application from scratch that will perform the equivalent of +`node -e `, i.e. that will take a piece of JavaScript and run it in +a Node.js-specific environment. + +The full code can be found [in the Node.js source tree][embedtest.cc]. + +### Setting up per-process state + +Node.js requires some per-process state management in order to run: + +* Arguments parsing for Node.js [CLI options][], +* V8 per-process requirements, such as a `v8::Platform` instance. + +The following example shows how these can be set up. Some class names are from +the `node` and `v8` C++ namespaces, respectively. + +```c++ +int main(int argc, char** argv) { + std::vector args(argv, argv + argc); + std::vector exec_args; + std::vector errors; + // Parse Node.js CLI options, and print any errors that have occurred while + // trying to parse them. + int exit_code = node::InitializeNodeWithArgs(&args, &exec_args, &errors); + for (const std::string& error : errors) + fprintf(stderr, "%s: %s\n", args[0].c_str(), error.c_str()); + if (exit_code != 0) { + return exit_code; + } + + // Create a v8::Platform instance. `MultiIsolatePlatform::Create()` is a way + // to create a v8::Platform instance that Node.js can use when creating + // Worker threads. When no `MultiIsolatePlatform` instance is present, + // Worker threads are disabled. + std::unique_ptr platform = + MultiIsolatePlatform::Create(4); + V8::InitializePlatform(platform.get()); + V8::Initialize(); + + // See below for the contents of this function. + int ret = RunNodeInstance(platform.get(), args, exec_args); + + V8::Dispose(); + V8::ShutdownPlatform(); + return ret; +} +``` + +### Per-instance state + +Node.js has a concept of a “Node.js instance”, that is commonly being referred +to as `node::Environment`. Each `node::Environment` is associated with: + +* Exactly one `v8::Isolate`, i.e. one JS Engine instance, +* Exactly one `uv_loop_t`, i.e. one event loop, and +* A number of `v8::Context`s, but exactly one main `v8::Context`. +* One `node::IsolateData` instance that contains information that could be + shared by multiple `node::Environment`s that use the same `v8::Isolate`. + Currently, no testing if performed for this scenario. + +In order to set up a `v8::Isolate`, an `v8::ArrayBuffer::Allocator` needs +to be provided. One possible choice is the default Node.js allocator, which +can be created through `node::ArrayBufferAllocator::Create()`. Using the Node.js +allocator allows minor performance optimizations when addons use the Node.js +C++ `Buffer` API, and is required in order to track `ArrayBuffer` memory in +[`process.memoryUsage()`][]. + +Additionally, each `v8::Isolate` that is used for a Node.js instance needs to +be registered and unregistered with the `MultiIsolatePlatform` instance, if one +is being used, in order for the platform to know which event loop to use +for tasks scheduled by the `v8::Isolate`. + +The `node::NewIsolate()` helper function creates a `v8::Isolate`, +sets it up with some Node.js-specific hooks (e.g. the Node.js error handler), +and registers it with the platform automatically. + +```c++ +int RunNodeInstance(MultiIsolatePlatform* platform, + const std::vector& args, + const std::vector& exec_args) { + int exit_code = 0; + // Set up a libuv event loop. + uv_loop_t loop; + int ret = uv_loop_init(&loop); + if (ret != 0) { + fprintf(stderr, "%s: Failed to initialize loop: %s\n", + args[0].c_str(), + uv_err_name(ret)); + return 1; + } + + std::shared_ptr allocator = + ArrayBufferAllocator::Create(); + + Isolate* isolate = NewIsolate(allocator, &loop, platform); + if (isolate == nullptr) { + fprintf(stderr, "%s: Failed to initialize V8 Isolate\n", args[0].c_str()); + return 1; + } + + { + Locker locker(isolate); + Isolate::Scope isolate_scope(isolate); + + // Create a node::IsolateData instance that will later be released using + // node::FreeIsolateData(). + std::unique_ptr isolate_data( + node::CreateIsolateData(isolate, &loop, platform, allocator.get()), + node::FreeIsolateData); + + // Set up a new v8::Context. + HandleScope handle_scope(isolate); + Local context = node::NewContext(isolate); + if (context.IsEmpty()) { + fprintf(stderr, "%s: Failed to initialize V8 Context\n", args[0].c_str()); + return 1; + } + + // The v8::Context needs to be entered when node::CreateEnvironment() and + // node::LoadEnvironment() are being called. + Context::Scope context_scope(context); + + // Create a node::Environment instance that will later be released using + // node::FreeEnvironment(). + std::unique_ptr env( + node::CreateEnvironment(isolate_data.get(), context, args, exec_args), + node::FreeEnvironment); + + // Set up the Node.js instance for execution, and run code inside of it. + // There is also a variant that takes a callback and provides it with + // the `require` and `process` objects, so that it can manually compile + // and run scripts as needed. + // The `require` function inside this script does *not* access the file + // system, and can only load built-in Node.js modules. + // `module.createRequire()` is being used to create one that is able to + // load files from the disk, and uses the standard CommonJS file loader + // instead of the internal-only `require` function. + MaybeLocal loadenv_ret = node::LoadEnvironment( + env.get(), + "const publicRequire =" + " require('module').createRequire(process.cwd() + '/');" + "globalThis.require = publicRequire;" + "require('vm').runInThisContext(process.argv[1]);"); + + if (loadenv_ret.IsEmpty()) // There has been a JS exception. + return 1; + + { + // SealHandleScope protects against handle leaks from callbacks. + SealHandleScope seal(isolate); + bool more; + do { + uv_run(&loop, UV_RUN_DEFAULT); + + // V8 tasks on background threads may end up scheduling new tasks in the + // foreground, which in turn can keep the event loop going. For example, + // WebAssembly.compile() may do so. + platform->DrainTasks(isolate); + + // If there are new tasks, continue. + more = uv_loop_alive(&loop); + if (more) continue; + + // node::EmitBeforeExit() is used to emit the 'beforeExit' event on + // the `process` object. + node::EmitBeforeExit(env.get()); + + // 'beforeExit' can also schedule new work that keeps the event loop + // running. + more = uv_loop_alive(&loop); + } while (more == true); + } + + // node::EmitExit() returns the current exit code. + exit_code = node::EmitExit(env.get()); + + // node::Stop() can be used to explicitly stop the event loop and keep + // further JavaScript from running. It can be called from any thread, + // and will act like worker.terminate() if called from another thread. + node::Stop(env.get()); + } + + // Unregister the Isolate with the platform and add a listener that is called + // when the Platform is done cleaning up any state it had associated with + // the Isolate. + bool platform_finished = false; + platform->AddIsolateFinishedCallback(isolate, [](void* data) { + *static_cast(data) = true; + }, &platform_finished); + platform->UnregisterIsolate(isolate); + isolate->Dispose(); + + // Wait until the platform has cleaned up all relevant resources. + while (!platform_finished) + uv_run(&loop, UV_RUN_ONCE); + int err = uv_loop_close(&loop); + assert(err == 0); + + return exit_code; +} +``` + +[`process.memoryUsage()`]: process.html#process_process_memoryusage +[CLI options]: cli.html +[deprecation policy]: deprecations.html +[embedtest.cc]: https://github.com/nodejs/node/blob/master/test/embedding/embedtest.cc +[src/node.h]: https://github.com/nodejs/node/blob/master/src/node.h diff --git a/doc/api/index.md b/doc/api/index.md index 841e0f3804e6f8..a6d54e1601c152 100644 --- a/doc/api/index.md +++ b/doc/api/index.md @@ -13,9 +13,10 @@ * [Assertion testing](assert.html) * [Async hooks](async_hooks.html) * [Buffer](buffer.html) -* [C++ addons](addons.html) -* [C/C++ addons with N-API](n-api.html) -* [Child processes](child_process.html) +* [C++ Addons](addons.html) +* [C/C++ Addons with N-API](n-api.html) +* [C++ Embedder API](embedding.html) +* [Child Processes](child_process.html) * [Cluster](cluster.html) * [Command line options](cli.html) * [Console](console.html) From a7ada43e83865f5f5f2b84913fd42d42625b61e2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Mar 2020 13:11:48 +0100 Subject: [PATCH 17/37] test: add extended embedder cctest Add an embedder cctest that also covers a multi-Environment situation, including worker_threads-style inspector support. Co-authored-by: Joyee Cheung PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- Makefile | 4 +- test/cctest/test_environment.cc | 141 +++++++++++++++++++++++++++++++ test/embedding/test-embedding.js | 33 ++++++++ test/embedding/test.js | 19 ----- test/embedding/testcfg.py | 6 ++ tools/test.py | 1 + 6 files changed, 183 insertions(+), 21 deletions(-) create mode 100644 test/embedding/test-embedding.js delete mode 100644 test/embedding/test.js create mode 100644 test/embedding/testcfg.py diff --git a/Makefile b/Makefile index 35eeeb618b7097..c8731b5f4da534 100644 --- a/Makefile +++ b/Makefile @@ -278,7 +278,7 @@ coverage-report-js: # Runs the C++ tests using the built `cctest` executable. cctest: all @out/$(BUILDTYPE)/$@ --gtest_filter=$(GTEST_FILTER) - @out/$(BUILDTYPE)/embedtest "require('./test/embedding/test.js')" + @out/$(BUILDTYPE)/embedtest "require('./test/embedding/test-embedding.js')" .PHONY: list-gtests list-gtests: @@ -534,7 +534,7 @@ test-ci: | clear-stalled bench-addons-build build-addons build-js-native-api-tes $(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \ --mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \ $(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC) - out/Release/embedtest 'require("./test/embedding/test.js")' + out/Release/embedtest 'require("./test/embedding/test-embedding.js")' @echo "Clean up any leftover processes, error if found." ps awwx | grep Release/node | grep -v grep | cat @PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \ diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 7f6dbb8d9d9cfd..86688e303570da 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -329,3 +329,144 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { EXPECT_EQ(called, 1); EXPECT_EQ(called_unref, 0); } + +#if HAVE_INSPECTOR +TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { + // Tests that child Environments can be created through the public API + // that are accessible by the inspector. + // This test sets a global variable in the child Environment, and reads it + // back both through the inspector and inside the child Environment, and + // makes sure that those correspond to the value that was originally set. + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv}; + + v8::Local context = isolate_->GetCurrentContext(); + node::LoadEnvironment(*env, + "'use strict';\n" + "const { Worker } = require('worker_threads');\n" + "const { Session } = require('inspector');\n" + + "const session = new Session();\n" + "session.connect();\n" + "session.on('NodeWorker.attachedToWorker', (\n" + " ({ params: { workerInfo, sessionId } }) => {\n" + " session.post('NodeWorker.sendMessageToWorker', {\n" + " sessionId,\n" + " message: JSON.stringify({\n" + " id: 1,\n" + " method: 'Runtime.evaluate',\n" + " params: {\n" + " expression: 'global.variableFromParent = 42;'\n" + " }\n" + " })\n" + " });\n" + " session.on('NodeWorker.receivedMessageFromWorker',\n" + " ({ params: { message } }) => {\n" + " global.messageFromWorker = \n" + " JSON.parse(message).result.result.value;\n" + " });\n" + " }));\n" + "session.post('NodeWorker.enable', { waitForDebuggerOnStart: false });\n") + .ToLocalChecked(); + + struct ChildEnvironmentData { + node::ThreadId thread_id; + std::unique_ptr inspector_parent_handle; + node::MultiIsolatePlatform* platform; + int32_t extracted_value = -1; + uv_async_t thread_stopped_async; + }; + + ChildEnvironmentData data; + data.thread_id = node::AllocateEnvironmentThreadId(); + data.inspector_parent_handle = + GetInspectorParentHandle(*env, data.thread_id, "file:///embedded.js"); + CHECK(data.inspector_parent_handle); + data.platform = GetMultiIsolatePlatform(*env); + CHECK_NOT_NULL(data.platform); + + bool thread_stopped = false; + int err = uv_async_init( + ¤t_loop, &data.thread_stopped_async, [](uv_async_t* async) { + *static_cast(async->data) = true; + uv_close(reinterpret_cast(async), nullptr); + }); + CHECK_EQ(err, 0); + data.thread_stopped_async.data = &thread_stopped; + + uv_thread_t thread; + err = uv_thread_create(&thread, [](void* arg) { + ChildEnvironmentData* data = static_cast(arg); + std::shared_ptr aba = + node::ArrayBufferAllocator::Create(); + uv_loop_t loop; + uv_loop_init(&loop); + v8::Isolate* isolate = NewIsolate(aba, &loop, data->platform); + CHECK_NOT_NULL(isolate); + + { + v8::Isolate::Scope isolate_scope(isolate); + v8::HandleScope handle_scope(isolate); + + v8::Local context = node::NewContext(isolate); + CHECK(!context.IsEmpty()); + v8::Context::Scope context_scope(context); + + node::IsolateData* isolate_data = node::CreateIsolateData( + isolate, + &loop, + data->platform); + CHECK_NOT_NULL(isolate_data); + node::Environment* environment = node::CreateEnvironment( + isolate_data, + context, + { "dummy" }, + {}, + node::EnvironmentFlags::kNoFlags, + data->thread_id); + CHECK_NOT_NULL(environment); + + v8::Local extracted_value = LoadEnvironment( + environment, + "return global.variableFromParent;", + std::move(data->inspector_parent_handle)).ToLocalChecked(); + + uv_run(&loop, UV_RUN_DEFAULT); + CHECK(extracted_value->IsInt32()); + data->extracted_value = extracted_value.As()->Value(); + + node::FreeEnvironment(environment); + node::FreeIsolateData(isolate_data); + } + + data->platform->UnregisterIsolate(isolate); + isolate->Dispose(); + uv_run(&loop, UV_RUN_DEFAULT); + CHECK_EQ(uv_loop_close(&loop), 0); + + uv_async_send(&data->thread_stopped_async); + }, &data); + CHECK_EQ(err, 0); + + bool more; + do { + uv_run(¤t_loop, UV_RUN_DEFAULT); + data.platform->DrainTasks(isolate_); + more = uv_loop_alive(¤t_loop); + } while (!thread_stopped || more); + + uv_thread_join(&thread); + + v8::Local from_inspector = + context->Global()->Get( + context, + v8::String::NewFromOneByte( + isolate_, + reinterpret_cast("messageFromWorker"), + v8::NewStringType::kNormal).ToLocalChecked()) + .ToLocalChecked(); + CHECK_EQ(data.extracted_value, 42); + CHECK_EQ(from_inspector->IntegerValue(context).FromJust(), 42); +} +#endif // HAVE_INSPECTOR diff --git a/test/embedding/test-embedding.js b/test/embedding/test-embedding.js new file mode 100644 index 00000000000000..43dab0ab24ea53 --- /dev/null +++ b/test/embedding/test-embedding.js @@ -0,0 +1,33 @@ +'use strict'; +const common = require('../common'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); +const child_process = require('child_process'); +const path = require('path'); + +common.allowGlobals(global.require); +let binary = process.features.debug ? + 'out/Debug/embedtest' : 'out/Release/embedtest'; +if (common.isWindows) { + binary += '.exe'; +} +binary = path.resolve(__dirname, '..', '..', binary); + +assert.strictEqual( + child_process.spawnSync(binary, ['console.log(42)']) + .stdout.toString().trim(), + '42'); + +assert.strictEqual( + child_process.spawnSync(binary, ['throw new Error()']).status, + 1); + +assert.strictEqual( + child_process.spawnSync(binary, ['process.exitCode = 8']).status, + 8); + + +const fixturePath = JSON.stringify(fixtures.path('exit.js')); +assert.strictEqual( + child_process.spawnSync(binary, [`require(${fixturePath})`, 92]).status, + 92); diff --git a/test/embedding/test.js b/test/embedding/test.js deleted file mode 100644 index a802de1849021f..00000000000000 --- a/test/embedding/test.js +++ /dev/null @@ -1,19 +0,0 @@ -'use strict'; -const common = require('../common'); -const assert = require('assert'); -const child_process = require('child_process'); - -common.allowGlobals(global.require); - -assert.strictEqual( - child_process.spawnSync(process.execPath, ['console.log(42)']) - .stdout.toString().trim(), - '42'); - -assert.strictEqual( - child_process.spawnSync(process.execPath, ['throw new Error()']).status, - 1); - -assert.strictEqual( - child_process.spawnSync(process.execPath, ['process.exitCode = 8']).status, - 8); diff --git a/test/embedding/testcfg.py b/test/embedding/testcfg.py new file mode 100644 index 00000000000000..a4b90f490c670f --- /dev/null +++ b/test/embedding/testcfg.py @@ -0,0 +1,6 @@ +import sys, os +sys.path.append(os.path.join(os.path.dirname(__file__), '..')) +import testpy + +def GetConfiguration(context, root): + return testpy.SimpleTestConfiguration(context, root, 'embedding') diff --git a/tools/test.py b/tools/test.py index 811db910608812..532b855479b7bb 100755 --- a/tools/test.py +++ b/tools/test.py @@ -1486,6 +1486,7 @@ def PrintCrashed(code): 'addons', 'benchmark', 'doctool', + 'embedding', 'internet', 'js-native-api', 'node-api', From 1a8e9eec8369696cf4c8cb3f5a491780244b3ff5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 30 Mar 2020 11:01:07 +0200 Subject: [PATCH 18/37] test: wait for message from parent in embedding cctest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’ve seen this fail a few times in CI, presumably because the inspector commmand did not reach the child thread in time. Explicitly waiting seems to solve that. Refs: https://github.com/nodejs/node/pull/30467 PR-URL: https://github.com/nodejs/node/pull/32563 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- test/cctest/test_environment.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 86688e303570da..5857a815f27692 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -429,6 +429,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { v8::Local extracted_value = LoadEnvironment( environment, + "while (!global.variableFromParent) {}\n" "return global.variableFromParent;", std::move(data->inspector_parent_handle)).ToLocalChecked(); From 6d231dbdc0dfa1990786d5c326d56a3c478edcdf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 28 Mar 2020 02:46:41 +0100 Subject: [PATCH 19/37] embedding: provide hook for custom process.exit() behaviour Embedders may not want to terminate the process when `process.exit()` is called. This provides a hook for more flexible handling of that situation. Refs: https://github.com/nodejs/node/pull/30467#issuecomment-603689644 PR-URL: https://github.com/nodejs/node/pull/32531 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell --- src/api/environment.cc | 13 +++++++++++++ src/env-inl.h | 5 +++++ src/env.cc | 9 +-------- src/env.h | 5 +++++ src/node.h | 12 ++++++++++++ src/node_worker.cc | 10 +++++++--- test/cctest/test_environment.cc | 17 +++++++++++++++++ 7 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 354a979c201e09..cd05b3e4794a1a 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -711,4 +711,17 @@ ThreadId AllocateEnvironmentThreadId() { return ThreadId { next_thread_id++ }; } +void DefaultProcessExitHandler(Environment* env, int exit_code) { + env->set_can_call_into_js(false); + env->stop_sub_worker_contexts(); + DisposePlatform(); + exit(exit_code); +} + + +void SetProcessExitHandler(Environment* env, + std::function&& handler) { + env->set_process_exit_handler(std::move(handler)); +} + } // namespace node diff --git a/src/env-inl.h b/src/env-inl.h index cb10b4d9554e23..0e3b2842e4d1ad 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1221,6 +1221,11 @@ void Environment::set_main_utf16(std::unique_ptr str) { main_utf16_ = std::move(str); } +void Environment::set_process_exit_handler( + std::function&& handler) { + process_exit_handler_ = std::move(handler); +} + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) diff --git a/src/env.cc b/src/env.cc index f0b8b521f70363..b2e4721b96db1b 100644 --- a/src/env.cc +++ b/src/env.cc @@ -987,14 +987,7 @@ void Environment::Exit(int exit_code) { StackTrace::CurrentStackTrace( isolate(), stack_trace_limit(), StackTrace::kDetailed)); } - if (is_main_thread()) { - set_can_call_into_js(false); - stop_sub_worker_contexts(); - DisposePlatform(); - exit(exit_code); - } else { - worker_context()->Exit(exit_code); - } + process_exit_handler_(this, exit_code); } void Environment::stop_sub_worker_contexts() { diff --git a/src/env.h b/src/env.h index 0566c2d26dc4f2..13c562133ce44a 100644 --- a/src/env.h +++ b/src/env.h @@ -1270,6 +1270,8 @@ class Environment : public MemoryRetainer { std::shared_ptr); inline void set_main_utf16(std::unique_ptr); + inline void set_process_exit_handler( + std::function&& handler); private: inline void ThrowError(v8::Local (*fun)(v8::Local), @@ -1428,6 +1430,9 @@ class Environment : public MemoryRetainer { ArrayBufferAllocatorList; ArrayBufferAllocatorList* keep_alive_allocators_ = nullptr; + std::function process_exit_handler_ { + DefaultProcessExitHandler }; + template void ForEachBaseObject(T&& iterator); diff --git a/src/node.h b/src/node.h index cb3ff139278ecd..74fe59097ff673 100644 --- a/src/node.h +++ b/src/node.h @@ -452,6 +452,18 @@ NODE_EXTERN v8::MaybeLocal LoadEnvironment( std::unique_ptr inspector_parent_handle = {}); NODE_EXTERN void FreeEnvironment(Environment* env); +// Set a callback that is called when process.exit() is called from JS, +// overriding the default handler. +// It receives the Environment* instance and the exit code as arguments. +// This could e.g. call Stop(env); in order to terminate execution and stop +// the event loop. +// The default handler disposes of the global V8 platform instance, if one is +// being used, and calls exit(). +NODE_EXTERN void SetProcessExitHandler( + Environment* env, + std::function&& handler); +NODE_EXTERN void DefaultProcessExitHandler(Environment* env, int exit_code); + // This may return nullptr if context is not associated with a Node instance. NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local context); diff --git a/src/node_worker.cc b/src/node_worker.cc index a61bf51f1c9a02..afa4a9a52d0192 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -321,6 +321,9 @@ void Worker::Run() { if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); + SetProcessExitHandler(env_.get(), [this](Environment*, int exit_code) { + Exit(exit_code); + }); } { Mutex::ScopedLock lock(mutex_); @@ -430,9 +433,10 @@ void Worker::JoinThread() { MakeCallback(env()->onexit_string(), arraysize(args), args); } - // We cleared all libuv handles bound to this Worker above, - // the C++ object is no longer needed for anything now. - MakeWeak(); + // If we get here, the !thread_joined_ condition at the top of the function + // implies that the thread was running. In that case, its final action will + // be to schedule a callback on the parent thread which will delete this + // object, so there's nothing more to do here. } Worker::~Worker() { diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 5857a815f27692..b3a02de3726372 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -471,3 +471,20 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { CHECK_EQ(from_inspector->IntegerValue(context).FromJust(), 42); } #endif // HAVE_INSPECTOR + +TEST_F(EnvironmentTest, ExitHandlerTest) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + + int callback_calls = 0; + + Env env {handle_scope, argv}; + SetProcessExitHandler(*env, [&](node::Environment* env_, int exit_code) { + EXPECT_EQ(*env, env_); + EXPECT_EQ(exit_code, 42); + callback_calls++; + node::Stop(*env); + }); + node::LoadEnvironment(*env, "process.exit(42)").ToLocalChecked(); + EXPECT_EQ(callback_calls, 1); +} From 93a23b977de7b3b470c4528710f7f32bc29f3ee6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Mar 2020 15:07:00 +0200 Subject: [PATCH 20/37] embedding: make Stop() stop Workers This makes sense given that terminating execution of the parent thread this way likely also is supposed to stop all running Worker threads spawned by it. PR-URL: https://github.com/nodejs/node/pull/32531 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell --- src/api/environment.cc | 3 +-- src/env.cc | 3 ++- src/env.h | 2 +- src/node.cc | 2 +- src/node.h | 7 ++++--- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index cd05b3e4794a1a..c56b277a8acc3b 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -712,8 +712,7 @@ ThreadId AllocateEnvironmentThreadId() { } void DefaultProcessExitHandler(Environment* env, int exit_code) { - env->set_can_call_into_js(false); - env->stop_sub_worker_contexts(); + Stop(env); DisposePlatform(); exit(exit_code); } diff --git a/src/env.cc b/src/env.cc index b2e4721b96db1b..5d29deedd70ba8 100644 --- a/src/env.cc +++ b/src/env.cc @@ -523,9 +523,10 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { } } -void Environment::ExitEnv() { +void Environment::Stop() { set_can_call_into_js(false); set_stopping(true); + stop_sub_worker_contexts(); isolate_->TerminateExecution(); SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); }); } diff --git a/src/env.h b/src/env.h index 13c562133ce44a..e9e760bbb8b3b0 100644 --- a/src/env.h +++ b/src/env.h @@ -913,7 +913,7 @@ class Environment : public MemoryRetainer { void RegisterHandleCleanups(); void CleanupHandles(); void Exit(int code); - void ExitEnv(); + void Stop(); // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, diff --git a/src/node.cc b/src/node.cc index 46e8f74cc286f7..00ae36cc0fe669 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1035,7 +1035,7 @@ int Start(int argc, char** argv) { } int Stop(Environment* env) { - env->ExitEnv(); + env->Stop(); return 0; } diff --git a/src/node.h b/src/node.h index 74fe59097ff673..eea39ed44eda0c 100644 --- a/src/node.h +++ b/src/node.h @@ -218,7 +218,8 @@ class Environment; NODE_EXTERN int Start(int argc, char* argv[]); // Tear down Node.js while it is running (there are active handles -// in the loop and / or actively executing JavaScript code). +// in the loop and / or actively executing JavaScript code). This also stops +// all Workers that may have been started earlier. NODE_EXTERN int Stop(Environment* env); // TODO(addaleax): Officially deprecate this and replace it with something @@ -457,8 +458,8 @@ NODE_EXTERN void FreeEnvironment(Environment* env); // It receives the Environment* instance and the exit code as arguments. // This could e.g. call Stop(env); in order to terminate execution and stop // the event loop. -// The default handler disposes of the global V8 platform instance, if one is -// being used, and calls exit(). +// The default handler calls Stop(), disposes of the global V8 platform +// instance, if one is being used, and calls exit(). NODE_EXTERN void SetProcessExitHandler( Environment* env, std::function&& handler); From 72f42bf86f188cbf8986bb796464a0bd3823a2f4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 21 Mar 2020 11:25:14 +0100 Subject: [PATCH 21/37] test: use InitializeNodeWithArgs in cctest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs: https://github.com/nodejs/node/commit/d7f11077f15f52a2db191d3a5bcc41581cb7361f Fixes: https://github.com/nodejs/node/issues/30257 PR-URL: https://github.com/nodejs/node/pull/32406 Reviewed-By: Michaël Zasso Reviewed-By: Colin Ihrig Reviewed-By: Jiawen Geng Reviewed-By: Matheus Marchini Reviewed-By: David Carlier Reviewed-By: James M Snell --- test/cctest/node_test_fixture.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 291bc5962bed77..2f7ab06c14334f 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -73,11 +73,13 @@ class NodeTestFixture : public ::testing::Test { if (!node_initialized) { uv_os_unsetenv("NODE_OPTIONS"); node_initialized = true; - int argc = 1; - const char* argv0 = "cctest"; - int exec_argc; - const char** exec_argv; - node::Init(&argc, &argv0, &exec_argc, &exec_argv); + std::vector argv { "cctest" }; + std::vector exec_argv; + std::vector errors; + + int exitcode = node::InitializeNodeWithArgs(&argv, &exec_argv, &errors); + CHECK_EQ(exitcode, 0); + CHECK(errors.empty()); } tracing_agent = std::make_unique(); From b931e686bb0977ebc4cbba302914d5cc11db93df Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 11:50:20 +0100 Subject: [PATCH 22/37] test: use common.buildType in embedding test This un-breaks testing in the case of `./configure --debug-node`. PR-URL: https://github.com/nodejs/node/pull/32422 Reviewed-By: Richard Lau Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- test/embedding/test-embedding.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/embedding/test-embedding.js b/test/embedding/test-embedding.js index 43dab0ab24ea53..ac50f22e867de1 100644 --- a/test/embedding/test-embedding.js +++ b/test/embedding/test-embedding.js @@ -6,8 +6,7 @@ const child_process = require('child_process'); const path = require('path'); common.allowGlobals(global.require); -let binary = process.features.debug ? - 'out/Debug/embedtest' : 'out/Release/embedtest'; +let binary = `out/${common.buildType}/embedtest`; if (common.isWindows) { binary += '.exe'; } From 9aa4cbebfc5cb46ebcb964f29604a95033298099 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 5 Apr 2020 23:13:31 +0200 Subject: [PATCH 23/37] src: initialize inspector before RunBootstrapping() This is necessary for `--inspect-brk-node` to work, and for the inspector to be aware of scripts created before bootstrapping. Fixes: https://github.com/nodejs/node/issues/32648 Refs: https://github.com/nodejs/node/pull/30467#discussion_r396879908 PR-URL: https://github.com/nodejs/node/pull/32672 Reviewed-By: Gus Caplan Reviewed-By: Eugene Ostroukhov Reviewed-By: Joyee Cheung Reviewed-By: David Carlier Reviewed-By: James M Snell --- src/api/environment.cc | 56 +++++++++---------- src/node.h | 18 +++--- src/node_worker.cc | 9 +-- test/cctest/node_test_fixture.h | 12 +++- test/cctest/test_environment.cc | 11 ++-- .../test-inspector-inspect-brk-node.js | 28 ++++++++++ 6 files changed, 85 insertions(+), 49 deletions(-) create mode 100644 test/parallel/test-inspector-inspect-brk-node.js diff --git a/src/api/environment.cc b/src/api/environment.cc index c56b277a8acc3b..9f41c82c7d2acb 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -317,6 +317,19 @@ void FreeIsolateData(IsolateData* isolate_data) { delete isolate_data; } +InspectorParentHandle::~InspectorParentHandle() {} + +// Hide the internal handle class from the public API. +#if HAVE_INSPECTOR +struct InspectorParentHandleImpl : public InspectorParentHandle { + std::unique_ptr impl; + + explicit InspectorParentHandleImpl( + std::unique_ptr&& impl) + : impl(std::move(impl)) {} +}; +#endif + Environment* CreateEnvironment(IsolateData* isolate_data, Local context, int argc, @@ -335,7 +348,8 @@ Environment* CreateEnvironment( const std::vector& args, const std::vector& exec_args, EnvironmentFlags::Flags flags, - ThreadId thread_id) { + ThreadId thread_id, + std::unique_ptr inspector_parent_handle) { Isolate* isolate = context->GetIsolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(context); @@ -352,6 +366,16 @@ Environment* CreateEnvironment( env->set_abort_on_uncaught_exception(false); } +#if HAVE_INSPECTOR + if (inspector_parent_handle) { + env->InitializeInspector( + std::move(static_cast( + inspector_parent_handle.get())->impl)); + } else { + env->InitializeInspector({}); + } +#endif + if (env->RunBootstrapping().IsEmpty()) { FreeEnvironment(env); return nullptr; @@ -381,19 +405,6 @@ void FreeEnvironment(Environment* env) { delete env; } -InspectorParentHandle::~InspectorParentHandle() {} - -// Hide the internal handle class from the public API. -#if HAVE_INSPECTOR -struct InspectorParentHandleImpl : public InspectorParentHandle { - std::unique_ptr impl; - - explicit InspectorParentHandleImpl( - std::unique_ptr&& impl) - : impl(std::move(impl)) {} -}; -#endif - NODE_EXTERN std::unique_ptr GetInspectorParentHandle( Environment* env, ThreadId thread_id, @@ -417,27 +428,17 @@ void LoadEnvironment(Environment* env) { MaybeLocal LoadEnvironment( Environment* env, StartExecutionCallback cb, - std::unique_ptr inspector_parent_handle) { + std::unique_ptr removeme) { env->InitializeLibuv(per_process::v8_is_profiling); env->InitializeDiagnostics(); -#if HAVE_INSPECTOR - if (inspector_parent_handle) { - env->InitializeInspector( - std::move(static_cast( - inspector_parent_handle.get())->impl)); - } else { - env->InitializeInspector({}); - } -#endif - return StartExecution(env, cb); } MaybeLocal LoadEnvironment( Environment* env, const char* main_script_source_utf8, - std::unique_ptr inspector_parent_handle) { + std::unique_ptr removeme) { CHECK_NOT_NULL(main_script_source_utf8); return LoadEnvironment( env, @@ -462,8 +463,7 @@ MaybeLocal LoadEnvironment( env->process_object(), env->native_module_require()}; return ExecuteBootstrapper(env, name.c_str(), ¶ms, &args); - }, - std::move(inspector_parent_handle)); + }); } Environment* GetCurrentEnvironment(Local context) { diff --git a/src/node.h b/src/node.h index eea39ed44eda0c..fbe9c9e74061ec 100644 --- a/src/node.h +++ b/src/node.h @@ -400,6 +400,10 @@ enum Flags : uint64_t { }; } // namespace EnvironmentFlags +struct InspectorParentHandle { + virtual ~InspectorParentHandle(); +}; + // TODO(addaleax): Maybe move per-Environment options parsing here. // Returns nullptr when the Environment cannot be created e.g. there are // pending JavaScript exceptions. @@ -416,16 +420,14 @@ NODE_EXTERN Environment* CreateEnvironment( const std::vector& args, const std::vector& exec_args, EnvironmentFlags::Flags flags = EnvironmentFlags::kDefaultFlags, - ThreadId thread_id = {} /* allocates a thread id automatically */); + ThreadId thread_id = {} /* allocates a thread id automatically */, + std::unique_ptr inspector_parent_handle = {}); -struct InspectorParentHandle { - virtual ~InspectorParentHandle(); -}; // Returns a handle that can be passed to `LoadEnvironment()`, making the // child Environment accessible to the inspector as if it were a Node.js Worker. // `child_thread_id` can be created using `AllocateEnvironmentThreadId()` // and then later passed on to `CreateEnvironment()` to create the child -// Environment. +// Environment, together with the inspector handle. // This method should not be called while the parent Environment is active // on another thread. NODE_EXTERN std::unique_ptr GetInspectorParentHandle( @@ -443,14 +445,16 @@ using StartExecutionCallback = // TODO(addaleax): Deprecate this in favour of the MaybeLocal<> overload. NODE_EXTERN void LoadEnvironment(Environment* env); +// The `InspectorParentHandle` arguments here are ignored and not used. +// For passing `InspectorParentHandle`, use `CreateEnvironment()`. NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, StartExecutionCallback cb, - std::unique_ptr inspector_parent_handle = {}); + std::unique_ptr ignored_donotuse_removeme = {}); NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, const char* main_script_source_utf8, - std::unique_ptr inspector_parent_handle = {}); + std::unique_ptr ignored_donotuse_removeme = {}); NODE_EXTERN void FreeEnvironment(Environment* env); // Set a callback that is called when process.exit() is called from JS, diff --git a/src/node_worker.cc b/src/node_worker.cc index afa4a9a52d0192..bd43a9cfbab97f 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -317,7 +317,8 @@ void Worker::Run() { std::move(argv_), std::move(exec_argv_), EnvironmentFlags::kNoFlags, - thread_id_)); + thread_id_, + std::move(inspector_parent_handle_))); if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); @@ -335,12 +336,8 @@ void Worker::Run() { { CreateEnvMessagePort(env_.get()); Debug(this, "Created message port for worker %llu", thread_id_.id); - if (LoadEnvironment(env_.get(), - StartExecutionCallback{}, - std::move(inspector_parent_handle_)) - .IsEmpty()) { + if (LoadEnvironment(env_.get(), StartExecutionCallback{}).IsEmpty()) return; - } Debug(this, "Loaded environment for worker %llu", thread_id_.id); } diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 2f7ab06c14334f..903900ac9c94f1 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -125,7 +125,10 @@ class EnvironmentTestFixture : public NodeTestFixture { public: class Env { public: - Env(const v8::HandleScope& handle_scope, const Argv& argv) { + Env(const v8::HandleScope& handle_scope, + const Argv& argv, + node::EnvironmentFlags::Flags flags = + node::EnvironmentFlags::kDefaultFlags) { auto isolate = handle_scope.GetIsolate(); context_ = node::NewContext(isolate); CHECK(!context_.IsEmpty()); @@ -135,10 +138,13 @@ class EnvironmentTestFixture : public NodeTestFixture { &NodeTestFixture::current_loop, platform.get()); CHECK_NE(nullptr, isolate_data_); + std::vector args(*argv, *argv + 1); + std::vector exec_args(*argv, *argv + 1); environment_ = node::CreateEnvironment(isolate_data_, context_, - 1, *argv, - argv.nr_args(), *argv); + args, + exec_args, + flags); CHECK_NE(nullptr, environment_); } diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index b3a02de3726372..7197e93c073126 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -168,8 +168,9 @@ TEST_F(EnvironmentTest, AtExitRunsJS) { TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { const v8::HandleScope handle_scope(isolate_); const Argv argv; + // Only one of the Environments can have default flags and own the inspector. Env env1 {handle_scope, argv}; - Env env2 {handle_scope, argv}; + Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags}; AtExit(*env1, at_exit_callback1); AtExit(*env2, at_exit_callback2); @@ -357,7 +358,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { " id: 1,\n" " method: 'Runtime.evaluate',\n" " params: {\n" - " expression: 'global.variableFromParent = 42;'\n" + " expression: 'globalThis.variableFromParent = 42;'\n" " }\n" " })\n" " });\n" @@ -424,14 +425,14 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { { "dummy" }, {}, node::EnvironmentFlags::kNoFlags, - data->thread_id); + data->thread_id, + std::move(data->inspector_parent_handle)); CHECK_NOT_NULL(environment); v8::Local extracted_value = LoadEnvironment( environment, "while (!global.variableFromParent) {}\n" - "return global.variableFromParent;", - std::move(data->inspector_parent_handle)).ToLocalChecked(); + "return global.variableFromParent;").ToLocalChecked(); uv_run(&loop, UV_RUN_DEFAULT); CHECK(extracted_value->IsInt32()); diff --git a/test/parallel/test-inspector-inspect-brk-node.js b/test/parallel/test-inspector-inspect-brk-node.js new file mode 100644 index 00000000000000..caf17b956dcd19 --- /dev/null +++ b/test/parallel/test-inspector-inspect-brk-node.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); + +// Regression test for https://github.com/nodejs/node/issues/32648 + +common.skipIfInspectorDisabled(); + +const { NodeInstance } = require('../common/inspector-helper.js'); + +async function runTest() { + const child = new NodeInstance(['--inspect-brk-node=0', '-p', '42']); + const session = await child.connectInspectorSession(); + await session.send({ method: 'Runtime.enable' }); + await session.send({ method: 'Debugger.enable' }); + await session.send({ method: 'Runtime.runIfWaitingForDebugger' }); + await session.waitForNotification((notification) => { + // The main assertion here is that we do hit the loader script first. + return notification.method === 'Debugger.scriptParsed' && + notification.params.url === 'internal/bootstrap/loaders.js'; + }); + + await session.waitForNotification('Debugger.paused'); + await session.send({ method: 'Debugger.resume' }); + await session.waitForNotification('Debugger.paused'); + await session.runToCompletion(); +} + +runTest().then(common.mustCall()); From 0a2f5e9f6d17dbb1133e1d45657c84f89dcd3a58 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 27 Apr 2020 03:36:23 +0200 Subject: [PATCH 24/37] worker: unify custom error creation Mostly, this introduces a pattern that makes sure that if a custom error is reported, `stopped_` will be set to `true` correctly in every cast, which was previously missing for the `NewContext().IsEmpty()` case (which led to a hard crash from the `Worker` destructor). This also leaves TODO comments for a few cases in which `ERR_WORKER_OUT_OF_MEMORY` was not used in accordance with the documentation for that error code (or according to its intention). Fixing that is semver-major. PR-URL: https://github.com/nodejs/node/pull/33084 Reviewed-By: Gireesh Punathil Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/node_worker.cc | 30 +++++++++++++++++------------- src/node_worker.h | 7 +++++-- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index bd43a9cfbab97f..db809d12f81310 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -136,9 +136,7 @@ class WorkerThreadData { if (ret != 0) { char err_buf[128]; uv_err_name_r(ret, err_buf, sizeof(err_buf)); - w->custom_error_ = "ERR_WORKER_INIT_FAILED"; - w->custom_error_str_ = err_buf; - w->stopped_ = true; + w->Exit(1, "ERR_WORKER_INIT_FAILED", err_buf); return; } loop_init_failed_ = false; @@ -152,9 +150,9 @@ class WorkerThreadData { Isolate* isolate = Isolate::Allocate(); if (isolate == nullptr) { - w->custom_error_ = "ERR_WORKER_OUT_OF_MEMORY"; - w->custom_error_str_ = "Failed to create new Isolate"; - w->stopped_ = true; + // TODO(addaleax): This should be ERR_WORKER_INIT_FAILED, + // ERR_WORKER_OUT_OF_MEMORY is for reaching the per-Worker heap limit. + w->Exit(1, "ERR_WORKER_OUT_OF_MEMORY", "Failed to create new Isolate"); return; } @@ -237,9 +235,7 @@ class WorkerThreadData { size_t Worker::NearHeapLimit(void* data, size_t current_heap_limit, size_t initial_heap_limit) { Worker* worker = static_cast(data); - worker->custom_error_ = "ERR_WORKER_OUT_OF_MEMORY"; - worker->custom_error_str_ = "JS heap out of memory"; - worker->Exit(1); + worker->Exit(1, "ERR_WORKER_OUT_OF_MEMORY", "JS heap out of memory"); // Give the current GC some extra leeway to let it finish rather than // crash hard. We are not going to perform further allocations anyway. constexpr size_t kExtraHeapAllowance = 16 * 1024 * 1024; @@ -301,8 +297,9 @@ void Worker::Run() { TryCatch try_catch(isolate_); context = NewContext(isolate_); if (context.IsEmpty()) { - custom_error_ = "ERR_WORKER_OUT_OF_MEMORY"; - custom_error_str_ = "Failed to create new Context"; + // TODO(addaleax): This should be ERR_WORKER_INIT_FAILED, + // ERR_WORKER_OUT_OF_MEMORY is for reaching the per-Worker heap limit. + Exit(1, "ERR_WORKER_OUT_OF_MEMORY", "Failed to create new Context"); return; } } @@ -685,9 +682,16 @@ Local Worker::GetResourceLimits(Isolate* isolate) const { return Float64Array::New(ab, 0, kTotalResourceLimitCount); } -void Worker::Exit(int code) { +void Worker::Exit(int code, const char* error_code, const char* error_message) { Mutex::ScopedLock lock(mutex_); - Debug(this, "Worker %llu called Exit(%d)", thread_id_.id, code); + Debug(this, "Worker %llu called Exit(%d, %s, %s)", + thread_id_.id, code, error_code, error_message); + + if (error_code != nullptr) { + custom_error_ = error_code; + custom_error_str_ = error_message; + } + if (env_ != nullptr) { exit_code_ = code; Stop(env_); diff --git a/src/node_worker.h b/src/node_worker.h index a7d02914826563..069bd190fca5f7 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -34,8 +34,11 @@ class Worker : public AsyncWrap { void Run(); // Forcibly exit the thread with a specified exit code. This may be called - // from any thread. - void Exit(int code); + // from any thread. `error_code` and `error_message` can be used to create + // a custom `'error'` event before emitting `'exit'`. + void Exit(int code, + const char* error_code = nullptr, + const char* error_message = nullptr); // Wait for the worker thread to stop (in a blocking manner). void JoinThread(); From b910fc68b3b9aaeb7abe9f0be452870199c87576 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 17 Sep 2020 16:07:32 +0200 Subject: [PATCH 25/37] fixup! src: run native immediates during Environment cleanup --- test/cctest/test_environment.cc | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 7197e93c073126..977a1e1902d763 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -267,10 +267,10 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { EXPECT_EQ(env_arg, *env); called++; }); - (*env)->SetUnrefImmediate([&](node::Environment* env_arg) { + (*env)->SetImmediate([&](node::Environment* env_arg) { EXPECT_EQ(env_arg, *env); called_unref++; - }); + }, node::CallbackFlags::kUnrefed); } EXPECT_EQ(called, 1); @@ -308,29 +308,6 @@ TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) { CHECK_EQ(ab->ByteLength(), 0); } -TEST_F(EnvironmentTest, SetImmediateCleanup) { - int called = 0; - int called_unref = 0; - - { - const v8::HandleScope handle_scope(isolate_); - const Argv argv; - Env env {handle_scope, argv}; - - (*env)->SetImmediate([&](node::Environment* env_arg) { - EXPECT_EQ(env_arg, *env); - called++; - }); - (*env)->SetImmediate([&](node::Environment* env_arg) { - EXPECT_EQ(env_arg, *env); - called_unref++; - }, node::CallbackFlags::kUnrefed); - } - - EXPECT_EQ(called, 1); - EXPECT_EQ(called_unref, 0); -} - #if HAVE_INSPECTOR TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { // Tests that child Environments can be created through the public API From 1ee495df2b384b61f80421f7389987002a8d1936 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 17 Sep 2020 16:08:08 +0200 Subject: [PATCH 26/37] fixup! test: add extended embedder cctest --- test/cctest/test_environment.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 977a1e1902d763..d0182ca8620fac 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -380,7 +380,7 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { node::ArrayBufferAllocator::Create(); uv_loop_t loop; uv_loop_init(&loop); - v8::Isolate* isolate = NewIsolate(aba, &loop, data->platform); + v8::Isolate* isolate = NewIsolate(aba.get(), &loop, data->platform); CHECK_NOT_NULL(isolate); { From 30b6c2f79b2b4b73c06424216378cf7872056f1c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 17 Sep 2020 16:09:53 +0200 Subject: [PATCH 27/37] fixup! src,test: add full-featured embedder API test --- test/embedding/embedtest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index 14e2dfb802d22c..366c8f12e2c11f 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -62,7 +62,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform, std::shared_ptr allocator = ArrayBufferAllocator::Create(); - Isolate* isolate = NewIsolate(allocator, &loop, platform); + Isolate* isolate = NewIsolate(allocator.get(), &loop, platform); if (isolate == nullptr) { fprintf(stderr, "%s: Failed to initialize V8 Isolate\n", args[0].c_str()); return 1; From 6e63159027e72dd812905062a6d1d0c6ab0b07d6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 17 Sep 2020 16:18:52 +0200 Subject: [PATCH 28/37] fixup! doc: add basic embedding example documentation --- doc/api/embedding.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/embedding.md b/doc/api/embedding.md index 7db24b325ec7ef..f1ab0db5a5329d 100644 --- a/doc/api/embedding.md +++ b/doc/api/embedding.md @@ -33,7 +33,7 @@ Node.js requires some per-process state management in order to run: The following example shows how these can be set up. Some class names are from the `node` and `v8` C++ namespaces, respectively. -```c++ +```cpp int main(int argc, char** argv) { std::vector args(argv, argv + argc); std::vector exec_args; @@ -93,7 +93,7 @@ The `node::NewIsolate()` helper function creates a `v8::Isolate`, sets it up with some Node.js-specific hooks (e.g. the Node.js error handler), and registers it with the platform automatically. -```c++ +```cpp int RunNodeInstance(MultiIsolatePlatform* platform, const std::vector& args, const std::vector& exec_args) { From 8f464fe91bd197fdbbddbf9d9859f129a02c7bf5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 21:07:21 +0100 Subject: [PATCH 29/37] src: make `Environment::interrupt_data_` atomic Otherwise this potentially leads to race conditions when used in a multi-threaded environment (i.e. when used for its very purpose). PR-URL: https://github.com/nodejs/node/pull/32523 Reviewed-By: Matheus Marchini Reviewed-By: James M Snell --- src/env.cc | 24 ++++++++++++++++++------ src/env.h | 2 +- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/env.cc b/src/env.cc index 5d29deedd70ba8..4f1ffbd7400f43 100644 --- a/src/env.cc +++ b/src/env.cc @@ -412,7 +412,8 @@ Environment::Environment(IsolateData* isolate_data, } Environment::~Environment() { - if (interrupt_data_ != nullptr) *interrupt_data_ = nullptr; + if (Environment** interrupt_data = interrupt_data_.load()) + *interrupt_data = nullptr; // FreeEnvironment() should have set this. CHECK(is_stopping()); @@ -737,12 +738,23 @@ void Environment::RunAndClearNativeImmediates(bool only_refed) { } void Environment::RequestInterruptFromV8() { - if (interrupt_data_ != nullptr) return; // Already scheduled. - // The Isolate may outlive the Environment, so some logic to handle the // situation in which the Environment is destroyed before the handler runs // is required. - interrupt_data_ = new Environment*(this); + + // We allocate a new pointer to a pointer to this Environment instance, and + // try to set it as interrupt_data_. If interrupt_data_ was already set, then + // callbacks are already scheduled to run and we can delete our own pointer + // and just return. If it was nullptr previously, the Environment** is stored; + // ~Environment sets the Environment* contained in it to nullptr, so that + // the callback can check whether ~Environment has already run and it is thus + // not safe to access the Environment instance itself. + Environment** interrupt_data = new Environment*(this); + Environment** dummy = nullptr; + if (!interrupt_data_.compare_exchange_strong(dummy, interrupt_data)) { + delete interrupt_data; + return; // Already scheduled. + } isolate()->RequestInterrupt([](Isolate* isolate, void* data) { std::unique_ptr env_ptr { static_cast(data) }; @@ -753,9 +765,9 @@ void Environment::RequestInterruptFromV8() { // handled during cleanup. return; } - env->interrupt_data_ = nullptr; + env->interrupt_data_.store(nullptr); env->RunAndClearInterrupts(); - }, interrupt_data_); + }, interrupt_data); } void Environment::ScheduleTimer(int64_t duration_ms) { diff --git a/src/env.h b/src/env.h index e9e760bbb8b3b0..98a3172075ce6e 100644 --- a/src/env.h +++ b/src/env.h @@ -1412,7 +1412,7 @@ class Environment : public MemoryRetainer { void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); - Environment** interrupt_data_ = nullptr; + std::atomic interrupt_data_ {nullptr}; void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); From a74f8704357a9bb958bb79aca29d7cf1e6a3b7e5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 27 Mar 2020 21:38:39 +0100 Subject: [PATCH 30/37] src: fix cleanup hook removal for InspectorTimer Fix this to account for the fact that `Stop()` may already have been called from a cleanup hook when the `inspector::Agent` is deleted along with the `Environment`, at which point cleanup hooks are no longer available. PR-URL: https://github.com/nodejs/node/pull/32523 Reviewed-By: Matheus Marchini Reviewed-By: James M Snell --- src/inspector_agent.cc | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index c53e3af1d55f65..b9da4e4d5a6100 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -355,32 +355,26 @@ class InspectorTimer { int64_t interval_ms = 1000 * interval_s; uv_timer_start(&timer_, OnTimer, interval_ms, interval_ms); timer_.data = this; - - env->AddCleanupHook(CleanupHook, this); } InspectorTimer(const InspectorTimer&) = delete; void Stop() { - env_->RemoveCleanupHook(CleanupHook, this); + if (timer_.data == nullptr) return; - if (timer_.data == this) { - timer_.data = nullptr; - uv_timer_stop(&timer_); - env_->CloseHandle(reinterpret_cast(&timer_), TimerClosedCb); - } + timer_.data = nullptr; + uv_timer_stop(&timer_); + env_->CloseHandle(reinterpret_cast(&timer_), TimerClosedCb); } + inline Environment* env() const { return env_; } + private: static void OnTimer(uv_timer_t* uvtimer) { InspectorTimer* timer = node::ContainerOf(&InspectorTimer::timer_, uvtimer); timer->callback_(timer->data_); } - static void CleanupHook(void* data) { - static_cast(data)->Stop(); - } - static void TimerClosedCb(uv_handle_t* uvtimer) { std::unique_ptr timer( node::ContainerOf(&InspectorTimer::timer_, @@ -403,16 +397,29 @@ class InspectorTimerHandle { InspectorTimerHandle(Environment* env, double interval_s, V8InspectorClient::TimerCallback callback, void* data) { timer_ = new InspectorTimer(env, interval_s, callback, data); + + env->AddCleanupHook(CleanupHook, this); } InspectorTimerHandle(const InspectorTimerHandle&) = delete; ~InspectorTimerHandle() { - CHECK_NOT_NULL(timer_); - timer_->Stop(); - timer_ = nullptr; + Stop(); } + private: + void Stop() { + if (timer_ != nullptr) { + timer_->env()->RemoveCleanupHook(CleanupHook, this); + timer_->Stop(); + } + timer_ = nullptr; + } + + static void CleanupHook(void* data) { + static_cast(data)->Stop(); + } + InspectorTimer* timer_; }; @@ -735,8 +742,9 @@ class NodeInspectorClient : public V8InspectorClient { bool is_main_; bool running_nested_loop_ = false; std::unique_ptr client_; - std::unordered_map> channels_; + // Note: ~ChannelImpl may access timers_ so timers_ has to come first. std::unordered_map timers_; + std::unordered_map> channels_; int next_session_id_ = 1; bool waiting_for_resume_ = false; bool waiting_for_frontend_ = false; From a2ebe11ebe989b345bf0a76dba5f9fc89c61351f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 19:03:24 +0100 Subject: [PATCH 31/37] src: use env->RequestInterrupt() for inspector io thread start This cleans up `Agent::RequestIoThreadStart()` significantly. PR-URL: https://github.com/nodejs/node/pull/32523 Reviewed-By: Matheus Marchini Reviewed-By: James M Snell --- src/inspector_agent.cc | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index b9da4e4d5a6100..7d665aa5d1381f 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -46,7 +46,6 @@ using v8::Local; using v8::Message; using v8::Object; using v8::Task; -using v8::TaskRunner; using v8::Value; using v8_inspector::StringBuffer; using v8_inspector::StringView; @@ -61,18 +60,6 @@ static std::atomic_bool start_io_thread_async_initialized { false }; // Protects the Agent* stored in start_io_thread_async.data. static Mutex start_io_thread_async_mutex; -class StartIoTask : public Task { - public: - explicit StartIoTask(Agent* agent) : agent(agent) {} - - void Run() override { - agent->StartIoThread(); - } - - private: - Agent* agent; -}; - std::unique_ptr ToProtocolString(Isolate* isolate, Local value) { TwoByteValue buffer(isolate, value); @@ -84,10 +71,6 @@ void StartIoThreadAsyncCallback(uv_async_t* handle) { static_cast(handle->data)->StartIoThread(); } -void StartIoInterrupt(Isolate* isolate, void* agent) { - static_cast(agent)->StartIoThread(); -} - #ifdef __POSIX__ static void StartIoThreadWakeup(int signo) { @@ -981,12 +964,10 @@ void Agent::RequestIoThreadStart() { // for IO events) CHECK(start_io_thread_async_initialized); uv_async_send(&start_io_thread_async); - Isolate* isolate = parent_env_->isolate(); - v8::Platform* platform = parent_env_->isolate_data()->platform(); - std::shared_ptr taskrunner = - platform->GetForegroundTaskRunner(isolate); - taskrunner->PostTask(std::make_unique(this)); - isolate->RequestInterrupt(StartIoInterrupt, this); + parent_env_->RequestInterrupt([this](Environment*) { + StartIoThread(); + }); + CHECK(start_io_thread_async_initialized); uv_async_send(&start_io_thread_async); } From 83015b003c9adcfb402d21d480af43cb9ebbb14f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 22 Mar 2020 19:04:57 +0100 Subject: [PATCH 32/37] src: use env->RequestInterrupt() for inspector MainThreadInterface This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. https://github.com/nodejs/node/issues/32415 PR-URL: https://github.com/nodejs/node/pull/32523 Reviewed-By: Matheus Marchini Reviewed-By: James M Snell --- src/env.h | 5 ++- src/inspector/main_thread_interface.cc | 41 ++++--------------- src/inspector/main_thread_interface.h | 5 +-- src/inspector_agent.cc | 6 +-- src/inspector_agent.h | 2 + src/inspector_js_api.cc | 2 +- .../test-inspector-connect-main-thread.js | 6 +++ 7 files changed, 23 insertions(+), 44 deletions(-) diff --git a/src/env.h b/src/env.h index 98a3172075ce6e..f947f0bf4301fc 100644 --- a/src/env.h +++ b/src/env.h @@ -1273,6 +1273,9 @@ class Environment : public MemoryRetainer { inline void set_process_exit_handler( std::function&& handler); + void RunAndClearNativeImmediates(bool only_refed = false); + void RunAndClearInterrupts(); + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1410,8 +1413,6 @@ class Environment : public MemoryRetainer { // yet or already have been destroyed. bool task_queues_async_initialized_ = false; - void RunAndClearNativeImmediates(bool only_refed = false); - void RunAndClearInterrupts(); std::atomic interrupt_data_ {nullptr}; void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc index 48a964d564fec2..a15cd52d239e40 100644 --- a/src/inspector/main_thread_interface.cc +++ b/src/inspector/main_thread_interface.cc @@ -1,5 +1,6 @@ #include "main_thread_interface.h" +#include "env-inl.h" #include "node_mutex.h" #include "v8-inspector.h" #include "util-inl.h" @@ -85,19 +86,6 @@ class CallRequest : public Request { Fn fn_; }; -class DispatchMessagesTask : public v8::Task { - public: - explicit DispatchMessagesTask(std::weak_ptr thread) - : thread_(thread) {} - - void Run() override { - if (auto thread = thread_.lock()) thread->DispatchMessages(); - } - - private: - std::weak_ptr thread_; -}; - template class AnotherThreadObjectReference { public: @@ -212,12 +200,7 @@ class ThreadSafeDelegate : public InspectorSessionDelegate { } // namespace -MainThreadInterface::MainThreadInterface(Agent* agent, uv_loop_t* loop, - v8::Isolate* isolate, - v8::Platform* platform) - : agent_(agent), isolate_(isolate), - platform_(platform) { -} +MainThreadInterface::MainThreadInterface(Agent* agent) : agent_(agent) {} MainThreadInterface::~MainThreadInterface() { if (handle_) @@ -225,23 +208,15 @@ MainThreadInterface::~MainThreadInterface() { } void MainThreadInterface::Post(std::unique_ptr request) { + CHECK_NOT_NULL(agent_); Mutex::ScopedLock scoped_lock(requests_lock_); bool needs_notify = requests_.empty(); requests_.push_back(std::move(request)); if (needs_notify) { - if (isolate_ != nullptr && platform_ != nullptr) { - std::shared_ptr taskrunner = - platform_->GetForegroundTaskRunner(isolate_); - std::weak_ptr* interface_ptr = - new std::weak_ptr(shared_from_this()); - taskrunner->PostTask( - std::make_unique(*interface_ptr)); - isolate_->RequestInterrupt([](v8::Isolate* isolate, void* opaque) { - std::unique_ptr> interface_ptr { - static_cast*>(opaque) }; - if (auto iface = interface_ptr->lock()) iface->DispatchMessages(); - }, static_cast(interface_ptr)); - } + std::weak_ptr weak_self {shared_from_this()}; + agent_->env()->RequestInterrupt([weak_self](Environment*) { + if (auto iface = weak_self.lock()) iface->DispatchMessages(); + }); } incoming_message_cond_.Broadcast(scoped_lock); } @@ -274,7 +249,7 @@ void MainThreadInterface::DispatchMessages() { std::swap(dispatching_message_queue_.front(), task); dispatching_message_queue_.pop_front(); - v8::SealHandleScope seal_handle_scope(isolate_); + v8::SealHandleScope seal_handle_scope(agent_->env()->isolate()); task->Call(this); } } while (had_messages); diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h index 78337a25e43808..3ec5b3db3e1600 100644 --- a/src/inspector/main_thread_interface.h +++ b/src/inspector/main_thread_interface.h @@ -72,8 +72,7 @@ class MainThreadHandle : public std::enable_shared_from_this { class MainThreadInterface : public std::enable_shared_from_this { public: - MainThreadInterface(Agent* agent, uv_loop_t*, v8::Isolate* isolate, - v8::Platform* platform); + explicit MainThreadInterface(Agent* agent); ~MainThreadInterface(); void DispatchMessages(); @@ -98,8 +97,6 @@ class MainThreadInterface : ConditionVariable incoming_message_cond_; // Used from any thread Agent* const agent_; - v8::Isolate* const isolate_; - v8::Platform* const platform_; std::shared_ptr handle_; std::unordered_map> managed_objects_; }; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 7d665aa5d1381f..26101fe0e13b5e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -660,8 +660,7 @@ class NodeInspectorClient : public V8InspectorClient { std::shared_ptr getThreadHandle() { if (!interface_) { interface_ = std::make_shared( - env_->inspector_agent(), env_->event_loop(), env_->isolate(), - env_->isolate_data()->platform()); + env_->inspector_agent()); } return interface_->GetHandle(); } @@ -697,10 +696,9 @@ class NodeInspectorClient : public V8InspectorClient { running_nested_loop_ = true; - MultiIsolatePlatform* platform = env_->isolate_data()->platform(); while (shouldRunMessageLoop()) { if (interface_) interface_->WaitForFrontendEvent(); - while (platform->FlushForegroundTasks(env_->isolate())) {} + env_->RunAndClearInterrupts(); } running_nested_loop_ = false; } diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 089077370db049..efd090c49b4311 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -116,6 +116,8 @@ class Agent { // Interface for interacting with inspectors in worker threads std::shared_ptr GetWorkerManager(); + inline Environment* env() const { return parent_env_; } + private: void ToggleAsyncHook(v8::Isolate* isolate, const v8::Global& fn); diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index acc767cb8745bb..8616575e066121 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -83,7 +83,7 @@ class JSBindingsConnection : public AsyncWrap { private: Environment* env_; - JSBindingsConnection* connection_; + BaseObjectPtr connection_; }; JSBindingsConnection(Environment* env, diff --git a/test/parallel/test-inspector-connect-main-thread.js b/test/parallel/test-inspector-connect-main-thread.js index bb21a54e90f1d1..be34981cd0c5be 100644 --- a/test/parallel/test-inspector-connect-main-thread.js +++ b/test/parallel/test-inspector-connect-main-thread.js @@ -76,6 +76,12 @@ function doConsoleLog(arrayBuffer) { // and do not interrupt the main code. Interrupting the code flow // can lead to unexpected behaviors. async function ensureListenerDoesNotInterrupt(session) { + // Make sure that the following code is not affected by the fact that it may + // run inside an inspector message callback, during which other inspector + // message callbacks (such as the one triggered by doConsoleLog()) would + // not be processed. + await new Promise(setImmediate); + const currentTime = Date.now(); let consoleLogHappened = false; session.once('Runtime.consoleAPICalled', From 2b0315fd5e5763e09d3859fb4cfa0ad39e69efa5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 28 Mar 2020 01:14:06 +0100 Subject: [PATCH 33/37] src: flush V8 interrupts from Environment dtor This avoids an edge-case memory leak. Refs: https://github.com/nodejs/node/pull/32523#discussion_r399554491 PR-URL: https://github.com/nodejs/node/pull/32523 Reviewed-By: Matheus Marchini Reviewed-By: James M Snell --- src/env.cc | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/env.cc b/src/env.cc index 4f1ffbd7400f43..703a2a15758f95 100644 --- a/src/env.cc +++ b/src/env.cc @@ -41,11 +41,13 @@ using v8::NewStringType; using v8::Number; using v8::Object; using v8::Private; +using v8::Script; using v8::SnapshotCreator; using v8::StackTrace; using v8::String; using v8::Symbol; using v8::TracingController; +using v8::TryCatch; using v8::Undefined; using v8::Value; using worker::Worker; @@ -412,11 +414,30 @@ Environment::Environment(IsolateData* isolate_data, } Environment::~Environment() { - if (Environment** interrupt_data = interrupt_data_.load()) + if (Environment** interrupt_data = interrupt_data_.load()) { + // There are pending RequestInterrupt() callbacks. Tell them not to run, + // then force V8 to run interrupts by compiling and running an empty script + // so as not to leak memory. *interrupt_data = nullptr; - // FreeEnvironment() should have set this. - CHECK(is_stopping()); + Isolate::AllowJavascriptExecutionScope allow_js_here(isolate()); + HandleScope handle_scope(isolate()); + TryCatch try_catch(isolate()); + Context::Scope context_scope(context()); + +#ifdef DEBUG + bool consistency_check = false; + isolate()->RequestInterrupt([](Isolate*, void* data) { + *static_cast(data) = true; + }, &consistency_check); +#endif + + Local