Skip to content

Commit

Permalink
src: render N-API weak callbacks as cleanup hooks
Browse files Browse the repository at this point in the history
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
Gabriel Schulhof committed Oct 13, 2019
1 parent fce1a51 commit 53ca0b9
Show file tree
Hide file tree
Showing 8 changed files with 288 additions and 33 deletions.
82 changes: 82 additions & 0 deletions benchmark/napi/ref/addon.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#include <stdlib.h>
#define NAPI_EXPERIMENTAL
#include <node_api.h>

#define NAPI_CALL(env, call) \
do { \
napi_status status = (call); \
if (status != napi_ok) { \
napi_throw_error((env), NULL, #call " failed"); \
return NULL; \
} \
} while (0)

static napi_value
GetCount(napi_env env, napi_callback_info info) {
napi_value result;
size_t* count;

NAPI_CALL(env, napi_get_instance_data(env, (void**)&count));
NAPI_CALL(env, napi_create_uint32(env, *count, &result));

return result;
}

static napi_value
SetCount(napi_env env, napi_callback_info info) {
size_t* count;

NAPI_CALL(env, napi_get_instance_data(env, (void**)&count));

// Set the count to zero irrespective of what is passed into the setter.
*count = 0;

return NULL;
}

static void
IncrementCounter(napi_env env, void* data, void* hint) {
size_t* count = data;
(*count) = (*count) + 1;
}

static napi_value
NewWeak(napi_env env, napi_callback_info info) {
napi_value result;
void* instance_data;

NAPI_CALL(env, napi_create_object(env, &result));
NAPI_CALL(env, napi_get_instance_data(env, &instance_data));
NAPI_CALL(env, napi_add_finalizer(env,
result,
instance_data,
IncrementCounter,
NULL,
NULL));

return result;
}

static void
FreeCount(napi_env env, void* data, void* hint) {
free(data);
}

/* napi_value */
NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) {
napi_property_descriptor props[] = {
{ "count", NULL, NULL, GetCount, SetCount, NULL, napi_enumerable, NULL },
{ "newWeak", NULL, NewWeak, NULL, NULL, NULL, napi_enumerable, NULL }
};

size_t* count = malloc(sizeof(*count));
*count = 0;

NAPI_CALL(env, napi_define_properties(env,
exports,
sizeof(props) / sizeof(*props),
props));
NAPI_CALL(env, napi_set_instance_data(env, count, FreeCount, NULL));

return exports;
}
10 changes: 10 additions & 0 deletions benchmark/napi/ref/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
'targets': [
{
'target_name': 'addon',
'sources': [
'addon.c'
]
}
]
}
17 changes: 17 additions & 0 deletions benchmark/napi/ref/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';
const common = require('../../common');
const addon = require(`./build/${common.buildType}/addon`);
const bench = common.createBenchmark(main, { n: [1e7] });

function callNewWeak() {
addon.newWeak();
}

function main({ n }) {
addon.count = 0;
bench.start();
while (addon.count < n) {
callNewWeak();
}
bench.end(n);
}
46 changes: 26 additions & 20 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ inline static napi_status ConcludeDeferred(napi_env env,
}

