From 8b632dff0ea2960f24c4e5f3507238df9573bd22 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 26 Nov 2019 17:00:53 +0100 Subject: [PATCH 1/4] src: keep object alive in stream_pipe code This was overlooked in a489583eda4d7cebc06516834b31dc2a4cedb1b6. Refs: https://github.com/nodejs/node/pull/30374 --- src/stream_pipe.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index 6e339378ceb374..b84c8f4c663422 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -72,7 +72,7 @@ void StreamPipe::Unpipe() { // inside the garbage collector, so we can’t run JS here. HandleScope handle_scope(env()->isolate()); BaseObjectPtr strong_ref{this}; - env()->SetImmediate([this](Environment* env) { + env()->SetImmediate([this, strong_ref](Environment* env) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local object = this->object(); From 9d455b9a71562b5db528adb20fa0aa44b303832f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 26 Nov 2019 17:05:25 +0100 Subject: [PATCH 2/4] src: add more `can_call_into_js()` guards This is in preparation for running native `SetImmediate()` callbacks during shutdown. --- src/env.cc | 5 ++++- src/node_errors.cc | 5 ++++- src/node_process_events.cc | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/env.cc b/src/env.cc index 5f9a6acb461f97..da253fac6d0b46 100644 --- a/src/env.cc +++ b/src/env.cc @@ -527,6 +527,9 @@ void Environment::RegisterHandleCleanups() { } void Environment::CleanupHandles() { + Isolate::DisallowJavascriptExecutionScope disallow_js(isolate(), + Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); + for (ReqWrapBase* request : req_wrap_queue_) request->Cancel(); @@ -661,7 +664,7 @@ void Environment::RunAndClearNativeImmediates() { head->Call(this); if (UNLIKELY(try_catch.HasCaught())) { - if (!try_catch.HasTerminated()) + if (!try_catch.HasTerminated() && can_call_into_js()) errors::TriggerUncaughtException(isolate(), try_catch); // We are done with the current callback. Move one iteration along, diff --git a/src/node_errors.cc b/src/node_errors.cc index e094fe4681fc56..17c1bd7f55c0fb 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -278,6 +278,9 @@ static void ReportFatalException(Environment* env, Local error, Local message, EnhanceFatalException enhance_stack) { + if (!env->can_call_into_js()) + enhance_stack = EnhanceFatalException::kDontEnhance; + Isolate* isolate = env->isolate(); CHECK(!error.IsEmpty()); CHECK(!message.IsEmpty()); @@ -956,7 +959,7 @@ void TriggerUncaughtException(Isolate* isolate, } MaybeLocal handled; - { + if (env->can_call_into_js()) { // We do not expect the global uncaught exception itself to throw any more // exceptions. If it does, exit the current Node.js instance. errors::TryCatchScope try_catch(env, diff --git a/src/node_process_events.cc b/src/node_process_events.cc index 440e67d412322b..06096226625bb6 100644 --- a/src/node_process_events.cc +++ b/src/node_process_events.cc @@ -34,6 +34,8 @@ Maybe ProcessEmitWarningGeneric(Environment* env, const char* warning, const char* type, const char* code) { + if (!env->can_call_into_js()) return Just(false); + HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); From 2286fc0b0f4214fcdb8b40e5f4e5aaa194102b47 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 26 Nov 2019 17:08:11 +0100 Subject: [PATCH 3/4] src: no SetImmediate from destructor in stream_pipe code Guard against running `SetImmediate()` from the destructor. The object will not be alive or usable in the callback, so it does not make sense to attempt to schedule the `SetImmediate()`. --- src/stream_pipe.cc | 6 ++++-- src/stream_pipe.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc index b84c8f4c663422..d405c4d5cbeaed 100644 --- a/src/stream_pipe.cc +++ b/src/stream_pipe.cc @@ -42,7 +42,7 @@ StreamPipe::StreamPipe(StreamBase* source, } StreamPipe::~StreamPipe() { - Unpipe(); + Unpipe(true); } StreamBase* StreamPipe::source() { @@ -53,7 +53,7 @@ StreamBase* StreamPipe::sink() { return static_cast(writable_listener_.stream()); } -void StreamPipe::Unpipe() { +void StreamPipe::Unpipe(bool is_in_deletion) { if (is_closed_) return; @@ -68,6 +68,8 @@ void StreamPipe::Unpipe() { source()->RemoveStreamListener(&readable_listener_); sink()->RemoveStreamListener(&writable_listener_); + if (is_in_deletion) return; + // Delay the JS-facing part with SetImmediate, because this might be from // inside the garbage collector, so we can’t run JS here. HandleScope handle_scope(env()->isolate()); diff --git a/src/stream_pipe.h b/src/stream_pipe.h index 061ad9842e8f6d..0e155006102eaa 100644 --- a/src/stream_pipe.h +++ b/src/stream_pipe.h @@ -12,7 +12,7 @@ class StreamPipe : public AsyncWrap { StreamPipe(StreamBase* source, StreamBase* sink, v8::Local obj); ~StreamPipe() override; - void Unpipe(); + void Unpipe(bool is_in_deletion = false); static void New(const v8::FunctionCallbackInfo& args); static void Start(const v8::FunctionCallbackInfo& args); From 64ed123f78cd34414c2fb356f4fa24d59ecb6fb7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 26 Nov 2019 17:09:27 +0100 Subject: [PATCH 4/4] 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 --- 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 da253fac6d0b46..9034c4e06971f3 100644 --- a/src/env.cc +++ b/src/env.cc @@ -530,6 +530,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(); @@ -645,7 +647,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; @@ -662,7 +664,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 495d92471a336f..78724cebc1e519 100644 --- a/src/env.h +++ b/src/env.h @@ -1415,7 +1415,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 cc9b8e4531f6ef..0db2963acc9ba3 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -185,3 +185,26 @@ static void at_exit_js(void* arg) { assert(obj->IsObject()); called_at_exit_js = true; } + +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); +}