Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
async_wrap: schedule destroy hook as unref
Since the `DestroyAsyncIdsCallback` in Node.js can be scheduled as
a result of GC, that means that it can accidentally keep the event
loop open when it shouldn't.

Replace `SetImmediate` with the newly introduced `SetUnrefImmediate`
and in addition introduce RunBeforeExit callbacks, of which
`DestroyAsyncIdsCallback` is now the first. These callbacks will run
before the `beforeExit` event is emitted (which will now only be
emitted if the event loop is still not active).
  • Loading branch information
apapirovski committed Jan 20, 2018
commit 002937f34d6a38b1c182e268ab3bb6b7d5cd9b1b
10 changes: 9 additions & 1 deletion src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,12 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) {
} while (!env->destroy_async_id_list()->empty());
}

static void DestroyAsyncIdsCallback(void* arg) {
Environment* env = static_cast<Environment*>(arg);
if (!env->destroy_async_id_list()->empty())
DestroyAsyncIdsCallback(env, nullptr);
}


void AsyncWrap::EmitPromiseResolve(Environment* env, double async_id) {
AsyncHooks* async_hooks = env->async_hooks();
Expand Down Expand Up @@ -502,6 +508,8 @@ void AsyncWrap::Initialize(Local<Object> target,
Isolate* isolate = env->isolate();
HandleScope scope(isolate);

env->BeforeExit(DestroyAsyncIdsCallback, env);

env->SetMethod(target, "setupHooks", SetupHooks);
env->SetMethod(target, "pushAsyncIds", PushAsyncIds);
env->SetMethod(target, "popAsyncIds", PopAsyncIds);
Expand Down Expand Up @@ -663,7 +671,7 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
return;

if (env->destroy_async_id_list()->empty()) {
env->SetImmediate(DestroyAsyncIdsCallback, nullptr);
env->SetUnrefImmediate(DestroyAsyncIdsCallback, nullptr);
}

env->destroy_async_id_list()->push_back(async_id);
Expand Down
11 changes: 11 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,17 @@ void Environment::PrintSyncTrace() const {
fflush(stderr);
}

void Environment::RunBeforeExitCallbacks() {
for (BeforeExitCallback before_exit : before_exit_functions_) {
before_exit.cb_(before_exit.arg_);
}
before_exit_functions_.clear();
}

void Environment::BeforeExit(void (*cb)(void* arg), void* arg) {
before_exit_functions_.push_back(BeforeExitCallback{cb, arg});
}

void Environment::RunAtExitCallbacks() {
for (AtExitCallback at_exit : at_exit_functions_) {
at_exit.cb_(at_exit.arg_);
Expand Down
8 changes: 8 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,8 @@ class Environment {
const char* name,
v8::FunctionCallback callback);

void BeforeExit(void (*cb)(void* arg), void* arg);
void RunBeforeExitCallbacks();
void AtExit(void (*cb)(void* arg), void* arg);
void RunAtExitCallbacks();

Expand Down Expand Up @@ -773,6 +775,12 @@ class Environment {

double* fs_stats_field_array_;

struct BeforeExitCallback {
void (*cb_)(void* arg);
void* arg_;
};
std::list<BeforeExitCallback> before_exit_functions_;

struct AtExitCallback {
void (*cb_)(void* arg);
void* arg_;
Expand Down
10 changes: 9 additions & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4317,6 +4317,14 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) {
}


void RunBeforeExit(Environment* env) {
env->RunBeforeExitCallbacks();

if (!uv_loop_alive(env->event_loop()))
EmitBeforeExit(env);
}


void EmitBeforeExit(Environment* env) {
HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());
Expand Down Expand Up @@ -4467,7 +4475,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
if (more)
continue;

EmitBeforeExit(&env);
RunBeforeExit(&env);

// Emit `beforeExit` if the loop became alive either after emitting
// event, or after running some callbacks.
Expand Down