From 24700356d1d7ecae41d03fd8c6c28c9d522a3c93 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 26 Nov 2019 17:09:27 +0100 Subject: [PATCH] src: run native immediates during Environment cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/issues/30643 PR-URL: https://github.com/nodejs/node/pull/30666 Refs: https://github.com/nodejs/node/pull/30374 Reviewed-By: Rich Trott Reviewed-By: James M Snell --- src/env.cc | 8 ++++++-- src/env.h | 2 +- test/cctest/test_environment.cc | 23 +++++++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/env.cc b/src/env.cc index 161c6b04f2ec55..68199f23c7a200 100644 --- a/src/env.cc +++ b/src/env.cc @@ -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(); @@ -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; @@ -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); diff --git a/src/env.h b/src/env.h index 155cd9e8323a58..7591c7ebfd1ec0 100644 --- a/src/env.h +++ b/src/env.h @@ -1428,7 +1428,7 @@ class Environment : public MemoryRetainer { std::unique_ptr 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. diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 8bc1611330be31..a15e56a772ed19 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -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); +}