From 4fb86404e550069eecf45f4ef9a3074098048010 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 31 Oct 2020 13:31:38 +0100 Subject: [PATCH] src: clean up embedder API Remove deprecated APIs (and deprecate one legacy API). --- src/api/environment.cc | 32 ++--------------- src/api/hooks.cc | 5 --- src/env-inl.h | 4 --- src/env.cc | 10 ------ src/env.h | 3 -- src/node.cc | 45 ------------------------ src/node.h | 62 ++++----------------------------- src/node_main_instance.cc | 2 +- test/cctest/test_environment.cc | 20 +++-------- test/cctest/test_platform.cc | 4 +-- 10 files changed, 17 insertions(+), 170 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index fa994bc41ec9bc..68395a61675b09 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -264,10 +264,6 @@ void SetIsolateUpForNode(v8::Isolate* isolate) { SetIsolateUpForNode(isolate, settings); } -Isolate* NewIsolate(ArrayBufferAllocator* allocator, uv_loop_t* event_loop) { - return NewIsolate(allocator, event_loop, GetMainThreadMultiIsolatePlatform()); -} - // TODO(joyeecheung): we may want to expose this, but then we need to be // careful about what we override in the params. Isolate* NewIsolate(Isolate::CreateParams* params, @@ -327,18 +323,6 @@ struct InspectorParentHandleImpl : public InspectorParentHandle { }; #endif -Environment* CreateEnvironment(IsolateData* isolate_data, - Local context, - int argc, - 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, @@ -410,16 +394,9 @@ NODE_EXTERN std::unique_ptr GetInspectorParentHandle( #endif } -void LoadEnvironment(Environment* env) { - USE(LoadEnvironment(env, - StartExecutionCallback{}, - {})); -} - MaybeLocal LoadEnvironment( Environment* env, - StartExecutionCallback cb, - std::unique_ptr removeme) { + StartExecutionCallback cb) { env->InitializeLibuv(); env->InitializeDiagnostics(); @@ -428,8 +405,7 @@ MaybeLocal LoadEnvironment( MaybeLocal LoadEnvironment( Environment* env, - const char* main_script_source_utf8, - std::unique_ptr removeme) { + const char* main_script_source_utf8) { CHECK_NOT_NULL(main_script_source_utf8); return LoadEnvironment( env, @@ -460,10 +436,6 @@ Environment* GetCurrentEnvironment(Local context) { return Environment::GetCurrent(context); } -MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform() { - return per_process::v8_platform.Platform(); -} - MultiIsolatePlatform* GetMultiIsolatePlatform(Environment* env) { return GetMultiIsolatePlatform(env->isolate_data()); } diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 84c91a2100b156..f3f685b167f3cd 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -22,11 +22,6 @@ void RunAtExit(Environment* env) { env->RunAtExitCallbacks(); } -void AtExit(void (*cb)(void* arg), void* arg) { - auto env = Environment::GetThreadLocalEnv(); - AtExit(env, cb, arg); -} - void AtExit(Environment* env, void (*cb)(void* arg), void* arg) { CHECK_NOT_NULL(env); env->AtExit(cb, arg); diff --git a/src/env-inl.h b/src/env-inl.h index 7e7f4b3808a3f9..3dd0e372ecab18 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -383,10 +383,6 @@ inline T* Environment::AddBindingData( return item.get(); } -inline Environment* Environment::GetThreadLocalEnv() { - return static_cast(uv_key_get(&thread_local_env)); -} - inline v8::Isolate* Environment::isolate() const { return isolate_; } diff --git a/src/env.cc b/src/env.cc index ba9ac1e31ffe92..eef024ab450097 100644 --- a/src/env.cc +++ b/src/env.cc @@ -227,10 +227,6 @@ void IsolateData::MemoryInfo(MemoryTracker* tracker) const { // TODO(joyeecheung): implement MemoryRetainer in the option classes. } -void InitThreadLocalOnce() { - CHECK_EQ(0, uv_key_create(&Environment::thread_local_env)); -} - void TrackingTraceStateObserver::UpdateTraceCategoryState() { if (!env_->owns_process_state() || !env_->can_call_into_js()) { // Ideally, we’d have a consistent story that treats all threads/Environment @@ -370,10 +366,6 @@ Environment::Environment(IsolateData* isolate_data, inspector_agent_ = std::make_unique(this); #endif - 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()) @@ -1143,8 +1135,6 @@ void AsyncHooks::grow_async_ids_stack() { async_ids_stack_.GetJSArray()).Check(); } -uv_key_t Environment::thread_local_env = {}; - void Environment::Exit(int exit_code) { if (options()->trace_exit) { HandleScope handle_scope(isolate()); diff --git a/src/env.h b/src/env.h index b3797e2c7e5880..0af1cd3fa58737 100644 --- a/src/env.h +++ b/src/env.h @@ -1008,9 +1008,6 @@ class Environment : public MemoryRetainer { BaseObjectPtr, FastStringKey::Hash> BindingDataStore; - static uv_key_t thread_local_env; - static inline Environment* GetThreadLocalEnv(); - // Create an Environment without initializing a main Context. Use // InitializeMainContext() to initialize a main context for it. Environment(IsolateData* isolate_data, diff --git a/src/node.cc b/src/node.cc index efb4876002f774..c3f423cb579479 100644 --- a/src/node.cc +++ b/src/node.cc @@ -947,51 +947,6 @@ int InitializeNodeWithArgs(std::vector* argv, return 0; } -// TODO(addaleax): Deprecate and eventually remove this. -void Init(int* argc, - const char** argv, - int* exec_argc, - const char*** exec_argv) { - std::vector argv_(argv, argv + *argc); // NOLINT - std::vector exec_argv_; - std::vector errors; - - // This (approximately) duplicates some logic that has been moved to - // node::Start(), with the difference that here we explicitly call `exit()`. - int exit_code = InitializeNodeWithArgs(&argv_, &exec_argv_, &errors); - - for (const std::string& error : errors) - fprintf(stderr, "%s: %s\n", argv_.at(0).c_str(), error.c_str()); - if (exit_code != 0) exit(exit_code); - - if (per_process::cli_options->print_version) { - printf("%s\n", NODE_VERSION); - exit(0); - } - - if (per_process::cli_options->print_bash_completion) { - std::string completion = options_parser::GetBashCompletion(); - printf("%s\n", completion.c_str()); - exit(0); - } - - if (per_process::cli_options->print_v8_help) { - V8::SetFlagsFromString("--help", static_cast(6)); - exit(0); - } - - *argc = argv_.size(); - *exec_argc = exec_argv_.size(); - // These leak memory, because, in the original code of this function, no - // extra allocations were visible. This should be okay because this function - // is only supposed to be called once per process, though. - *exec_argv = Malloc(*exec_argc); - for (int i = 0; i < *exec_argc; ++i) - (*exec_argv)[i] = strdup(exec_argv_[i].c_str()); - for (int i = 0; i < *argc; ++i) - argv[i] = strdup(argv_[i].c_str()); -} - InitializationResult InitializeOncePerProcess(int argc, char** argv) { // Initialized the enabled list for Debug() calls with system // environment variables. diff --git a/src/node.h b/src/node.h index 0dc2de975b811d..0286655c7ada89 100644 --- a/src/node.h +++ b/src/node.h @@ -222,14 +222,6 @@ NODE_EXTERN int Start(int argc, char* argv[]); // in the loop and / or actively executing JavaScript code). NODE_EXTERN int Stop(Environment* env); -// 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_DEPRECATED("Use InitializeNodeWithArgs() instead", - 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 @@ -357,11 +349,9 @@ NODE_EXTERN void SetIsolateUpForNode(v8::Isolate* isolate); // This is a convenience method equivalent to using SetIsolateCreateParams(), // Isolate::Allocate(), MultiIsolatePlatform::RegisterIsolate(), // Isolate::Initialize(), and SetIsolateUpForNode(). -NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator, - struct uv_loop_s* event_loop); NODE_EXTERN v8::Isolate* NewIsolate(ArrayBufferAllocator* allocator, struct uv_loop_s* event_loop, - MultiIsolatePlatform* platform); + MultiIsolatePlatform* platform = nullptr); NODE_EXTERN v8::Isolate* NewIsolate( std::shared_ptr allocator, struct uv_loop_s* event_loop, @@ -422,14 +412,6 @@ struct InspectorParentHandle { // TODO(addaleax): Maybe move per-Environment options parsing here. // Returns nullptr when the Environment cannot be created e.g. there are // pending JavaScript exceptions. -// It is recommended to use the second variant taking a flags argument. -NODE_DEPRECATED("Use overload 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, @@ -459,18 +441,12 @@ struct StartExecutionCallbackInfo { using StartExecutionCallback = std::function(const StartExecutionCallbackInfo&)>; -NODE_DEPRECATED("Use variants returning MaybeLocal<> instead", - NODE_EXTERN void LoadEnvironment(Environment* env)); -// The `InspectorParentHandle` arguments here are ignored and not used. -// For passing `InspectorParentHandle`, use `CreateEnvironment()`. NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, - StartExecutionCallback cb, - std::unique_ptr ignored_donotuse_removeme = {}); + StartExecutionCallback cb); NODE_EXTERN v8::MaybeLocal LoadEnvironment( Environment* env, - const char* main_script_source_utf8, - std::unique_ptr ignored_donotuse_removeme = {}); + const char* main_script_source_utf8); NODE_EXTERN void FreeEnvironment(Environment* env); // Set a callback that is called when process.exit() is called from JS, @@ -498,25 +474,17 @@ NODE_EXTERN v8::MaybeLocal PrepareStackTraceCallback( v8::Local exception, v8::Local trace); -// This returns the MultiIsolatePlatform used in the main thread of Node.js. -// If NODE_USE_V8_PLATFORM has not been defined when Node.js was built, -// it returns nullptr. -NODE_DEPRECATED("Use GetMultiIsolatePlatform(env) instead", - 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_DEPRECATED("Use variant taking a v8::TracingController* pointer instead", +NODE_DEPRECATED("Use MultiIsolatePlatform::Create() instead", 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); + v8::TracingController* tracing_controller)); +NODE_DEPRECATED("Use MultiIsolatePlatform::Create() instead", + NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform)); // Get/set the currently active tracing controller. Using CreatePlatform() // will implicitly set this by default. This is global and should be initialized @@ -920,16 +888,6 @@ NODE_EXTERN void AddLinkedBinding(Environment* env, addon_context_register_func fn, void* priv); -/* Called after the event loop exits but before the VM is disposed. - * Callbacks are run in reverse order of registration, i.e. newest first. - * - * You should always use the three-argument variant (or, for addons, - * AddEnvironmentCleanupHook) in order to avoid relying on global state. - */ -NODE_DEPRECATED( - "Use the three-argument variant of AtExit() or AddEnvironmentCleanupHook()", - NODE_EXTERN void AtExit(void (*cb)(void* arg), void* arg = nullptr)); - /* Registers a callback with the passed-in Environment instance. The callback * is called after the event loop exits, but before the VM is disposed. * Callbacks are run in reverse order of registration, i.e. newest first. @@ -937,12 +895,6 @@ NODE_DEPRECATED( NODE_EXTERN void AtExit(Environment* env, void (*cb)(void* arg), void* arg); -NODE_DEPRECATED( - "Use the three-argument variant of AtExit() or AddEnvironmentCleanupHook()", - inline void AtExit(Environment* env, - void (*cb)(void* arg)) { - AtExit(env, cb, nullptr); - }) typedef double async_id; struct async_context { diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 84d39831dcd1f7..07277ccfa5c167 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -139,7 +139,7 @@ int NodeMainInstance::Run(const EnvSerializeInfo* env_info) { Context::Scope context_scope(env->context()); if (exit_code == 0) { - LoadEnvironment(env.get()); + LoadEnvironment(env.get(), StartExecutionCallback{}); exit_code = SpinEventLoop(env.get()).FromMaybe(1); } diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 27706044b800f6..a6b27e9871cd81 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -182,17 +182,7 @@ TEST_F(EnvironmentTest, AtExitWithEnvironment) { const Argv argv; Env env {handle_scope, argv}; - AtExit(*env, at_exit_callback1); - RunAtExit(*env); - EXPECT_TRUE(called_cb_1); -} - -TEST_F(EnvironmentTest, AtExitWithoutEnvironment) { - const v8::HandleScope handle_scope(isolate_); - const Argv argv; - Env env {handle_scope, argv}; - - AtExit(at_exit_callback1); // No Environment is passed to AtExit. + AtExit(*env, at_exit_callback1, nullptr); RunAtExit(*env); EXPECT_TRUE(called_cb_1); } @@ -203,8 +193,8 @@ TEST_F(EnvironmentTest, AtExitOrder) { Env env {handle_scope, argv}; // Test that callbacks are run in reverse order. - AtExit(*env, at_exit_callback_ordered1); - AtExit(*env, at_exit_callback_ordered2); + AtExit(*env, at_exit_callback_ordered1, nullptr); + AtExit(*env, at_exit_callback_ordered2, nullptr); RunAtExit(*env); EXPECT_TRUE(called_cb_ordered_1); EXPECT_TRUE(called_cb_ordered_2); @@ -239,8 +229,8 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { Env env1 {handle_scope, argv}; Env env2 {handle_scope, argv, node::EnvironmentFlags::kNoFlags}; - AtExit(*env1, at_exit_callback1); - AtExit(*env2, at_exit_callback2); + AtExit(*env1, at_exit_callback1, nullptr); + AtExit(*env2, at_exit_callback2, nullptr); RunAtExit(*env1); EXPECT_TRUE(called_cb_1); EXPECT_FALSE(called_cb_2); diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index 07bea57b314bf0..c2d78938130006 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -93,8 +93,8 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { std::unique_ptr environment{node::CreateEnvironment(isolate_data.get(), context, - 0, nullptr, - 0, nullptr), + {}, + {}), node::FreeEnvironment}; CHECK(environment); }