From e8cc269ee0107661eacc289479cfc38278a51ae4 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 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 | 3 ++- 5 files changed, 32 insertions(+), 28 deletions(-) diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 66ea96ce9d7298..da7b2745e53bbc 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -38,8 +38,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 AliasedFloat64Array // in Environment::AsyncHooks::async_ids_stack_ which tracks the resource @@ -80,6 +79,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 @@ -109,7 +109,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. @@ -130,6 +130,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 // @@ -138,6 +147,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 f5a0def438c922..7e7f4ed57236a1 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -535,11 +535,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) { @@ -617,10 +621,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( \ @@ -728,7 +728,7 @@ bool AsyncWrap::IsDoneInitializing() const { AsyncWrap::~AsyncWrap() { EmitTraceEventDestroy(); - EmitDestroy(); + EmitDestroy(true /* from gc */); } void AsyncWrap::EmitTraceEventDestroy() { @@ -778,10 +778,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()) { @@ -851,7 +854,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. @@ -890,14 +893,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 dac86d694ac28e..6004801117ba5a 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; @@ -219,7 +218,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 72ca9449236ae3..af4c73e9d3179d 100644 --- a/src/env.h +++ b/src/env.h @@ -160,8 +160,9 @@ 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