Skip to content

Commit

Permalink
src: use symbol to store AsyncWrap resource
Browse files Browse the repository at this point in the history
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: #31745
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
  • Loading branch information
addaleax authored and codebytere committed Jun 18, 2020
1 parent 3f971d8 commit e8cc269
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 28 deletions.
16 changes: 13 additions & 3 deletions lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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 //

Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}
Expand Down
35 changes: 15 additions & 20 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -535,11 +535,15 @@ void AsyncWrap::GetProviderType(const FunctionCallbackInfo<Value>& 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<Value>& args) {
Expand Down Expand Up @@ -617,10 +621,6 @@ void AsyncWrap::Initialize(Local<Object> 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<Object> constants = Object::New(isolate);
#define SET_HOOKS_CONSTANT(name) \
FORCE_SET_TARGET_FIELD( \
Expand Down Expand Up @@ -728,7 +728,7 @@ bool AsyncWrap::IsDoneInitializing() const {

AsyncWrap::~AsyncWrap() {
EmitTraceEventDestroy();
EmitDestroy();
EmitDestroy(true /* from gc */);
}

void AsyncWrap::EmitTraceEventDestroy() {
Expand Down Expand Up @@ -778,10 +778,13 @@ void AsyncWrap::AsyncReset(Local<Object> 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<Object> obj = object();
CHECK(!obj.IsEmpty());
if (resource != obj) {
USE(obj->Set(env()->context(), env()->resource_symbol(), resource));
}
}

switch (provider_type()) {
Expand Down Expand Up @@ -851,7 +854,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
ProviderType provider = provider_type();
async_context context { get_async_id(), get_trigger_async_id() };
MaybeLocal<Value> 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.
Expand Down Expand Up @@ -890,14 +893,6 @@ Local<Object> AsyncWrap::GetOwner(Environment* env, Local<Object> obj) {
}
}

Local<Object> AsyncWrap::GetResource() {
if (resource_.IsEmpty()) {
return object();
}

return resource_.Get(env()->isolate());
}

} // namespace node

NODE_MODULE_CONTEXT_AWARE_INTERNAL(async_wrap, node::AsyncWrap::Initialize)
4 changes: 1 addition & 3 deletions src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -199,7 +199,6 @@ class AsyncWrap : public BaseObject {
v8::Local<v8::Object> obj);

bool IsDoneInitializing() const override;
v8::Local<v8::Object> GetResource();

private:
friend class PromiseWrap;
Expand All @@ -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<v8::Object> resource_;
};

} // namespace node
Expand Down
3 changes: 2 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit e8cc269

Please sign in to comment.