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
Backport-PR-URL: #33962
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
  • Loading branch information
addaleax authored and BethGriggs committed Jun 26, 2020
1 parent 97a3f7b commit 30b0339
Show file tree
Hide file tree
Showing 6 changed files with 34 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 @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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 //

Expand All @@ -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.
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 @@ -451,11 +451,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 @@ -533,10 +537,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 @@ -632,7 +632,7 @@ bool AsyncWrap::IsDoneInitializing() const {

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

void AsyncWrap::EmitTraceEventDestroy() {
Expand Down Expand Up @@ -682,10 +682,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 @@ -755,7 +758,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 @@ -794,14 +797,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 @@ -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<v8::Object> resource_;
};

} // namespace node
Expand Down
4 changes: 3 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down

0 comments on commit 30b0339

Please sign in to comment.