From 30b0339061cbc4d31f23b41e9ca6fa1c8861e32f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 12 Feb 2020 00:51:53 +0100 Subject: [PATCH] src: use symbol to store `AsyncWrap` resource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use a symbol on the bindings object to store the public resource object, rather than a `v8::Global` Persistent. This has several advantages: - It’s harder to inadvertently create memory leaks this way. The garbage collector sees the `AsyncWrap` → resource link like a regular JS property, and can collect the objects as a group, even if the resource object should happen to point back to the `AsyncWrap` object. - This will make it easier in the future to use `owner_symbol` for this purpose, which is generally the direction we should be moving the `async_hooks` API into (i.e. using more public objects instead of letting internal wires stick out). PR-URL: https://github.com/nodejs/node/pull/31745 Backport-PR-URL: https://github.com/nodejs/node/pull/33962 Reviewed-By: Stephen Belanger Reviewed-By: David Carlier Reviewed-By: Denys Otrishko Reviewed-By: Chengzhong Wu Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Minwoo Jung Reviewed-By: Colin Ihrig Reviewed-By: Franziska Hinkelmann Reviewed-By: Gerhard Stöbich --- lib/internal/async_hooks.js | 16 ++++++++--- src/api/callback.cc | 2 +- src/async_wrap.cc | 35 +++++++++++-------------- src/async_wrap.h | 4 +-- src/env.h | 4 ++- test/parallel/test-bootstrap-modules.js | 1 + 6 files changed, 34 insertions(+), 28 deletions(-) diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 8439260f1af1a6..62ced406bfc1e2 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -36,8 +36,7 @@ const async_wrap = internalBinding('async_wrap'); const { async_hook_fields, async_id_fields, - execution_async_resources, - owner_symbol + execution_async_resources } = async_wrap; // Store the pair executionAsyncId and triggerAsyncId in a std::stack on // Environment::AsyncHooks::async_ids_stack_ tracks the resource responsible for @@ -78,6 +77,7 @@ const active_hooks = { const { registerDestroyHook } = async_wrap; const { enqueueMicrotask } = internalBinding('task_queue'); +const { resource_symbol, owner_symbol } = internalBinding('symbols'); // Each constant tracks how many callbacks there are for any given step of // async execution. These are tracked so if the user didn't include callbacks @@ -106,7 +106,7 @@ function executionAsyncResource() { const index = async_hook_fields[kStackLength] - 1; if (index === -1) return topLevelResource; const resource = execution_async_resources[index]; - return resource; + return lookupPublicResource(resource); } // Used to fatally abort the process if a callback throws. @@ -127,6 +127,15 @@ function fatalError(e) { process.exit(1); } +function lookupPublicResource(resource) { + if (typeof resource !== 'object' || resource === null) return resource; + // TODO(addaleax): Merge this with owner_symbol and use it across all + // AsyncWrap instances. + const publicResource = resource[resource_symbol]; + if (publicResource !== undefined) + return publicResource; + return resource; +} // Emit From Native // @@ -135,6 +144,7 @@ function fatalError(e) { // emitInitScript. function emitInitNative(asyncId, type, triggerAsyncId, resource) { active_hooks.call_depth += 1; + resource = lookupPublicResource(resource); // Use a single try/catch for all hooks to avoid setting up one per iteration. try { // Using var here instead of let because "for (var ...)" is faster than let. diff --git a/src/api/callback.cc b/src/api/callback.cc index c8934e1cd33a36..a03a2587b4c796 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -36,7 +36,7 @@ CallbackScope::~CallbackScope() { InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags) : InternalCallbackScope(async_wrap->env(), - async_wrap->GetResource(), + async_wrap->object(), { async_wrap->get_async_id(), async_wrap->get_trigger_async_id() }, flags) {} diff --git a/src/async_wrap.cc b/src/async_wrap.cc index a3761b92a69130..b7c34f0121760c 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -451,11 +451,15 @@ void AsyncWrap::GetProviderType(const FunctionCallbackInfo& args) { } -void AsyncWrap::EmitDestroy() { +void AsyncWrap::EmitDestroy(bool from_gc) { AsyncWrap::EmitDestroy(env(), async_id_); // Ensure no double destroy is emitted via AsyncReset(). async_id_ = kInvalidAsyncId; - resource_.Reset(); + + if (!persistent().IsEmpty() && !from_gc) { + HandleScope handle_scope(env()->isolate()); + USE(object()->Set(env()->context(), env()->resource_symbol(), object())); + } } void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo& args) { @@ -533,10 +537,6 @@ void AsyncWrap::Initialize(Local target, env->async_ids_stack_string(), env->async_hooks()->async_ids_stack().GetJSArray()).Check(); - target->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"), - env->owner_symbol()).Check(); - Local constants = Object::New(isolate); #define SET_HOOKS_CONSTANT(name) \ FORCE_SET_TARGET_FIELD( \ @@ -632,7 +632,7 @@ bool AsyncWrap::IsDoneInitializing() const { AsyncWrap::~AsyncWrap() { EmitTraceEventDestroy(); - EmitDestroy(); + EmitDestroy(true /* from gc */); } void AsyncWrap::EmitTraceEventDestroy() { @@ -682,10 +682,13 @@ void AsyncWrap::AsyncReset(Local resource, double execution_async_id, : execution_async_id; trigger_async_id_ = env()->get_default_trigger_async_id(); - if (resource != object()) { - // TODO(addaleax): Using a strong reference here makes it very easy to - // introduce memory leaks. Move away from using a strong reference. - resource_.Reset(env()->isolate(), resource); + { + HandleScope handle_scope(env()->isolate()); + Local obj = object(); + CHECK(!obj.IsEmpty()); + if (resource != obj) { + USE(obj->Set(env()->context(), env()->resource_symbol(), resource)); + } } switch (provider_type()) { @@ -755,7 +758,7 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, ProviderType provider = provider_type(); async_context context { get_async_id(), get_trigger_async_id() }; MaybeLocal ret = InternalMakeCallback( - env(), GetResource(), object(), cb, argc, argv, context); + env(), object(), object(), cb, argc, argv, context); // This is a static call with cached values because the `this` object may // no longer be alive at this point. @@ -794,14 +797,6 @@ Local AsyncWrap::GetOwner(Environment* env, Local obj) { } } -Local AsyncWrap::GetResource() { - if (resource_.IsEmpty()) { - return object(); - } - - return resource_.Get(env()->isolate()); -} - } // namespace node NODE_MODULE_CONTEXT_AWARE_INTERNAL(async_wrap, node::AsyncWrap::Initialize) diff --git a/src/async_wrap.h b/src/async_wrap.h index 34e84fde6d4823..24bc3672fa467d 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -152,7 +152,7 @@ class AsyncWrap : public BaseObject { static void EmitAfter(Environment* env, double async_id); static void EmitPromiseResolve(Environment* env, double async_id); - void EmitDestroy(); + void EmitDestroy(bool from_gc = false); void EmitTraceEventBefore(); static void EmitTraceEventAfter(ProviderType type, double async_id); @@ -199,7 +199,6 @@ class AsyncWrap : public BaseObject { v8::Local obj); bool IsDoneInitializing() const override; - v8::Local GetResource(); private: friend class PromiseWrap; @@ -214,7 +213,6 @@ class AsyncWrap : public BaseObject { // Because the values may be Reset(), cannot be made const. double async_id_ = kInvalidAsyncId; double trigger_async_id_; - v8::Global resource_; }; } // namespace node diff --git a/src/env.h b/src/env.h index c1fda87ecd536f..6de5ba9b192dd1 100644 --- a/src/env.h +++ b/src/env.h @@ -167,8 +167,10 @@ constexpr size_t kFsStatsBufferLength = V(handle_onclose_symbol, "handle_onclose") \ V(no_message_symbol, "no_message_symbol") \ V(oninit_symbol, "oninit") \ - V(owner_symbol, "owner") \ + V(owner_symbol, "owner_symbol") \ V(onpskexchange_symbol, "onpskexchange") \ + V(resource_symbol, "resource_symbol") \ + V(trigger_async_id_symbol, "trigger_async_id_symbol") \ // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index e07ca03e807b5d..ee2a40c0a6cd79 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -24,6 +24,7 @@ const expectedModules = new Set([ 'Internal Binding process_methods', 'Internal Binding report', 'Internal Binding string_decoder', + 'Internal Binding symbols', 'Internal Binding task_queue', 'Internal Binding timers', 'Internal Binding trace_events',