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/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..9d7a1b21e6c9e9 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([](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.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_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..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 @@ -147,7 +148,8 @@ int NodeMainInstance::Run() { } env->set_trace_sync_io(false); - exit_code = EmitExit(env.get()); + if (!env->is_stopping()) env->VerifyNoStrongBaseObjects(); + exit_code = EmitProcessExit(env.get()).FromMaybe(1); } ResetStdio(); 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..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. @@ -366,8 +367,10 @@ void Worker::Run() { { int exit_code; bool stopped = is_stopped(); - if (!stopped) - exit_code = EmitExit(env_.get()); + if (!stopped) { + env_->VerifyNoStrongBaseObjects(); + exit_code = EmitProcessExit(env_.get()).FromMaybe(1); + } Mutex::ScopedLock lock(mutex_); if (exit_code_ == 0 && !stopped) exit_code_ = exit_code; @@ -718,6 +721,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/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-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 ' + 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); +}));