// Wrapper around v8impl::Persistent that implements reference counting.
class Reference : private Finalizer {
class Reference : private Finalizer, RefTracker {
private:
Reference(napi_env env,
v8::Local<v8::Value> value,
Expand All @@ -203,6 +203,9 @@ class Reference : private Finalizer {
_persistent.SetWeak(
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
}
Link(finalize_callback == nullptr
? &env->reflist
: &env->finalizing_reflist);
}

public:
Expand Down Expand Up @@ -242,6 +245,7 @@ class Reference : private Finalizer {
// the finalizer and _delete_self is set. In this case we
// know we need to do the deletion so just do it.
static void Delete(Reference* reference) {
reference->Unlink();
if ((reference->RefCount() != 0) ||
(reference->_delete_self) ||
(reference->_finalize_ran)) {
Expand Down Expand Up @@ -286,6 +290,26 @@ class Reference : private Finalizer {
}

private:
void Finalize(bool is_env_teardown = false) override {
if (_finalize_callback != nullptr) {
_env->CallIntoModuleThrow([&](napi_env env) {
_finalize_callback(
env,
_finalize_data,
_finalize_hint);
});
}

// this is safe because if a request to delete the reference
// is made in the finalize_callback it will defer deletion
// to this block and set _delete_self to true
if (_delete_self || is_env_teardown) {
Delete(this);
} else {
_finalize_ran = true;
}
}

// The N-API finalizer callback may make calls into the engine. V8's heap is
// not in a consistent state during the weak callback, and therefore it does
// not support calls back into it. However, it provides a mechanism for adding
Expand All @@ -303,25 +327,7 @@ class Reference : private Finalizer {
}

static void SecondPassCallback(const v8::WeakCallbackInfo<Reference>& data) {
Reference* reference = data.GetParameter();

if (reference->_finalize_callback != nullptr) {
reference->_env->CallIntoModuleThrow([&](napi_env env) {
reference->_finalize_callback(
env,
reference->_finalize_data,
reference->_finalize_hint);
});
}

// this is safe because if a request to delete the reference
// is made in the finalize_callback it will defer deletion
// to this block and set _delete_self to true
if (reference->_delete_self) {
Delete(reference);
} else {
reference->_finalize_ran = true;
}
data.GetParameter()->Finalize();
}

v8impl::Persistent<v8::Value> _persistent;
Expand Down
13 changes: 13 additions & 0 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ struct napi_env__ {
CHECK_EQ(isolate, context->GetIsolate());
}
virtual ~napi_env__() {
// First we must finalize those references that have `napi_finalizer`
// callbacks. The reason is that addons might store other references which
// they delete during their `napi_finalizer` callbacks. If we deleted such
// references here first, they would be doubly deleted when the
// `napi_finalizer` deleted them subsequently.
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
v8impl::RefTracker::FinalizeAll(&reflist);
if (instance_data.finalize_cb != nullptr) {
CallIntoModuleThrow([&](napi_env env) {
instance_data.finalize_cb(env, instance_data.data, instance_data.hint);
Expand Down Expand Up @@ -55,6 +62,12 @@ struct napi_env__ {
}

v8impl::Persistent<v8::Value> last_exception;

// We store references in two different lists, depending on whether they have
// `napi_finalizer` callbacks, because we must first finalize the ones that
// have such a callback. See `~napi_env__()` above for details.
v8impl::RefTracker::RefList reflist;
v8impl::RefTracker::RefList finalizing_reflist;
napi_extended_error_info last_error;
int open_handle_scopes = 0;
int open_callback_scopes = 0;
Expand Down
39 changes: 39 additions & 0 deletions src/js_native_api_v8_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,45 @@

namespace v8impl {

class RefTracker {
public:
RefTracker() {}
virtual ~RefTracker() {}
virtual void Finalize(bool isEnvTeardown) {}

typedef RefTracker RefList;

inline void Link(RefList* list) {
prev_ = list;
next_ = list->next_;
if (next_ != nullptr) {
next_->prev_ = this;
}
list->next_ = this;
}

inline void Unlink() {
if (prev_ != nullptr) {
prev_->next_ = next_;
}
if (next_ != nullptr) {
next_->prev_ = prev_;
}
prev_ = nullptr;
next_ = nullptr;
}

static void FinalizeAll(RefList* list) {
while (list->next_ != nullptr) {
list->next_->Finalize(true);
}
}

private:
RefList* next_ = nullptr;
RefList* prev_ = nullptr;
};

template <typename T>
using Persistent = v8::Global<T>;

Expand Down
57 changes: 57 additions & 0 deletions test/js-native-api/test_general/testEnvCleanup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict';

if (process.argv[2] === 'child') {
const common = require('../../common');
const test_general = require(`./build/${common.buildType}/test_general`);

// The second argument to `envCleanupWrap()` is an index into the global
// static string array named `env_cleanup_finalizer_messages` on the native
// side. A reverse mapping is reproduced here for clarity.
const finalizerMessages = {
'simple wrap': 0,
'wrap, removeWrap': 1,
'first wrap': 2,
'second wrap': 3
};

// We attach the three objects we will test to `module.exports` to ensure they
// will not be garbage-collected before the process exits.

// Make sure the finalizer for a simple wrap will be called at env cleanup.
module.exports['simple wrap'] =
test_general.envCleanupWrap({}, finalizerMessages['simple wrap']);

// Make sure that a removed wrap does not result in a call to its finalizer at
// env cleanup.
module.exports['wrap, removeWrap'] =
test_general.envCleanupWrap({}, finalizerMessages['wrap, removeWrap']);
test_general.removeWrap(module.exports['wrap, removeWrap']);

// Make sure that only the latest attached version of a re-wrapped item's
// finalizer gets called at env cleanup.
module.exports['first wrap'] =
test_general.envCleanupWrap({}, finalizerMessages['first wrap']),
test_general.removeWrap(module.exports['first wrap']);
test_general.envCleanupWrap(module.exports['first wrap'],
finalizerMessages['second wrap']);
} else {
const assert = require('assert');
const { spawnSync } = require('child_process');

const child = spawnSync(process.execPath, [__filename, 'child'], {
stdio: [ process.stdin, 'pipe', process.stderr ]
});

// Grab the child's output and construct an object whose keys are the rows of
// the output and whose values are `true`, so we can compare the output while
// ignoring the order in which the lines of it were produced.
assert.deepStrictEqual(
child.stdout.toString().split(/\r\n|\r|\n/g).reduce((obj, item) =>
Object.assign(obj, item ? { [item]: true } : {}), {}), {
'finalize at env cleanup for simple wrap': true,
'finalize at env cleanup for second wrap': true
});

// Ensure that the child exited successfully.
assert.strictEqual(child.status, 0);
}
Loading

3 comments on commit 53ca0b9

@Trott
Copy link
Member

@Trott Trott commented on 53ca0b9 Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @gabrielschulhof! Looks like this broke make bench-addons-build && tools/test.py test/benchmark/test-benchmark-napi.js but that wasn't detected in the regular CI because the benchmark tests are only run during nightly CI. Not a big deal, and I'm guessing the fix is simple but haven't looked....

@gabrielschulhof
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott checking it out ...

@richardlau
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#29995 should address the breakage.

Please sign in to comment.