From b6738bc62479391e5d93026716ed92d6879ea60e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Nov 2019 19:47:03 +0000 Subject: [PATCH 01/28] 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.) --- src/api/environment.cc | 18 +++++++++++++++++- src/node_main_instance.cc | 18 ++++++------------ src/node_main_instance.h | 3 ++- src/node_platform.cc | 7 +++++-- src/node_worker.cc | 26 +++++++++----------------- 5 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 456854a318e7f1..0a37da8aefe863 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -353,7 +353,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 998cf260de2a62..831ba528284a52 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -112,7 +112,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()); @@ -151,10 +152,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. @@ -169,10 +167,6 @@ int NodeMainInstance::Run() { } #endif - RunAtExit(env.get()); - - per_process::v8_platform.DrainVMTasks(isolate_); - #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); #endif @@ -182,8 +176,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_); @@ -208,14 +202,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 713051efc3fc7a..9f660d21be7bac 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -422,6 +422,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) { void NodePlatform::DrainTasks(Isolate* isolate) { std::shared_ptr per_isolate = ForNodeIsolate(isolate); + if (!per_isolate) return; do { // Worker tasks aren't associated with an Isolate. @@ -490,12 +491,14 @@ std::shared_ptr NodePlatform::ForNodeIsolate(Isolate* isolate) { Mutex::ScopedLock lock(per_isolate_mutex_); auto data = per_isolate_[isolate]; - CHECK(data.second); + CHECK_NOT_NULL(data.first); return data.second; } bool NodePlatform::FlushForegroundTasks(Isolate* isolate) { - return ForNodeIsolate(isolate)->FlushForegroundTasksInternal(); + std::shared_ptr per_isolate = ForNodeIsolate(isolate); + if (!per_isolate) return false; + return per_isolate->FlushForegroundTasksInternal(); } bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { diff --git a/src/node_worker.cc b/src/node_worker.cc index 6ff1a0afe9915b..1e1877e026c63c 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -270,26 +270,18 @@ void Worker::Run() { auto cleanup_env = OnScopeLeave([&]() { 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 7e2da59c66a39f090279028e1dee9020c037a86c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 11 Nov 2019 12:29:07 +0000 Subject: [PATCH 02/28] src: fix memory leak in CreateEnvironment when bootstrap fails --- 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 0a37da8aefe863..a839be86afeb6e 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -347,8 +347,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 8a04c598df0fe8c7b8083f0e3563cdc8a7e7ca29 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 18:57:10 +0000 Subject: [PATCH 03/28] 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. --- 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 69b7316e405d41..3969969a970f62 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -64,6 +64,15 @@ inline MultiIsolatePlatform* IsolateData::platform() const { return platform_; } +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), @@ -904,12 +913,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 a3ee9158c49d49..a2b31edad0c42d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -970,7 +970,7 @@ void Environment::Exit(int exit_code) { DisposePlatform(); exit(exit_code); } else { - worker_context_->Exit(exit_code); + worker_context()->Exit(exit_code); } } @@ -984,8 +984,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 MemoryTracker::TrackField(const char* edge_name, diff --git a/src/env.h b/src/env.h index 13af1874050a95..a1ef7bdbb3c2c5 100644 --- a/src/env.h +++ b/src/env.h @@ -498,6 +498,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) @@ -542,6 +545,7 @@ class IsolateData : public MemoryRetainer { const bool uses_node_allocator_; MultiIsolatePlatform* platform_; std::shared_ptr options_; + worker::Worker* worker_context_ = nullptr; }; struct ContextInfo { @@ -1062,7 +1066,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(); @@ -1382,8 +1385,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 1e1877e026c63c..e71866b9b1f493 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -183,6 +183,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_); @@ -317,7 +318,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 b732a01a0479749d360e8bf53757c996868de5a0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 12 Nov 2019 19:36:07 +0000 Subject: [PATCH 04/28] 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. --- 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 a839be86afeb6e..1c769008c01622 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -343,8 +343,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 3969969a970f62..b0af5bfcbdc473 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -889,7 +889,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 a1ef7bdbb3c2c5..c89be75e755c7e 100644 --- a/src/env.h +++ b/src/env.h @@ -861,7 +861,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 831ba528284a52..643041571fb5f3 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -207,8 +207,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 162b9ee57445490d3d7106e3b3ccf2eaa6d91550 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 11 Nov 2019 14:24:13 +0000 Subject: [PATCH 05/28] 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. --- lib/internal/bootstrap/pre_execution.js | 8 +++ src/api/environment.cc | 88 +++++++++++++++++++++++-- src/env-inl.h | 4 +- src/env.cc | 32 +++++---- src/env.h | 15 +---- src/node.cc | 15 ++--- 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 +- test/cctest/test_environment.cc | 5 +- 13 files changed, 213 insertions(+), 113 deletions(-) diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 46af6c30e0daa8..3211d19194ec3a 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -11,7 +11,15 @@ const { Buffer } = require('buffer'); const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes; const assert = require('internal/assert'); +// TODO(addaleax): Remove the flag guarding against a double run of this, +// it's only needed because of lib/internal/bootstrap/environment.js +// which should also be removed (because the legacy `CreateEnvironment()` +// also runs this file in addition to `LoadEnvironment()`, and the former +// shouldn't). +let ranPrepareMainThreadExecution = false; function prepareMainThreadExecution(expandArgv1 = false) { + if (ranPrepareMainThreadExecution) return; + ranPrepareMainThreadExecution = true; // Patch the process object with legacy properties and normalizations patchProcessObject(expandArgv1); setupTraceCategoryState(); diff --git a/src/api/environment.cc b/src/api/environment.cc index 1c769008c01622..059476b8e11c64 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; @@ -330,26 +334,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; } @@ -374,6 +392,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); } @@ -560,4 +630,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 b0af5bfcbdc473..deb7ab404a78bb 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -893,11 +893,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; } bool Environment::filehandle_close_warning() const { diff --git a/src/env.cc b/src/env.cc index a2b31edad0c42d..7fd6e13ad58cce 100644 --- a/src/env.cc +++ b/src/env.cc @@ -232,12 +232,6 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() { .ToLocalChecked(); } -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(); @@ -294,8 +288,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()), @@ -307,7 +301,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) { @@ -315,6 +310,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); @@ -333,6 +336,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()) @@ -389,6 +396,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); @@ -481,10 +491,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() { diff --git a/src/env.h b/src/env.h index c89be75e755c7e..082ee0321dc1e0 100644 --- a/src/env.h +++ b/src/env.h @@ -859,12 +859,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( @@ -883,8 +877,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); @@ -1056,9 +1050,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; @@ -1342,7 +1333,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_; diff --git a/src/node.cc b/src/node.cc index 1fec85aa793dc4..931f47035f98bf 100644 --- a/src/node.cc +++ b/src/node.cc @@ -405,7 +405,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. @@ -413,6 +413,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]; @@ -451,15 +455,6 @@ 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 diff --git a/src/node.h b/src/node.h index 189f94d9438ee5..ac5887cb6f4cbe 100644 --- a/src/node.h +++ b/src/node.h @@ -384,18 +384,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 7dcbf65f8e698a..6c9da144aa982b 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -298,6 +298,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 643041571fb5f3..033ab56188bfee 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -174,8 +174,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 @@ -202,26 +200,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 e71866b9b1f493..90e4cfb9034b9d 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 @@ -55,10 +51,10 @@ Worker::Worker(Environment* env, per_isolate_opts_(per_isolate_opts), exec_argv_(exec_argv), platform_(env->isolate_data()->platform()), - 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()); @@ -76,19 +72,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 { @@ -191,7 +185,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_); @@ -249,19 +243,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.w_->loop_init_failed_); - 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_); @@ -307,42 +301,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; @@ -382,11 +368,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) { @@ -445,7 +431,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) { @@ -643,7 +629,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); } @@ -682,7 +668,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_); @@ -709,7 +695,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); @@ -749,6 +735,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 dbd286109948da..384a9f160e09e8 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -73,12 +73,9 @@ class Worker : public AsyncWrap { MultiIsolatePlatform* platform_; 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_; @@ -88,7 +85,7 @@ class Worker : public AsyncWrap { std::string custom_error_str_; bool loop_init_failed_ = false; int exit_code_ = 0; - uint64_t thread_id_ = -1; + ThreadId thread_id_; uintptr_t stack_base_ = 0; // Custom resource constraints: diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 90c5cff5e09bf4..216b62e8a41d6e 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -188,6 +188,9 @@ static void at_exit_js(void* arg) { called_at_exit_js = true; } +// TODO(addaleax): Re-enable this test once it is possible to initialize the +// Environment properly. +/* TEST_F(EnvironmentTest, SetImmediateCleanup) { int called = 0; int called_unref = 0; @@ -209,7 +212,7 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { EXPECT_EQ(called, 1); EXPECT_EQ(called_unref, 0); -} +}*/ static char hello[] = "hello"; From 2f7cda2e040c4a1c8c512b3e76ea95a3d291f213 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 2 Mar 2020 20:46:15 +0000 Subject: [PATCH 06/28] fixup! src: align worker and main thread code with embedder API --- src/env-inl.h | 7 +++++-- src/env.cc | 15 +++++++++++++++ src/env.h | 5 +++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index deb7ab404a78bb..d78ed09d40de7a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -820,8 +820,9 @@ void Environment::SetImmediateThreadsafe(Fn&& cb) { { 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 @@ -831,8 +832,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(); } @@ -1226,6 +1228,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 7fd6e13ad58cce..28e6a0a9215177 100644 --- a/src/env.cc +++ b/src/env.cc @@ -482,6 +482,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 @@ -539,6 +548,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); @@ -1107,6 +1121,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 082ee0321dc1e0..36c18cfc6c38ac 100644 --- a/src/env.h +++ b/src/env.h @@ -1435,6 +1435,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(); From eb04cace3987877e030476ef52d9d1a4d9319bc5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Mar 2020 15:55:55 +0100 Subject: [PATCH 07/28] fixup! src: align worker and main thread code with embedder API --- lib/internal/bootstrap/pre_execution.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 3211d19194ec3a..46af6c30e0daa8 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -11,15 +11,7 @@ const { Buffer } = require('buffer'); const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes; const assert = require('internal/assert'); -// TODO(addaleax): Remove the flag guarding against a double run of this, -// it's only needed because of lib/internal/bootstrap/environment.js -// which should also be removed (because the legacy `CreateEnvironment()` -// also runs this file in addition to `LoadEnvironment()`, and the former -// shouldn't). -let ranPrepareMainThreadExecution = false; function prepareMainThreadExecution(expandArgv1 = false) { - if (ranPrepareMainThreadExecution) return; - ranPrepareMainThreadExecution = true; // Patch the process object with legacy properties and normalizations patchProcessObject(expandArgv1); setupTraceCategoryState(); From cf89ff078279df59a1409a8f1ef47244f25d2470 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Mar 2020 12:46:41 +0100 Subject: [PATCH 08/28] fixup! src: align worker and main thread code with embedder API --- src/node.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node.cc b/src/node.cc index 931f47035f98bf..ece911fb205c18 100644 --- a/src/node.cc +++ b/src/node.cc @@ -201,8 +201,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 { @@ -216,7 +216,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 From 7336b7fc1ba97d2b7d1028252631748cf12a2fed Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 13 Nov 2019 15:54:57 +0000 Subject: [PATCH 09/28] 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`. --- 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 | 40 +++++++++++++++++++++++++++++---- 6 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 059476b8e11c64..98c5146c776799 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -420,11 +420,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(); @@ -439,9 +440,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 ece911fb205c18..44d3c3f36d944d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -375,6 +375,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); @@ -395,17 +396,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 ac5887cb6f4cbe..9561ca39e250ad 100644 --- a/src/node.h +++ b/src/node.h @@ -73,6 +73,7 @@ #include "node_version.h" // NODE_MODULE_VERSION #include +#include // We cannot use __POSIX__ in this header because that's only defined when // building Node.js. @@ -438,12 +439,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 6c9da144aa982b..dfb2c6e0c37c04 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -298,9 +298,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 90e4cfb9034b9d..6772cb9c91f00f 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -323,6 +323,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 216b62e8a41d6e..a79c4709a31ef8 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,9 +217,6 @@ static void at_exit_js(void* arg) { called_at_exit_js = true; } -// TODO(addaleax): Re-enable this test once it is possible to initialize the -// Environment properly. -/* TEST_F(EnvironmentTest, SetImmediateCleanup) { int called = 0; int called_unref = 0; @@ -200,6 +226,12 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { 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++; @@ -212,7 +244,7 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { EXPECT_EQ(called, 1); EXPECT_EQ(called_unref, 0); -}*/ +} static char hello[] = "hello"; From 61098fcc7093a74ffc7c6afc8636ee501b838981 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 15:42:09 +0100 Subject: [PATCH 10/28] src: add LoadEnvironment() variant taking a string Allow passing a string as the main module rather than using the callback variant. --- src/api/environment.cc | 35 ++++++++++++++++++++++++++++++++- 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 +++++++++++++++++++++++++ 8 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 98c5146c776799..3c679f1204862d 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -420,7 +420,9 @@ NODE_EXTERN std::unique_ptr GetInspectorParentHandle( } void LoadEnvironment(Environment* env) { - USE(LoadEnvironment(env, nullptr, {})); + USE(LoadEnvironment(env, + StartExecutionCallback{}, + {})); } MaybeLocal LoadEnvironment( @@ -443,6 +445,37 @@ 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(); + String::Value main_utf16(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())); + 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/node.h b/src/node.h index 9561ca39e250ad..b69dd32b4e0480 100644 --- a/src/node.h +++ b/src/node.h @@ -453,6 +453,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 680c585d0fb3df..5523685408fffc 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -33,6 +33,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 31536000fc8d2f..b48ae962dd639f 100644 --- a/src/node_native_module_env.cc +++ b/src/node_native_module_env.cc @@ -33,6 +33,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 6772cb9c91f00f..d359f6fcea2939 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -323,7 +323,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 a79c4709a31ef8..15d9b45d490e8f 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 541d7f6c69508acfa30f02d3c2f39cbea0d18c9f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Mar 2020 15:51:04 +0100 Subject: [PATCH 11/28] fixup! src: add LoadEnvironment() variant taking a string --- src/api/environment.cc | 5 +++-- src/env-inl.h | 5 +++++ src/env.h | 7 +++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 3c679f1204862d..36df0948709976 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -458,13 +458,14 @@ MaybeLocal LoadEnvironment( String::NewFromUtf8(env->isolate(), main_script_source_utf8, v8::NewStringType::kNormal).ToLocalChecked(); - String::Value main_utf16(env->isolate(), str); + 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())); + UnionBytes(**main_utf16, main_utf16->length())); + env->set_main_utf16(std::move(main_utf16)); std::vector> params = { env->process_string(), env->require_string()}; diff --git a/src/env-inl.h b/src/env-inl.h index d78ed09d40de7a..5ee9ea6ef02e10 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1266,6 +1266,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 36c18cfc6c38ac..c639681211af8b 100644 --- a/src/env.h +++ b/src/env.h @@ -1257,6 +1257,8 @@ class Environment : public MemoryRetainer { #endif // HAVE_INSPECTOR + inline void set_main_utf16(std::unique_ptr); + private: template inline void CreateImmediate(Fn&& cb, bool ref); @@ -1466,6 +1468,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 From c0b957c1b808486799747b7ddca482af64aa3a17 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 14:20:00 -0800 Subject: [PATCH 12/28] test: re-enable cctest that was commented out Refs: https://github.com/nodejs/node/pull/31910 --- 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 15d9b45d490e8f..4d8237f103a07e 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 9bd88e7e5812187a65019802abdb3118407cba8d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 17:11:35 -0800 Subject: [PATCH 13/28] src: add unique_ptr equivalent of CreatePlatform This makes this bit of the embedder situation a bit easier to use. --- 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 36df0948709976..47bfd39f6ec9ce 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -488,13 +488,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 b69dd32b4e0480..b25afe3150f8fe 100644 --- a/src/node.h +++ b/src/node.h @@ -316,6 +316,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 { @@ -467,6 +471,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 99912a03d9f0f88170199c36085e326c11d59620 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 17:40:54 -0800 Subject: [PATCH 14/28] src: make InitializeNodeWithArgs() official public API This is a decent replacement for the to-be-deprecated Init() API. --- src/node.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/node.h b/src/node.h index b25afe3150f8fe..a003e021a21976 100644 --- a/src/node.h +++ b/src/node.h @@ -229,10 +229,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 23b9acec8453baaa3264b1d966c016baa6100c5e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 21:13:59 -0800 Subject: [PATCH 15/28] src: add ability to look up platform based on `Environment*` This should eventually remove any necessity to use the global-state `GetMainThreadMultiIsolatePlatform()`. --- 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 47bfd39f6ec9ce..d25f1482cc43ad 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -485,6 +485,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 a003e021a21976..fcd5d4c1319d52 100644 --- a/src/node.h +++ b/src/node.h @@ -477,9 +477,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 ededfc94f7017ff28392deb283da31418c0073cc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 21:43:02 -0800 Subject: [PATCH 16/28] 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 --- 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 d25f1482cc43ad..d018dafc7d472d 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -496,6 +496,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(); } @@ -506,7 +514,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 fcd5d4c1319d52..94ef91f5332121 100644 --- a/src/node.h +++ b/src/node.h @@ -329,7 +329,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 { @@ -487,9 +487,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 9f660d21be7bac..3a8715d30de4ca 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 { @@ -325,12 +324,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); } @@ -519,7 +522,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 79aad88b881f6b..508cbeab003dbc 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -138,7 +138,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; @@ -160,7 +160,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; @@ -185,7 +185,7 @@ class NodePlatform : public MultiIsolatePlatform { IsolatePlatformDelegate*, std::shared_ptr>; 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 27408eafd832ae..be2276a9e27670 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); }; @@ -505,9 +507,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 36ec54b8789069206e1fbdd594838fa3c7560642 Mon Sep 17 00:00:00 2001 From: Jichan Date: Wed, 8 Jan 2020 21:36:05 +0900 Subject: [PATCH 17/28] 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 --- src/node_v8_platform-inl.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index 0f4a98a98551ba..1dc7f878357770 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,15 @@ class NodeTraceStateObserver }; struct V8Platform { + bool initialize_; + + V8Platform() + : initialize_(false) {} + #if NODE_USE_V8_PLATFORM inline void Initialize(int thread_pool_size) { + CHECK(!initialize_); + initialize_ = true; tracing_agent_ = std::make_unique(); node::tracing::TraceEventHelper::SetAgent(tracing_agent_.get()); node::tracing::TracingController* controller = @@ -99,6 +107,10 @@ struct V8Platform { } inline void Dispose() { + if(!initialize_) + return; + initialize_ = false; + StopTracingAgent(); platform_->Shutdown(); delete platform_; From 718f91f2e161fcad8e8af77afdc5189dde2fb7c0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 21:46:46 -0800 Subject: [PATCH 18/28] fixup! src: fix what a dispose without checking --- src/node_v8_platform-inl.h | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index 1dc7f878357770..ce6d5dd223d2cb 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -80,15 +80,12 @@ class NodeTraceStateObserver }; struct V8Platform { - bool initialize_; - - V8Platform() - : initialize_(false) {} + bool initialized_ = false; #if NODE_USE_V8_PLATFORM inline void Initialize(int thread_pool_size) { - CHECK(!initialize_); - initialize_ = true; + CHECK(!initialized_); + initialized_ = true; tracing_agent_ = std::make_unique(); node::tracing::TraceEventHelper::SetAgent(tracing_agent_.get()); node::tracing::TracingController* controller = @@ -107,9 +104,9 @@ struct V8Platform { } inline void Dispose() { - if(!initialize_) + if (!initialized_) return; - initialize_ = false; + initialized_ = false; StopTracingAgent(); platform_->Shutdown(); From 12bb53a4d0e16bfd7fdd071fc3311bd6b1fef622 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 22:02:12 -0800 Subject: [PATCH 19/28] src: shutdown platform from FreePlatform() There is currently no way to properly do this. --- 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 3a8715d30de4ca..77256f98a2796c 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -338,6 +338,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_); auto delegate = std::make_shared(isolate, loop); @@ -381,6 +385,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 508cbeab003dbc..3998afae4bba5b 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -139,7 +139,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(); @@ -187,6 +187,7 @@ class NodePlatform : public MultiIsolatePlatform { v8::TracingController* tracing_controller_; std::shared_ptr worker_thread_task_runner_; + bool has_shut_down_ = false; }; } // namespace node From 487df3174ad2a13121da90fd0225263d64bd8916 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 18:54:13 -0800 Subject: [PATCH 20/28] src,test: add full-featured embedder API test --- Makefile | 10 ++- node.gyp | 49 +++++++++++++ src/node_code_cache_stub.cc | 4 ++ src/node_snapshot_stub.cc | 4 ++ test/embedding/embedtest.cc | 135 ++++++++++++++++++++++++++++++++++++ test/embedding/test.js | 20 ++++++ 6 files changed, 220 insertions(+), 2 deletions(-) create mode 100644 test/embedding/embedtest.cc create mode 100644 test/embedding/test.js diff --git a/Makefile b/Makefile index 3a97f15fc3c6a4..e144d8b81cec3b 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: @@ -529,6 +532,7 @@ test-ci: | clear-stalled build-addons build-js-native-api-tests build-node-api-t $(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}'`; \ @@ -1258,6 +1262,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 8a23ec2f1f398b..2ddad25616e314 100644 --- a/node.gyp +++ b/node.gyp @@ -1217,6 +1217,55 @@ ], }, # 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', ], + }, + }], + ], + }, # 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 2dd6d8beb9e17e..2468933cfd11a1 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" // This is supposed to be generated by tools/generate_code_cache.js 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..422a6f62368b6f --- /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() + '/');" + "global.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..4040763526d381 --- /dev/null +++ b/test/embedding/test.js @@ -0,0 +1,20 @@ +'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 799c9b8f663ce30ffb136cb09073d61353e6d1b9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 28 Feb 2020 00:05:57 -0800 Subject: [PATCH 21/28] fixup! src,test: add full-featured embedder API test --- node.gyp | 7 +++++++ test/embedding/test.js | 1 - 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/node.gyp b/node.gyp index 2ddad25616e314..e237612eba0307 100644 --- a/node.gyp +++ b/node.gyp @@ -1263,6 +1263,13 @@ 'OTHER_LDFLAGS': [ '-Wl,-rpath,@loader_path', ], }, }], + ['OS=="win"', { + 'libraries': [ + 'Dbghelp.lib', + 'winmm.lib', + 'Ws2_32.lib', + ], + }], ], }, # embedtest diff --git a/test/embedding/test.js b/test/embedding/test.js index 4040763526d381..a802de1849021f 100644 --- a/test/embedding/test.js +++ b/test/embedding/test.js @@ -17,4 +17,3 @@ assert.strictEqual( assert.strictEqual( child_process.spawnSync(process.execPath, ['process.exitCode = 8']).status, 8); - From ab5e2115f22bf9f139b8c3304e9f8055f49561c0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 27 Feb 2020 23:14:33 -0800 Subject: [PATCH 22/28] doc: add basic embedding example documentation --- doc/api/embedding.md | 224 +++++++++++++++++++++++++++++++++++++++++++ doc/api/index.md | 1 + 2 files changed, 225 insertions(+) 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..1ec2b145b24877 --- /dev/null +++ b/doc/api/embedding.md @@ -0,0 +1,224 @@ +# 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* have access to 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. + MaybeLocal loadenv_ret = node::LoadEnvironment( + env.get(), + "const publicRequire =" + " require('module').createRequire(process.cwd() + '/');" + "global.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. + 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 6a39e2102ce14c..ec760f5342dc3f 100644 --- a/doc/api/index.md +++ b/doc/api/index.md @@ -15,6 +15,7 @@ * [Buffer](buffer.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) From 682e804fbc29e73134703111dff6b63c47821acc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Mar 2020 13:11:48 +0100 Subject: [PATCH 23/28] test: add extended embedder cctest Add an embedder cctest that also covers a multi-Environment situation, including worker_threads-style inspector support. --- test/cctest/test_environment.cc | 141 ++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 4d8237f103a07e..23b37ee3c60ae4 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -306,3 +306,144 @@ TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) { CHECK_EQ(callback_calls, 1); CHECK_EQ(ab->ByteLength(), 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 From f191a72667d7bc7da71b3ada0783dfc7719b25a3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 16 Mar 2020 18:24:23 +0100 Subject: [PATCH 24/28] fixup! doc: add basic embedding example documentation --- doc/api/embedding.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/doc/api/embedding.md b/doc/api/embedding.md index 1ec2b145b24877..cfb35619b2613e 100644 --- a/doc/api/embedding.md +++ b/doc/api/embedding.md @@ -149,15 +149,16 @@ int RunNodeInstance(MultiIsolatePlatform* platform, // 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* have access to the - // file system, and can only load built-in Node.js modules. + // 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. + // 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() + '/');" - "global.require = publicRequire;" + "globalThis.require = publicRequire;" "require('vm').runInThisContext(process.argv[1]);"); if (loadenv_ret.IsEmpty()) // There has been a JS exception. From 3ce279a7597560000a6f5206828d0c796889908f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 16 Mar 2020 18:25:05 +0100 Subject: [PATCH 25/28] 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 422a6f62368b6f..14e2dfb802d22c 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -92,7 +92,7 @@ int RunNodeInstance(MultiIsolatePlatform* platform, env.get(), "const publicRequire =" " require('module').createRequire(process.cwd() + '/');" - "global.require = publicRequire;" + "globalThis.require = publicRequire;" "require('vm').runInThisContext(process.argv[1]);"); if (loadenv_ret.IsEmpty()) // There has been a JS exception. From f46c44c3578a08bf131524155668719478cfdd35 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 16 Mar 2020 18:25:35 +0100 Subject: [PATCH 26/28] fixup! fixup! doc: add basic embedding example documentation --- doc/api/embedding.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/embedding.md b/doc/api/embedding.md index cfb35619b2613e..7db24b325ec7ef 100644 --- a/doc/api/embedding.md +++ b/doc/api/embedding.md @@ -180,7 +180,8 @@ int RunNodeInstance(MultiIsolatePlatform* platform, more = uv_loop_alive(&loop); if (more) continue; - // node::EmitBeforeExit() is used to emit the 'beforeExit' event. + // 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 From 330f4a78feed0ad16cfe274be70cdaf017104f1f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 17 Mar 2020 13:07:15 +0100 Subject: [PATCH 27/28] squash! test: add extended embedder cctest Co-authored-by: Joyee Cheung --- Makefile | 4 ++-- test/embedding/test-embedding.js | 32 ++++++++++++++++++++++++++++++++ test/embedding/test.js | 19 ------------------- test/embedding/testcfg.py | 6 ++++++ tools/test.py | 1 + 5 files changed, 41 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 e144d8b81cec3b..71917bb6143c6d 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: @@ -532,7 +532,7 @@ test-ci: | clear-stalled build-addons build-js-native-api-tests build-node-api-t $(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/embedding/test-embedding.js b/test/embedding/test-embedding.js new file mode 100644 index 00000000000000..91d27a158b2e66 --- /dev/null +++ b/test/embedding/test-embedding.js @@ -0,0 +1,32 @@ +'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 112d5edaffaf4f..5eb1fa767219ce 100755 --- a/tools/test.py +++ b/tools/test.py @@ -1491,6 +1491,7 @@ def PrintCrashed(code): 'addons', 'benchmark', 'doctool', + 'embedding', 'internet', 'js-native-api', 'node-api', From 634a289a48c29100e23ed6bc0bad4fd7bb89269c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 17 Mar 2020 13:47:25 +0100 Subject: [PATCH 28/28] fixup! squash! test: add extended embedder cctest --- test/embedding/test-embedding.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/embedding/test-embedding.js b/test/embedding/test-embedding.js index 91d27a158b2e66..43dab0ab24ea53 100644 --- a/test/embedding/test-embedding.js +++ b/test/embedding/test-embedding.js @@ -6,7 +6,8 @@ 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 = process.features.debug ? + 'out/Debug/embedtest' : 'out/Release/embedtest'; if (common.isWindows) { binary += '.exe'; }