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