Skip to content

Commit

Permalink
src: run native immediates during Environment cleanup
Browse files Browse the repository at this point in the history
This can be necessary, because some parts of the Node.js code base
perform cleanup operations in the Immediate callbacks, e.g. HTTP/2.

This resolves flakiness in an HTTP/2 test that failed when a
`SetImmediate()` callback was not run or destroyed before the
`Environment` destructor started, because that callback held a
strong reference to the `Http2Session` object and the expectation
was that no such objects exist once the `Environment` constructor
starts.

Another, slightly more direct, alternative would have
been to clear the immediate queue rather than to run it. However,
this approach seems to make more sense as code generally assumes
that the `SetImmediate()` callback will always run; For example,
N-API uses an immediate callback to call buffer finalization
callbacks.

Unref’ed immediates are skipped, as the expectation is generally
that they may not run anyway.

Fixes: #30643

Backport-PR-URL: #32301
PR-URL: #30666
Refs: #30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and MylesBorins committed Apr 1, 2020
1 parent 5b65348 commit 2625244
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 3 deletions.
8 changes: 6 additions & 2 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,8 @@ void Environment::CleanupHandles() {
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(),
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);

RunAndClearNativeImmediates(true /* skip SetUnrefImmediate()s */);

for (ReqWrapBase* request : req_wrap_queue_)
request->Cancel();

Expand Down Expand Up @@ -658,7 +660,7 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) {
at_exit_functions_.push_front(ExitCallback{cb, arg});
}

void Environment::RunAndClearNativeImmediates() {
void Environment::RunAndClearNativeImmediates(bool only_refed) {
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"RunAndClearNativeImmediates", this);
size_t ref_count = 0;
Expand All @@ -675,7 +677,9 @@ void Environment::RunAndClearNativeImmediates() {
if (head->is_refed())
ref_count++;

head->Call(this);
if (head->is_refed() || !only_refed)
head->Call(this);

if (UNLIKELY(try_catch.HasCaught())) {
if (!try_catch.HasTerminated() && can_call_into_js())
errors::TriggerUncaughtException(isolate(), try_catch);
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1428,7 +1428,7 @@ class Environment : public MemoryRetainer {
std::unique_ptr<NativeImmediateCallback> native_immediate_callbacks_head_;
NativeImmediateCallback* native_immediate_callbacks_tail_ = nullptr;

void RunAndClearNativeImmediates();
void RunAndClearNativeImmediates(bool only_refed = false);
static void CheckImmediate(uv_check_t* handle);

// Use an unordered_set, so that we have efficient insertion and removal.
Expand Down
23 changes: 23 additions & 0 deletions test/cctest/test_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,3 +218,26 @@ TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) {
CHECK_EQ(callback_calls, 1);
CHECK_EQ(ab->ByteLength(), 0);
}

TEST_F(EnvironmentTest, SetImmediateCleanup) {
int called = 0;
int called_unref = 0;

{
const v8::HandleScope handle_scope(isolate_);
const Argv argv;
Env env {handle_scope, argv};

(*env)->SetImmediate([&](node::Environment* env_arg) {
EXPECT_EQ(env_arg, *env);
called++;
});
(*env)->SetUnrefImmediate([&](node::Environment* env_arg) {
EXPECT_EQ(env_arg, *env);
called_unref++;
});
}

EXPECT_EQ(called, 1);
EXPECT_EQ(called_unref, 0);
}

0 comments on commit 2625244

Please sign in to comment.