From e87a6c2d13322da99d24fa5b548da149725e0680 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 28 Mar 2020 02:46:41 +0100 Subject: [PATCH 1/2] embedding: provide hook for custom process.exit() behaviour Embedders may not want to terminate the process when `process.exit()` is called. This provides a hook for more flexible handling of that situation. Refs: https://github.com/nodejs/node/pull/30467#issuecomment-603689644 --- src/api/environment.cc | 13 +++++++++++++ src/env-inl.h | 5 +++++ src/env.cc | 9 +-------- src/env.h | 5 +++++ src/node.h | 12 ++++++++++++ src/node_worker.cc | 10 +++++++--- test/cctest/test_environment.cc | 17 +++++++++++++++++ 7 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 128bfc972a0556..e5d4b27e67c8b6 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -724,4 +724,17 @@ ThreadId AllocateEnvironmentThreadId() { return ThreadId { next_thread_id++ }; } +void DefaultProcessExitHandler(Environment* env, int exit_code) { + env->set_can_call_into_js(false); + env->stop_sub_worker_contexts(); + DisposePlatform(); + exit(exit_code); +} + + +void SetProcessExitHandler(Environment* env, + std::function&& handler) { + env->set_process_exit_handler(std::move(handler)); +} + } // namespace node diff --git a/src/env-inl.h b/src/env-inl.h index de2e68e11c8d08..270d51e35d0238 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1279,6 +1279,11 @@ void Environment::set_main_utf16(std::unique_ptr str) { main_utf16_ = std::move(str); } +void Environment::set_process_exit_handler( + std::function&& handler) { + process_exit_handler_ = std::move(handler); +} + #define VP(PropertyName, StringValue) V(v8::Private, PropertyName) #define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName) #define VS(PropertyName, StringValue) V(v8::String, PropertyName) diff --git a/src/env.cc b/src/env.cc index 3b40bd2d191adf..b61501d96bbbd1 100644 --- a/src/env.cc +++ b/src/env.cc @@ -984,14 +984,7 @@ void Environment::Exit(int exit_code) { StackTrace::CurrentStackTrace( isolate(), stack_trace_limit(), StackTrace::kDetailed)); } - if (is_main_thread()) { - set_can_call_into_js(false); - stop_sub_worker_contexts(); - DisposePlatform(); - exit(exit_code); - } else { - worker_context()->Exit(exit_code); - } + process_exit_handler_(this, exit_code); } void Environment::stop_sub_worker_contexts() { diff --git a/src/env.h b/src/env.h index 82695aa392f0ba..7ca9a364e7177a 100644 --- a/src/env.h +++ b/src/env.h @@ -1257,6 +1257,8 @@ class Environment : public MemoryRetainer { #endif // HAVE_INSPECTOR inline void set_main_utf16(std::unique_ptr); + inline void set_process_exit_handler( + std::function&& handler); private: template @@ -1459,6 +1461,9 @@ class Environment : public MemoryRetainer { int64_t base_object_count_ = 0; std::atomic_bool is_stopping_ { false }; + std::function process_exit_handler_ { + DefaultProcessExitHandler }; + template void ForEachBaseObject(T&& iterator); diff --git a/src/node.h b/src/node.h index 94ef91f5332121..4278eedfcbe09e 100644 --- a/src/node.h +++ b/src/node.h @@ -473,6 +473,18 @@ NODE_EXTERN v8::MaybeLocal LoadEnvironment( std::unique_ptr inspector_parent_handle = {}); NODE_EXTERN void FreeEnvironment(Environment* env); +// Set a callback that is called when process.exit() is called from JS, +// overriding the default handler. +// It receives the Environment* instance and the exit code as arguments. +// This could e.g. call Stop(env); in order to terminate execution and stop +// the event loop. +// The default handler disposes of the global V8 platform instance, if one is +// being used, and calls exit(). +NODE_EXTERN void SetProcessExitHandler( + Environment* env, + std::function&& handler); +NODE_EXTERN void DefaultProcessExitHandler(Environment* env, int exit_code); + // This may return nullptr if context is not associated with a Node instance. NODE_EXTERN Environment* GetCurrentEnvironment(v8::Local context); diff --git a/src/node_worker.cc b/src/node_worker.cc index 26c6ce11cef677..f81daf284dd588 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -311,6 +311,9 @@ void Worker::Run() { if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); + SetProcessExitHandler(env_.get(), [this](Environment*, int exit_code) { + Exit(exit_code); + }); } { Mutex::ScopedLock lock(mutex_); @@ -420,9 +423,10 @@ void Worker::JoinThread() { MakeCallback(env()->onexit_string(), arraysize(args), args); } - // We cleared all libuv handles bound to this Worker above, - // the C++ object is no longer needed for anything now. - MakeWeak(); + // If we get here, the !thread_joined_ condition at the top of the function + // implies that the thread was running. In that case, its final action will + // be to schedule a callback on the parent thread which will delete this + // object, so there's nothing more to do here. } Worker::~Worker() { diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 23b37ee3c60ae4..86dc8f022e0a24 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -447,3 +447,20 @@ TEST_F(EnvironmentTest, InspectorMultipleEmbeddedEnvironments) { CHECK_EQ(from_inspector->IntegerValue(context).FromJust(), 42); } #endif // HAVE_INSPECTOR + +TEST_F(EnvironmentTest, ExitHandlerTest) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + + int callback_calls = 0; + + Env env {handle_scope, argv}; + SetProcessExitHandler(*env, [&](node::Environment* env_, int exit_code) { + EXPECT_EQ(*env, env_); + EXPECT_EQ(exit_code, 42); + callback_calls++; + node::Stop(*env); + }); + node::LoadEnvironment(*env, "process.exit(42)").ToLocalChecked(); + EXPECT_EQ(callback_calls, 1); +} From 03bad86a5a867bbd4f0fa5e5b281b8b4fcd353a6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 29 Mar 2020 15:07:00 +0200 Subject: [PATCH 2/2] embedding: make Stop() stop Workers This makes sense given that terminating execution of the parent thread this way likely also is supposed to stop all running Worker threads spawned by it. --- src/api/environment.cc | 3 +-- src/env.cc | 3 ++- src/env.h | 2 +- src/node.cc | 2 +- src/node.h | 7 ++++--- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index e5d4b27e67c8b6..857506013fab8c 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -725,8 +725,7 @@ ThreadId AllocateEnvironmentThreadId() { } void DefaultProcessExitHandler(Environment* env, int exit_code) { - env->set_can_call_into_js(false); - env->stop_sub_worker_contexts(); + Stop(env); DisposePlatform(); exit(exit_code); } diff --git a/src/env.cc b/src/env.cc index b61501d96bbbd1..dfa38ea8c588b0 100644 --- a/src/env.cc +++ b/src/env.cc @@ -501,9 +501,10 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { } } -void Environment::ExitEnv() { +void Environment::Stop() { set_can_call_into_js(false); set_stopping(true); + stop_sub_worker_contexts(); isolate_->TerminateExecution(); SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); }); } diff --git a/src/env.h b/src/env.h index 7ca9a364e7177a..57ce6de91d2ff8 100644 --- a/src/env.h +++ b/src/env.h @@ -897,7 +897,7 @@ class Environment : public MemoryRetainer { void RegisterHandleCleanups(); void CleanupHandles(); void Exit(int code); - void ExitEnv(); + void Stop(); // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, diff --git a/src/node.cc b/src/node.cc index 9e0e2464a02f4d..302b277493c178 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1059,7 +1059,7 @@ int Start(int argc, char** argv) { } int Stop(Environment* env) { - env->ExitEnv(); + env->Stop(); return 0; } diff --git a/src/node.h b/src/node.h index 4278eedfcbe09e..afa2859b5b9c7f 100644 --- a/src/node.h +++ b/src/node.h @@ -224,7 +224,8 @@ class Environment; NODE_EXTERN int Start(int argc, char* argv[]); // Tear down Node.js while it is running (there are active handles -// in the loop and / or actively executing JavaScript code). +// in the loop and / or actively executing JavaScript code). This also stops +// all Workers that may have been started earlier. NODE_EXTERN int Stop(Environment* env); // TODO(addaleax): Officially deprecate this and replace it with something @@ -478,8 +479,8 @@ NODE_EXTERN void FreeEnvironment(Environment* env); // It receives the Environment* instance and the exit code as arguments. // This could e.g. call Stop(env); in order to terminate execution and stop // the event loop. -// The default handler disposes of the global V8 platform instance, if one is -// being used, and calls exit(). +// The default handler calls Stop(), disposes of the global V8 platform +// instance, if one is being used, and calls exit(). NODE_EXTERN void SetProcessExitHandler( Environment* env, std::function&& handler);