From 733d9833b10fdcd4354f3385f66d5187f27216b0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 3 Oct 2020 22:45:08 +0200 Subject: [PATCH 1/3] src: add check against non-weak BaseObjects at process exit When a process exits cleanly, i.e. because the event loop ends up without things to wait for, the Node.js objects that are left on the heap should be: 1. weak, i.e. ready for garbage collection once no longer referenced, or 2. detached, i.e. scheduled for destruction once no longer referenced, or 3. an unrefed libuv handle, i.e. does not keep the event loop alive, or 4. an inactive libuv handle (essentially the same here) There are a few exceptions to this rule, but generally, if there are C++-backed Node.js objects on the heap that do not fall into the above categories, we may be looking at a potential memory leak. Most likely, the cause is a missing `MakeWeak()` call on the corresponding object. In order to avoid this kind of problem, we check the list of BaseObjects for these criteria. In this commit, we only do so when explicitly instructed to or when in debug mode (where --verify-base-objects is always-on). In particular, this avoids the kinds of memory leak issues that were fixed in the PRs referenced below. Refs: https://github.com/nodejs/node/pull/35488 Refs: https://github.com/nodejs/node/pull/35487 Refs: https://github.com/nodejs/node/pull/35481 PR-URL: https://github.com/nodejs/node/pull/35490 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: Joyee Cheung Reviewed-By: Benjamin Gruenbaum --- src/async_wrap.cc | 6 ++++ src/base_object-inl.h | 7 ++++ src/base_object.h | 9 +++++ src/env.cc | 34 +++++++++++++++++++ src/env.h | 1 + src/handle_wrap.cc | 7 ++++ src/handle_wrap.h | 1 + src/inspector_js_api.cc | 4 +++ src/module_wrap.h | 6 ++++ src/node_contextify.h | 2 ++ src/node_http_parser.cc | 9 +++++ src/node_main_instance.cc | 1 + src/node_options.cc | 4 +++ src/node_options.h | 6 ++++ src/node_worker.cc | 10 +++++- src/node_worker.h | 1 + src/stream_base.h | 8 +++++ ...rocess-env-allowed-flags-are-documented.js | 1 + 18 files changed, 116 insertions(+), 1 deletion(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index d6511a1dd8b3c3..2dfcf96e5620ec 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -98,6 +98,12 @@ struct AsyncWrapObject : public AsyncWrap { return tmpl; } + bool IsNotIndicativeOfMemoryLeakAtExit() const override { + // We can't really know what the underlying operation does. One of the + // signs that it's time to remove this class. :) + return true; + } + SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(AsyncWrapObject) SET_SELF_SIZE(AsyncWrapObject) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 555063f4e23e67..0b620ab864f67b 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -146,6 +146,13 @@ void BaseObject::ClearWeak() { persistent_handle_.ClearWeak(); } +bool BaseObject::IsWeakOrDetached() const { + if (persistent_handle_.IsWeak()) return true; + + if (!has_pointer_data()) return false; + const PointerData* pd = const_cast(this)->pointer_data(); + return pd->wants_weak_jsobj || pd->is_detached; +} v8::Local BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) { diff --git a/src/base_object.h b/src/base_object.h index 105114caf4459a..9392e1baa14fe9 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -78,6 +78,11 @@ class BaseObject : public MemoryRetainer { // root and will not be touched by the garbage collector. inline void ClearWeak(); + // Reports whether this BaseObject is using a weak reference or detached, + // i.e. whether is can be deleted by GC once no strong BaseObjectPtrs refer + // to it anymore. + inline bool IsWeakOrDetached() const; + // Utility to create a FunctionTemplate with one internal field (used for // the `BaseObject*` pointer) and a constructor that initializes that field // to `nullptr`. @@ -147,6 +152,10 @@ class BaseObject : public MemoryRetainer { virtual v8::Maybe FinalizeTransferRead( v8::Local context, v8::ValueDeserializer* deserializer); + // Indicates whether this object is expected to use a strong reference during + // a clean process exit (due to an empty event loop). + virtual bool IsNotIndicativeOfMemoryLeakAtExit() const; + virtual inline void OnGCCollect(); private: diff --git a/src/env.cc b/src/env.cc index 7155ae67ac625b..dc67a0e5f98196 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1043,6 +1043,36 @@ void Environment::RemoveUnmanagedFd(int fd) { } } +void Environment::VerifyNoStrongBaseObjects() { + // When a process exits cleanly, i.e. because the event loop ends up without + // things to wait for, the Node.js objects that are left on the heap should + // be: + // + // 1. weak, i.e. ready for garbage collection once no longer referenced, or + // 2. detached, i.e. scheduled for destruction once no longer referenced, or + // 3. an unrefed libuv handle, i.e. does not keep the event loop alive, or + // 4. an inactive libuv handle (essentially the same here) + // + // There are a few exceptions to this rule, but generally, if there are + // C++-backed Node.js objects on the heap that do not fall into the above + // categories, we may be looking at a potential memory leak. Most likely, + // the cause is a missing MakeWeak() call on the corresponding object. + // + // In order to avoid this kind of problem, we check the list of BaseObjects + // for these criteria. Currently, we only do so when explicitly instructed to + // or when in debug mode (where --verify-base-objects is always-on). + + if (!options()->verify_base_objects) return; + + ForEachBaseObject([this](BaseObject* obj) { + if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return; + fprintf(stderr, "Found bad BaseObject during clean exit: %s\n", + obj->MemoryInfoName().c_str()); + fflush(stderr); + ABORT(); + }); +} + void Environment::BuildEmbedderGraph(Isolate* isolate, EmbedderGraph* graph, void* data) { @@ -1135,4 +1165,8 @@ Local BaseObject::GetConstructorTemplate(Environment* env) { return tmpl; } +bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const { + return IsWeakOrDetached(); +} + } // namespace node diff --git a/src/env.h b/src/env.h index 9c435f12db0e2d..512dc878049e9a 100644 --- a/src/env.h +++ b/src/env.h @@ -833,6 +833,7 @@ class Environment : public MemoryRetainer { void MemoryInfo(MemoryTracker* tracker) const override; void CreateProperties(); + void VerifyNoStrongBaseObjects(); // Should be called before InitializeInspector() void InitializeDiagnostics(); #if HAVE_INSPECTOR diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index 4e039d5dbf2d37..447c0842b90d99 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -88,6 +88,13 @@ void HandleWrap::OnGCCollect() { } +bool HandleWrap::IsNotIndicativeOfMemoryLeakAtExit() const { + return IsWeakOrDetached() || + !HandleWrap::HasRef(this) || + !uv_is_active(GetHandle()); +} + + void HandleWrap::MarkAsInitialized() { env()->handle_wrap_queue()->PushBack(this); state_ = kInitialized; diff --git a/src/handle_wrap.h b/src/handle_wrap.h index a555da9479de93..560b2e718176fa 100644 --- a/src/handle_wrap.h +++ b/src/handle_wrap.h @@ -85,6 +85,7 @@ class HandleWrap : public AsyncWrap { AsyncWrap::ProviderType provider); virtual void OnClose() {} void OnGCCollect() final; + bool IsNotIndicativeOfMemoryLeakAtExit() const override; void MarkAsInitialized(); void MarkAsUninitialized(); diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 8616575e066121..f1eb5fe7fd7448 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -155,6 +155,10 @@ class JSBindingsConnection : public AsyncWrap { SET_MEMORY_INFO_NAME(JSBindingsConnection) SET_SELF_SIZE(JSBindingsConnection) + bool IsNotIndicativeOfMemoryLeakAtExit() const override { + return true; // Binding connections emit events on their own. + } + private: std::unique_ptr session_; Global callback_; diff --git a/src/module_wrap.h b/src/module_wrap.h index 3d8bb57750397f..dc7726f11d94fd 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -60,6 +60,12 @@ class ModuleWrap : public BaseObject { SET_MEMORY_INFO_NAME(ModuleWrap) SET_SELF_SIZE(ModuleWrap) + bool IsNotIndicativeOfMemoryLeakAtExit() const override { + // XXX: The garbage collection rules for ModuleWrap are *super* unclear. + // Do these objects ever get GC'd? Are we just okay with leaking them? + return true; + } + private: ModuleWrap(Environment* env, v8::Local object, diff --git a/src/node_contextify.h b/src/node_contextify.h index 1236613c73eeb8..357029d8e18312 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -174,6 +174,8 @@ class CompiledFnEntry final : public BaseObject { v8::Local script); ~CompiledFnEntry(); + bool IsNotIndicativeOfMemoryLeakAtExit() const override { return true; } + private: uint32_t id_; v8::Global script_; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index ed92cc459a5a66..cb44320c380e02 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -880,6 +880,15 @@ class Parser : public AsyncWrap, public StreamListener { return HPE_PAUSED; } + + bool IsNotIndicativeOfMemoryLeakAtExit() const override { + // HTTP parsers are able to emit events without any GC root referring + // to them, because they receive events directly from the underlying + // libuv resource. + return true; + } + + llhttp_t parser_; StringPtr fields_[kMaxHeaderFieldsCount]; // header fields StringPtr values_[kMaxHeaderFieldsCount]; // header values diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index f638e26dba5a04..0678d1a748edf6 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -147,6 +147,7 @@ int NodeMainInstance::Run() { } env->set_trace_sync_io(false); + if (!env->is_stopping()) env->VerifyNoStrongBaseObjects(); exit_code = EmitExit(env.get()); } diff --git a/src/node_options.cc b/src/node_options.cc index 0ed8dac7217753..e57e6d46c78552 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -483,6 +483,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { "exit code 1 unless 'unhandledRejection' hook is set).", &EnvironmentOptions::unhandled_rejections, kAllowedInEnvironment); + AddOption("--verify-base-objects", + "", /* undocumented, only for debugging */ + &EnvironmentOptions::verify_base_objects, + kAllowedInEnvironment); AddOption("--check", "syntax check script without executing", diff --git a/src/node_options.h b/src/node_options.h index c9624c04a6eff1..58a21e0d4f9bd6 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -151,6 +151,12 @@ class EnvironmentOptions : public Options { bool trace_warnings = false; std::string unhandled_rejections; std::string userland_loader; + bool verify_base_objects = +#ifdef DEBUG + true; +#else + false; +#endif // DEBUG bool syntax_check_only = false; bool has_eval_string = false; diff --git a/src/node_worker.cc b/src/node_worker.cc index dd132a87cfcbaa..e2f70f6b357766 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -366,8 +366,10 @@ void Worker::Run() { { int exit_code; bool stopped = is_stopped(); - if (!stopped) + if (!stopped) { + env_->VerifyNoStrongBaseObjects(); exit_code = EmitExit(env_.get()); + } Mutex::ScopedLock lock(mutex_); if (exit_code_ == 0 && !stopped) exit_code_ = exit_code; @@ -718,6 +720,12 @@ void Worker::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("parent_port", parent_port_); } +bool Worker::IsNotIndicativeOfMemoryLeakAtExit() const { + // Worker objects always stay alive as long as the child thread, regardless + // of whether they are being referenced in the parent thread. + return true; +} + class WorkerHeapSnapshotTaker : public AsyncWrap { public: WorkerHeapSnapshotTaker(Environment* env, Local obj) diff --git a/src/node_worker.h b/src/node_worker.h index a59bdc3f5bae17..2c65f0e1a83bbe 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -50,6 +50,7 @@ class Worker : public AsyncWrap { void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Worker) SET_SELF_SIZE(Worker) + bool IsNotIndicativeOfMemoryLeakAtExit() const override; bool is_stopped() const; diff --git a/src/stream_base.h b/src/stream_base.h index e106b004753836..a5680ba8860d49 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -423,6 +423,10 @@ class SimpleShutdownWrap : public ShutdownWrap, public OtherBase { SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(SimpleShutdownWrap) SET_SELF_SIZE(SimpleShutdownWrap) + + bool IsNotIndicativeOfMemoryLeakAtExit() const override { + return OtherBase::IsNotIndicativeOfMemoryLeakAtExit(); + } }; template @@ -436,6 +440,10 @@ class SimpleWriteWrap : public WriteWrap, public OtherBase { SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(SimpleWriteWrap) SET_SELF_SIZE(SimpleWriteWrap) + + bool IsNotIndicativeOfMemoryLeakAtExit() const override { + return OtherBase::IsNotIndicativeOfMemoryLeakAtExit(); + } }; } // namespace node diff --git a/test/parallel/test-process-env-allowed-flags-are-documented.js b/test/parallel/test-process-env-allowed-flags-are-documented.js index 0e0af9471ce84e..6f4537bd3e88fa 100644 --- a/test/parallel/test-process-env-allowed-flags-are-documented.js +++ b/test/parallel/test-process-env-allowed-flags-are-documented.js @@ -88,6 +88,7 @@ assert(undocumented.delete('--experimental-report')); assert(undocumented.delete('--experimental-worker')); assert(undocumented.delete('--no-node-snapshot')); assert(undocumented.delete('--loader')); +assert(undocumented.delete('--verify-base-objects')); assert.strictEqual(undocumented.size, 0, 'The following options are not documented as allowed in ' + From 09cfd99d06aab159dc71d35acd9b6dcdd3d0bee7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 7 Oct 2020 21:25:29 +0200 Subject: [PATCH 2/3] src: fix compiler warning in env.cc ../src/env.cc:1227:22: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture] ForEachBaseObject([this](BaseObject* obj) { ^~~~ PR-URL: https://github.com/nodejs/node/pull/35547 Reviewed-By: Colin Ihrig Reviewed-By: Shelley Vohr Reviewed-By: Gus Caplan Reviewed-By: Gireesh Punathil Reviewed-By: Jiawen Geng --- src/env.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/env.cc b/src/env.cc index dc67a0e5f98196..9d7a1b21e6c9e9 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1064,7 +1064,7 @@ void Environment::VerifyNoStrongBaseObjects() { if (!options()->verify_base_objects) return; - ForEachBaseObject([this](BaseObject* obj) { + ForEachBaseObject([](BaseObject* obj) { if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return; fprintf(stderr, "Found bad BaseObject during clean exit: %s\n", obj->MemoryInfoName().c_str()); From efcc595a2ac786fb050cdc5974609801b151e79e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 3 Oct 2020 23:29:41 +0200 Subject: [PATCH 3/3] src: add maybe versions of EmitExit and EmitBeforeExit This addresses a TODO comment, and removes invalid `.ToLocalChecked()` calls from our code base. PR-URL: https://github.com/nodejs/node/pull/35486 Reviewed-By: James M Snell --- doc/api/embedding.md | 11 ++-- src/api/hooks.cc | 55 ++++++++++++------- src/node.h | 15 ++++- src/node_main_instance.cc | 5 +- src/node_worker.cc | 5 +- test/embedding/embedtest.cc | 6 +- .../test-process-beforeexit-throw-exit.js | 12 ++++ .../test-worker-beforeexit-throw-exit.js | 28 ++++++++++ 8 files changed, 105 insertions(+), 32 deletions(-) create mode 100644 test/parallel/test-process-beforeexit-throw-exit.js create mode 100644 test/parallel/test-worker-beforeexit-throw-exit.js diff --git a/doc/api/embedding.md b/doc/api/embedding.md index 7ec22fbab63850..7dc4a2db7fca63 100644 --- a/doc/api/embedding.md +++ b/doc/api/embedding.md @@ -181,9 +181,10 @@ int RunNodeInstance(MultiIsolatePlatform* platform, more = uv_loop_alive(&loop); if (more) continue; - // node::EmitBeforeExit() is used to emit the 'beforeExit' event on - // the `process` object. - node::EmitBeforeExit(env.get()); + // node::EmitProcessBeforeExit() is used to emit the 'beforeExit' event + // on the `process` object. + if (node::EmitProcessBeforeExit(env.get()).IsNothing()) + break; // 'beforeExit' can also schedule new work that keeps the event loop // running. @@ -191,8 +192,8 @@ int RunNodeInstance(MultiIsolatePlatform* platform, } while (more == true); } - // node::EmitExit() returns the current exit code. - exit_code = node::EmitExit(env.get()); + // node::EmitProcessExit() returns the current exit code. + exit_code = node::EmitProcessExit(env.get()).FromMaybe(1); // node::Stop() can be used to explicitly stop the event loop and keep // further JavaScript from running. It can be called from any thread, diff --git a/src/api/hooks.cc b/src/api/hooks.cc index a719a861dbe9d8..84c91a2100b156 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -9,8 +9,11 @@ using v8::Context; using v8::HandleScope; using v8::Integer; using v8::Isolate; +using v8::Just; using v8::Local; +using v8::Maybe; using v8::NewStringType; +using v8::Nothing; using v8::Object; using v8::String; using v8::Value; @@ -30,6 +33,10 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) { } void EmitBeforeExit(Environment* env) { + USE(EmitProcessBeforeExit(env)); +} + +Maybe EmitProcessBeforeExit(Environment* env) { TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment), "BeforeExit", env); if (!env->destroy_async_id_list()->empty()) @@ -40,39 +47,49 @@ void EmitBeforeExit(Environment* env) { Local exit_code_v; if (!env->process_object()->Get(env->context(), env->exit_code_string()) - .ToLocal(&exit_code_v)) return; + .ToLocal(&exit_code_v)) return Nothing(); Local exit_code; - if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) return; + if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) { + return Nothing(); + } - // TODO(addaleax): Provide variants of EmitExit() and EmitBeforeExit() that - // actually forward empty MaybeLocal<>s (and check env->can_call_into_js()). - USE(ProcessEmit(env, "beforeExit", exit_code)); + return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ? + Nothing() : Just(true); } int EmitExit(Environment* env) { + return EmitProcessExit(env).FromMaybe(1); +} + +Maybe EmitProcessExit(Environment* env) { // process.emit('exit') HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); Local process_object = env->process_object(); - process_object + + // TODO(addaleax): It might be nice to share process._exiting and + // process.exitCode via getter/setter pairs that pass data directly to the + // native side, so that we don't manually have to read and write JS properties + // here. These getters could use e.g. a typed array for performance. + if (process_object ->Set(env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "_exiting"), - True(env->isolate())) - .Check(); + True(env->isolate())).IsNothing()) return Nothing(); Local exit_code = env->exit_code_string(); - int code = process_object->Get(env->context(), exit_code) - .ToLocalChecked() - ->Int32Value(env->context()) - .ToChecked(); - ProcessEmit(env, "exit", Integer::New(env->isolate(), code)); - - // Reload exit code, it may be changed by `emit('exit')` - return process_object->Get(env->context(), exit_code) - .ToLocalChecked() - ->Int32Value(env->context()) - .ToChecked(); + Local code_v; + int code; + if (!process_object->Get(env->context(), exit_code).ToLocal(&code_v) || + !code_v->Int32Value(env->context()).To(&code) || + ProcessEmit(env, "exit", Integer::New(env->isolate(), code)).IsEmpty() || + // Reload exit code, it may be changed by `emit('exit')` + !process_object->Get(env->context(), exit_code).ToLocal(&code_v) || + !code_v->Int32Value(env->context()).To(&code)) { + return Nothing(); + } + + return Just(code); } typedef void (*CleanupHook)(void* arg); diff --git a/src/node.h b/src/node.h index 38e0ef50f9b283..b3412d151b4819 100644 --- a/src/node.h +++ b/src/node.h @@ -539,8 +539,19 @@ NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform); NODE_EXTERN v8::TracingController* GetTracingController(); NODE_EXTERN void SetTracingController(v8::TracingController* controller); -NODE_EXTERN void EmitBeforeExit(Environment* env); -NODE_EXTERN int EmitExit(Environment* env); +// Run `process.emit('beforeExit')` as it would usually happen when Node.js is +// run in standalone mode. +NODE_EXTERN v8::Maybe EmitProcessBeforeExit(Environment* env); +NODE_DEPRECATED("Use Maybe version (EmitProcessBeforeExit) instead", + NODE_EXTERN void EmitBeforeExit(Environment* env)); +// Run `process.emit('exit')` as it would usually happen when Node.js is run +// in standalone mode. The return value corresponds to the exit code. +NODE_EXTERN v8::Maybe EmitProcessExit(Environment* env); +NODE_DEPRECATED("Use Maybe version (EmitProcessExit) instead", + NODE_EXTERN int EmitExit(Environment* env)); + +// Runs hooks added through `AtExit()`. This is part of `FreeEnvironment()`, +// so calling it manually is typically not necessary. NODE_EXTERN void RunAtExit(Environment* env); // This may return nullptr if the current v8::Context is not associated diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 0678d1a748edf6..280ccaab5ef89c 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -135,7 +135,8 @@ int NodeMainInstance::Run() { if (more && !env->is_stopping()) continue; if (!uv_loop_alive(env->event_loop())) { - EmitBeforeExit(env.get()); + if (EmitProcessBeforeExit(env.get()).IsNothing()) + break; } // Emit `beforeExit` if the loop became alive either after emitting @@ -148,7 +149,7 @@ int NodeMainInstance::Run() { env->set_trace_sync_io(false); if (!env->is_stopping()) env->VerifyNoStrongBaseObjects(); - exit_code = EmitExit(env.get()); + exit_code = EmitProcessExit(env.get()).FromMaybe(1); } ResetStdio(); diff --git a/src/node_worker.cc b/src/node_worker.cc index e2f70f6b357766..8058c4e9caf3da 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -352,7 +352,8 @@ void Worker::Run() { more = uv_loop_alive(&data.loop_); if (more && !is_stopped()) continue; - EmitBeforeExit(env_.get()); + if (EmitProcessBeforeExit(env_.get()).IsNothing()) + break; // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. @@ -368,7 +369,7 @@ void Worker::Run() { bool stopped = is_stopped(); if (!stopped) { env_->VerifyNoStrongBaseObjects(); - exit_code = EmitExit(env_.get()); + exit_code = EmitProcessExit(env_.get()).FromMaybe(1); } Mutex::ScopedLock lock(mutex_); if (exit_code_ == 0 && !stopped) diff --git a/test/embedding/embedtest.cc b/test/embedding/embedtest.cc index 21baadf93e5a26..fece8924ad6471 100644 --- a/test/embedding/embedtest.cc +++ b/test/embedding/embedtest.cc @@ -110,12 +110,14 @@ int RunNodeInstance(MultiIsolatePlatform* platform, more = uv_loop_alive(&loop); if (more) continue; - node::EmitBeforeExit(env.get()); + if (node::EmitProcessBeforeExit(env.get()).IsNothing()) + break; + more = uv_loop_alive(&loop); } while (more == true); } - exit_code = node::EmitExit(env.get()); + exit_code = node::EmitProcessExit(env.get()).FromMaybe(1); node::Stop(env.get()); } diff --git a/test/parallel/test-process-beforeexit-throw-exit.js b/test/parallel/test-process-beforeexit-throw-exit.js new file mode 100644 index 00000000000000..6e9d764be90baa --- /dev/null +++ b/test/parallel/test-process-beforeexit-throw-exit.js @@ -0,0 +1,12 @@ +'use strict'; +const common = require('../common'); +common.skipIfWorker(); + +// Test that 'exit' is emitted if 'beforeExit' throws. + +process.on('exit', common.mustCall(() => { + process.exitCode = 0; +})); +process.on('beforeExit', common.mustCall(() => { + throw new Error(); +})); diff --git a/test/parallel/test-worker-beforeexit-throw-exit.js b/test/parallel/test-worker-beforeexit-throw-exit.js new file mode 100644 index 00000000000000..2aa255ee82af8a --- /dev/null +++ b/test/parallel/test-worker-beforeexit-throw-exit.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +// Test that 'exit' is emitted if 'beforeExit' throws, both inside the Worker. + +const workerData = new Uint8Array(new SharedArrayBuffer(2)); +const w = new Worker(` + const { workerData } = require('worker_threads'); + process.on('exit', () => { + workerData[0] = 100; + }); + process.on('beforeExit', () => { + workerData[1] = 200; + throw new Error('banana'); + }); +`, { eval: true, workerData }); + +w.on('error', common.mustCall((err) => { + assert.strictEqual(err.message, 'banana'); +})); + +w.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 1); + assert.strictEqual(workerData[0], 100); + assert.strictEqual(workerData[1], 200); +}));