Skip to content

Commit

Permalink
src: do not include partial AsyncWrap instances in heap dump
Browse files Browse the repository at this point in the history
Heap dumps can be taken either through the inspector or the public API
for it during an async_hooks init() hook, but at that point the
AsyncWrap in question is not done initializing yet and virtual methods
cannot be called on it.

Address this issue (somewhat hackily) by excluding `AsyncWrap`
instances which have not yet executed their `init()` hook fully
from heap dumps.

Fixes: nodejs#28786

PR-URL: nodejs#28789
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
addaleax authored and Trott committed Jul 22, 2019
1 parent 89e4b36 commit 9949fbd
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 10 deletions.
36 changes: 29 additions & 7 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -577,22 +577,44 @@ AsyncWrap::AsyncWrap(Environment* env,
ProviderType provider,
double execution_async_id,
bool silent)
: BaseObject(env, object),
provider_type_(provider) {
: AsyncWrap(env, object) {
CHECK_NE(provider, PROVIDER_NONE);
CHECK_GE(object->InternalFieldCount(), 1);
provider_type_ = provider;

// Use AsyncReset() call to execute the init() callbacks.
AsyncReset(execution_async_id, silent);
init_hook_ran_ = true;
}

AsyncWrap::AsyncWrap(Environment* env, Local<Object> object)
: BaseObject(env, object),
provider_type_(PROVIDER_NONE) {
CHECK_GE(object->InternalFieldCount(), 1);
: BaseObject(env, object) {
}

// This method is necessary to work around one specific problem:
// Before the init() hook runs, if there is one, the BaseObject() constructor
// registers this object with the Environment for finilization and debugging
// purposes.
// If the Environment decides to inspect this object for debugging, it tries to
// call virtual methods on this object that are only (meaningfully) implemented
// by the subclasses of AsyncWrap.
// This could, with bad luck, happen during the AsyncWrap() constructor,
// because we run JS code as part of it and that in turn can lead to a heapdump
// being taken, either through the inspector or our programmatic API for it.
// The object being initialized is not fully constructed at that point, and
// in particular its virtual function table points to the AsyncWrap one
// (as the subclass constructor has not yet begun execution at that point).
// This means that the functions that are used for heap dump memory tracking
// are not yet available, and trying to call them would crash the process.
// We use this particular `IsDoneInitializing()` method to tell the Environment
// that such debugging methods are not yet available.
// This may be somewhat unreliable when it comes to future changes, because
// at this point it *only* protects AsyncWrap subclasses, and *only* for cases
// where heap dumps are being taken while the init() hook is on the call stack.
// For now, it seems like the best solution, though.
bool AsyncWrap::IsDoneInitializing() const {
return init_hook_ran_;
}


AsyncWrap::~AsyncWrap() {
EmitTraceEventDestroy();
EmitDestroy();
Expand Down
5 changes: 4 additions & 1 deletion src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ class AsyncWrap : public BaseObject {
AsyncWrap* wrap_ = nullptr;
};

bool IsDoneInitializing() const override;

private:
friend class PromiseWrap;

Expand All @@ -218,7 +220,8 @@ class AsyncWrap : public BaseObject {
ProviderType provider,
double execution_async_id,
bool silent);
ProviderType provider_type_;
ProviderType provider_type_ = PROVIDER_NONE;
bool init_hook_ran_ = false;
// Because the values may be Reset(), cannot be made const.
double async_id_ = kInvalidAsyncId;
double trigger_async_id_;
Expand Down
3 changes: 3 additions & 0 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ class BaseObject : public MemoryRetainer {
v8::Local<v8::Value> value,
const v8::PropertyCallbackInfo<void>& info);

// This is a bit of a hack. See the override in async_wrap.cc for details.
virtual bool IsDoneInitializing() const;

protected:
// Can be used to avoid the automatic object deletion when the Environment
// exits, for example when this object is owned and deleted by another
Expand Down
6 changes: 4 additions & 2 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -954,8 +954,8 @@ void MemoryTracker::TrackField(const char* edge_name,
// identified and tracked here (based on their deleters),
// but we may convert and track other known types here.
BaseObject* obj = value.GetBaseObject();
if (obj != nullptr) {
this->TrackField("arg", obj);
if (obj != nullptr && obj->IsDoneInitializing()) {
TrackField("arg", obj);
}
CHECK_EQ(CurrentNode(), n);
CHECK_NE(n->size_, 0);
Expand Down Expand Up @@ -1077,6 +1077,8 @@ void BaseObject::DeleteMe(void* data) {
delete self;
}

bool BaseObject::IsDoneInitializing() const { return true; }

Local<Object> BaseObject::WrappedObject() const {
return object();
}
Expand Down
46 changes: 46 additions & 0 deletions test/parallel/test-heapdump-async-hooks-init-promise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Flags: --expose-gc
'use strict';
const common = require('../common');
const assert = require('assert');
const async_hooks = require('async_hooks');
const v8 = require('v8');

// Regression test for https://github.com/nodejs/node/issues/28786
// Make sure that creating a heap snapshot inside an async_hooks hook
// works for Promises.

const createSnapshot = common.mustCall(() => {
v8.getHeapSnapshot().resume();
}, 8); // 2 × init + 2 × resolve + 1 × (after + before) + 2 × destroy = 8 calls

const promiseIds = [];

async_hooks.createHook({
init(id, type) {
if (type === 'PROMISE') {
createSnapshot();
promiseIds.push(id);
}
},

before(id) {
if (promiseIds.includes(id)) createSnapshot();
},

after(id) {
if (promiseIds.includes(id)) createSnapshot();
},

promiseResolve(id) {
assert(promiseIds.includes(id));
createSnapshot();
},

destroy(id) {
if (promiseIds.includes(id)) createSnapshot();
}
}).enable();


Promise.resolve().then(() => {});
setImmediate(global.gc);

0 comments on commit 9949fbd

Please sign in to comment.