Skip to content

Commit 4d73fd3

Browse files
committed
src: run native immediates during Environment cleanup
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 PR-URL: #30666 Refs: #30374 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 4c9db6c commit 4d73fd3

File tree

3 files changed

+30
-3
lines changed

3 files changed

+30
-3
lines changed

src/env.cc

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,8 @@ void Environment::CleanupHandles() {
530530
Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(),
531531
Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
532532

533+
RunAndClearNativeImmediates(true /* skip SetUnrefImmediate()s */);
534+
533535
for (ReqWrapBase* request : req_wrap_queue_)
534536
request->Cancel();
535537

@@ -645,7 +647,7 @@ void Environment::AtExit(void (*cb)(void* arg), void* arg) {
645647
at_exit_functions_.push_front(ExitCallback{cb, arg});
646648
}
647649

648-
void Environment::RunAndClearNativeImmediates() {
650+
void Environment::RunAndClearNativeImmediates(bool only_refed) {
649651
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
650652
"RunAndClearNativeImmediates", this);
651653
size_t ref_count = 0;
@@ -662,7 +664,9 @@ void Environment::RunAndClearNativeImmediates() {
662664
if (head->is_refed())
663665
ref_count++;
664666

665-
head->Call(this);
667+
if (head->is_refed() || !only_refed)
668+
head->Call(this);
669+
666670
if (UNLIKELY(try_catch.HasCaught())) {
667671
if (!try_catch.HasTerminated() && can_call_into_js())
668672
errors::TriggerUncaughtException(isolate(), try_catch);

src/env.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1415,7 +1415,7 @@ class Environment : public MemoryRetainer {
14151415
std::unique_ptr<NativeImmediateCallback> native_immediate_callbacks_head_;
14161416
NativeImmediateCallback* native_immediate_callbacks_tail_ = nullptr;
14171417

1418-
void RunAndClearNativeImmediates();
1418+
void RunAndClearNativeImmediates(bool only_refed = false);
14191419
static void CheckImmediate(uv_check_t* handle);
14201420

14211421
// Use an unordered_set, so that we have efficient insertion and removal.

test/cctest/test_environment.cc

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,3 +185,26 @@ static void at_exit_js(void* arg) {
185185
assert(obj->IsObject());
186186
called_at_exit_js = true;
187187
}
188+
189+
TEST_F(EnvironmentTest, SetImmediateCleanup) {
190+
int called = 0;
191+
int called_unref = 0;
192+
193+
{
194+
const v8::HandleScope handle_scope(isolate_);
195+
const Argv argv;
196+
Env env {handle_scope, argv};
197+
198+
(*env)->SetImmediate([&](node::Environment* env_arg) {
199+
EXPECT_EQ(env_arg, *env);
200+
called++;
201+
});
202+
(*env)->SetUnrefImmediate([&](node::Environment* env_arg) {
203+
EXPECT_EQ(env_arg, *env);
204+
called_unref++;
205+
});
206+
}
207+
208+
EXPECT_EQ(called, 1);
209+
EXPECT_EQ(called_unref, 0);
210+
}

0 commit comments

Comments
 (0)