Skip to content

Commit

Permalink
async_wrap: schedule destroy hook as unref
Browse files Browse the repository at this point in the history
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).

PR-URL: nodejs#18241
Fixes: nodejs#18190
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
  • Loading branch information
apapirovski authored and MayaLekova committed May 8, 2018
1 parent a84ec52 commit 739b638
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 2 deletions.
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 @@ -658,6 +658,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 @@ -778,6 +780,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 @@ -4319,6 +4319,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 @@ -4469,7 +4477,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

0 comments on commit 739b638

Please sign in to comment.