Skip to content

Commit

Permalink
src: make FreeEnvironment() perform all necessary cleanup
Browse files Browse the repository at this point in the history
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.)

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
addaleax committed Sep 23, 2020
1 parent 1d3638b commit 7d92ac7
Showing 5 changed files with 38 additions and 32 deletions.
18 changes: 17 additions & 1 deletion src/api/environment.cc
Original file line number Diff line number Diff line change
@@ -342,7 +342,23 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
}

void FreeEnvironment(Environment* env) {
env->RunCleanup();
{
// TODO(addaleax): This should maybe rather be in a SealHandleScope.
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());
env->set_stopping(true);
env->stop_sub_worker_contexts();
env->RunCleanup();
RunAtExit(env);
}

// This call needs to be made while the `Environment` is still alive
// because we assume that it is available for async tracking in the
// NodePlatform implementation.
MultiIsolatePlatform* platform = env->isolate_data()->platform();
if (platform != nullptr)
platform->DrainTasks(env->isolate());

delete env;
}

18 changes: 6 additions & 12 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
@@ -110,7 +110,8 @@ int NodeMainInstance::Run() {
HandleScope handle_scope(isolate_);

int exit_code = 0;
std::unique_ptr<Environment> env = CreateMainEnvironment(&exit_code);
DeleteFnPtr<Environment, FreeEnvironment> env =
CreateMainEnvironment(&exit_code);

CHECK_NOT_NULL(env);
Context::Scope context_scope(env->context());
@@ -149,10 +150,7 @@ int NodeMainInstance::Run() {
exit_code = EmitExit(env.get());
}

env->set_can_call_into_js(false);
env->stop_sub_worker_contexts();
ResetStdio();
env->RunCleanup();

// TODO(addaleax): Neither NODE_SHARED_MODE nor HAVE_INSPECTOR really
// make sense here.
@@ -167,10 +165,6 @@ int NodeMainInstance::Run() {
}
#endif

RunAtExit(env.get());

per_process::v8_platform.DrainVMTasks(isolate_);

#if defined(LEAK_SANITIZER)
__lsan_do_leak_check();
#endif
@@ -180,8 +174,8 @@ int NodeMainInstance::Run() {

// TODO(joyeecheung): align this with the CreateEnvironment exposed in node.h
// and the environment creation routine in workers somehow.
std::unique_ptr<Environment> NodeMainInstance::CreateMainEnvironment(
int* exit_code) {
DeleteFnPtr<Environment, FreeEnvironment>
NodeMainInstance::CreateMainEnvironment(int* exit_code) {
*exit_code = 0; // Reset the exit code to 0

HandleScope handle_scope(isolate_);
@@ -205,14 +199,14 @@ std::unique_ptr<Environment> NodeMainInstance::CreateMainEnvironment(
CHECK(!context.IsEmpty());
Context::Scope context_scope(context);

std::unique_ptr<Environment> env = std::make_unique<Environment>(
DeleteFnPtr<Environment, FreeEnvironment> env { new Environment(
isolate_data_.get(),
context,
args_,
exec_args_,
static_cast<Environment::Flags>(Environment::kIsMainThread |
Environment::kOwnsProcessState |
Environment::kOwnsInspector));
Environment::kOwnsInspector)) };
env->InitializeLibuv(per_process::v8_is_profiling);
env->InitializeDiagnostics();

3 changes: 2 additions & 1 deletion src/node_main_instance.h
Original file line number Diff line number Diff line change
@@ -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<Environment> CreateMainEnvironment(int* exit_code);
DeleteFnPtr<Environment, FreeEnvironment> CreateMainEnvironment(
int* exit_code);

// If nullptr is returned, the binary is not built with embedded
// snapshot.
5 changes: 4 additions & 1 deletion src/node_platform.cc
Original file line number Diff line number Diff line change
@@ -402,6 +402,7 @@ void PerIsolatePlatformData::RunForegroundTask(uv_timer_t* handle) {

void NodePlatform::DrainTasks(Isolate* isolate) {
std::shared_ptr<PerIsolatePlatformData> per_isolate = ForIsolate(isolate);
if (!per_isolate) return;

do {
// Worker tasks aren't associated with an Isolate.
@@ -468,7 +469,9 @@ NodePlatform::ForIsolate(Isolate* isolate) {
}

bool NodePlatform::FlushForegroundTasks(Isolate* isolate) {
return ForIsolate(isolate)->FlushForegroundTasksInternal();
std::shared_ptr<PerIsolatePlatformData> per_isolate = ForIsolate(isolate);
if (!per_isolate) return false;
return per_isolate->FlushForegroundTasksInternal();
}

bool NodePlatform::IdleTasksEnabled(Isolate* isolate) { return false; }
26 changes: 9 additions & 17 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
@@ -280,26 +280,18 @@ void Worker::Run() {

if (!env_) return;
env_->set_can_call_into_js(false);
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_,
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);

{
Context::Scope context_scope(env_->context());
{
Mutex::ScopedLock lock(mutex_);
stopped_ = true;
this->env_ = nullptr;
}
env_->set_stopping(true);
env_->stop_sub_worker_contexts();
env_->RunCleanup();
RunAtExit(env_.get());

// This call needs to be made while the `Environment` is still alive
// because we assume that it is available for async tracking in the
// NodePlatform implementation.
platform_->DrainTasks(isolate_);
Mutex::ScopedLock lock(mutex_);
stopped_ = true;
this->env_ = nullptr;
}

// TODO(addaleax): Try moving DisallowJavascriptExecutionScope into
// FreeEnvironment().
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_,
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
env_.reset();
});

if (is_stopped()) return;

0 comments on commit 7d92ac7

Please sign in to comment.