Skip to content

Commit

Permalink
src: clean up embedder API
Browse files Browse the repository at this point in the history
Remove deprecated APIs (and deprecate one legacy API).

PR-URL: #35897
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
  • Loading branch information
addaleax authored and nodejs-github-bot committed Nov 2, 2020
1 parent 09af8c8 commit dfc288e
Show file tree
Hide file tree
Showing 10 changed files with 17 additions and 170 deletions.
32 changes: 2 additions & 30 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -327,18 +323,6 @@ struct InspectorParentHandleImpl : public InspectorParentHandle {
};
#endif

Environment* CreateEnvironment(IsolateData* isolate_data,
Local<Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv) {
return CreateEnvironment(
isolate_data, context,
std::vector<std::string>(argv, argv + argc),
std::vector<std::string>(exec_argv, exec_argv + exec_argc));
}

Environment* CreateEnvironment(
IsolateData* isolate_data,
Local<Context> context,
Expand Down Expand Up @@ -410,16 +394,9 @@ NODE_EXTERN std::unique_ptr<InspectorParentHandle> GetInspectorParentHandle(
#endif
}

void LoadEnvironment(Environment* env) {
USE(LoadEnvironment(env,
StartExecutionCallback{},
{}));
}

MaybeLocal<Value> LoadEnvironment(
Environment* env,
StartExecutionCallback cb,
std::unique_ptr<InspectorParentHandle> removeme) {
StartExecutionCallback cb) {
env->InitializeLibuv();
env->InitializeDiagnostics();

Expand All @@ -428,8 +405,7 @@ MaybeLocal<Value> LoadEnvironment(

MaybeLocal<Value> LoadEnvironment(
Environment* env,
const char* main_script_source_utf8,
std::unique_ptr<InspectorParentHandle> removeme) {
const char* main_script_source_utf8) {
CHECK_NOT_NULL(main_script_source_utf8);
return LoadEnvironment(
env,
Expand Down Expand Up @@ -460,10 +436,6 @@ Environment* GetCurrentEnvironment(Local<Context> context) {
return Environment::GetCurrent(context);
}

MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform() {
return per_process::v8_platform.Platform();
}

MultiIsolatePlatform* GetMultiIsolatePlatform(Environment* env) {
return GetMultiIsolatePlatform(env->isolate_data());
}
Expand Down
5 changes: 0 additions & 5 deletions src/api/hooks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 0 additions & 4 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -383,10 +383,6 @@ inline T* Environment::AddBindingData(
return item.get();
}

inline Environment* Environment::GetThreadLocalEnv() {
return static_cast<Environment*>(uv_key_get(&thread_local_env));
}

inline v8::Isolate* Environment::isolate() const {
return isolate_;
}
Expand Down
10 changes: 0 additions & 10 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -370,10 +366,6 @@ Environment::Environment(IsolateData* isolate_data,
inspector_agent_ = std::make_unique<inspector::Agent>(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<TrackingTraceStateObserver>(this);
if (TracingController* tracing_controller = writer->GetTracingController())
Expand Down Expand Up @@ -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());
Expand Down
3 changes: 0 additions & 3 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1008,9 +1008,6 @@ class Environment : public MemoryRetainer {
BaseObjectPtr<BaseObject>,
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,
Expand Down
45 changes: 0 additions & 45 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -947,51 +947,6 @@ int InitializeNodeWithArgs(std::vector<std::string>* 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<std::string> argv_(argv, argv + *argc); // NOLINT
std::vector<std::string> exec_argv_;
std::vector<std::string> 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<size_t>(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<const char*>(*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.
Expand Down
62 changes: 7 additions & 55 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<ArrayBufferAllocator> allocator,
struct uv_loop_s* event_loop,
Expand Down Expand Up @@ -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<v8::Context> context,
int argc,
const char* const* argv,
int exec_argc,
const char* const* exec_argv));
NODE_EXTERN Environment* CreateEnvironment(
IsolateData* isolate_data,
v8::Local<v8::Context> context,
Expand Down Expand Up @@ -459,18 +441,12 @@ struct StartExecutionCallbackInfo {
using StartExecutionCallback =
std::function<v8::MaybeLocal<v8::Value>(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<v8::Value> LoadEnvironment(
Environment* env,
StartExecutionCallback cb,
std::unique_ptr<InspectorParentHandle> ignored_donotuse_removeme = {});
StartExecutionCallback cb);
NODE_EXTERN v8::MaybeLocal<v8::Value> LoadEnvironment(
Environment* env,
const char* main_script_source_utf8,
std::unique_ptr<InspectorParentHandle> 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,
Expand Down Expand Up @@ -498,25 +474,17 @@ NODE_EXTERN v8::MaybeLocal<v8::Value> PrepareStackTraceCallback(
v8::Local<v8::Value> exception,
v8::Local<v8::Array> 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
Expand Down Expand Up @@ -920,29 +888,13 @@ 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.
*/
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 {
Expand Down
2 changes: 1 addition & 1 deletion src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
20 changes: 5 additions & 15 deletions test/cctest/test_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions test/cctest/test_platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) {
std::unique_ptr<node::Environment, decltype(&node::FreeEnvironment)>
environment{node::CreateEnvironment(isolate_data.get(),
context,
0, nullptr,
0, nullptr),
{},
{}),
node::FreeEnvironment};
CHECK(environment);
}
Expand Down

0 comments on commit dfc288e

Please sign in to comment.