From c44edec4daff9f478eb9b5820d03da6c71253787 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 13 Nov 2019 15:54:57 +0000 Subject: [PATCH] src: provide a variant of LoadEnvironment taking a callback This allows embedders to flexibly control how they start JS code rather than using `third_party_main`. PR-URL: https://github.com/nodejs/node/pull/30467 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil --- src/api/environment.cc | 7 +++--- src/node.cc | 29 ++++++++++++++++++------ src/node.h | 15 ++++++++++--- src/node_internals.h | 5 +++-- src/node_worker.cc | 1 + test/cctest/test_environment.cc | 40 +++++++++++++++++++++++++++++---- 6 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index e1bd36fd086acb..beec7da66e9845 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -422,11 +422,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(); @@ -441,9 +442,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 21e7afe3a0de0b..9e0e2464a02f4d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -372,6 +372,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); @@ -392,17 +393,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";