From dd44bb92af3083a1ddf46722a48df767af7d299e 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). --- lib/internal/async_hooks.js | 16 +++++++++++++--- src/api/callback.cc | 2 +- src/async_wrap.cc | 29 ++++++++++++----------------- src/async_wrap.h | 2 -- src/env.h | 3 ++- 5 files changed, 28 insertions(+), 24 deletions(-) diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index b7a8c581cccd0b..556d6a4950eb0e 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 std::stack on // Environment::AsyncHooks::async_ids_stack_ tracks the resource responsible for @@ -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 602b0e9f5d5841..ae3e5f90e6276f 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -538,7 +538,11 @@ void AsyncWrap::EmitDestroy() { AsyncWrap::EmitDestroy(env(), async_id_); // Ensure no double destroy is emitted via AsyncReset(). async_id_ = kInvalidAsyncId; - resource_.Reset(); + + if (!persistent().IsEmpty()) { + HandleScope handle_scope(env()->isolate()); + USE(object()->Set(env()->context(), env()->resource_symbol(), object())); + } } void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo& args) { @@ -616,10 +620,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( \ @@ -777,10 +777,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()) { @@ -889,14 +892,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..f84a1d529d99b6 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -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 987006ae356c9b..9371ef296aefe8 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