Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit aac2476

Browse filesBrowse files
Gabriel Schulhoftargos
Gabriel Schulhof
authored and
targos
committed
src: render N-API weak callbacks as cleanup hooks
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>
1 parent 8a333a4 commit aac2476
Copy full SHA for aac2476

File tree

Expand file treeCollapse file tree

8 files changed

+288
-33
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

8 files changed

+288
-33
lines changed
Open diff view settings
Collapse file

‎benchmark/napi/ref/addon.c‎

Copy file name to clipboard
+82Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
#include <stdlib.h>
2+
#define NAPI_EXPERIMENTAL
3+
#include <node_api.h>
4+
5+
#define NAPI_CALL(env, call) \
6+
do { \
7+
napi_status status = (call); \
8+
if (status != napi_ok) { \
9+
napi_throw_error((env), NULL, #call " failed"); \
10+
return NULL; \
11+
} \
12+
} while (0)
13+
14+
static napi_value
15+
GetCount(napi_env env, napi_callback_info info) {
16+
napi_value result;
17+
size_t* count;
18+
19+
NAPI_CALL(env, napi_get_instance_data(env, (void**)&count));
20+
NAPI_CALL(env, napi_create_uint32(env, *count, &result));
21+
22+
return result;
23+
}
24+
25+
static napi_value
26+
SetCount(napi_env env, napi_callback_info info) {
27+
size_t* count;
28+
29+
NAPI_CALL(env, napi_get_instance_data(env, (void**)&count));
30+
31+
// Set the count to zero irrespective of what is passed into the setter.
32+
*count = 0;
33+
34+
return NULL;
35+
}
36+
37+
static void
38+
IncrementCounter(napi_env env, void* data, void* hint) {
39+
size_t* count = data;
40+
(*count) = (*count) + 1;
41+
}
42+
43+
static napi_value
44+
NewWeak(napi_env env, napi_callback_info info) {
45+
napi_value result;
46+
void* instance_data;
47+
48+
NAPI_CALL(env, napi_create_object(env, &result));
49+
NAPI_CALL(env, napi_get_instance_data(env, &instance_data));
50+
NAPI_CALL(env, napi_add_finalizer(env,
51+
result,
52+
instance_data,
53+
IncrementCounter,
54+
NULL,
55+
NULL));
56+
57+
return result;
58+
}
59+
60+
static void
61+
FreeCount(napi_env env, void* data, void* hint) {
62+
free(data);
63+
}
64+
65+
/* napi_value */
66+
NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) {
67+
napi_property_descriptor props[] = {
68+
{ "count", NULL, NULL, GetCount, SetCount, NULL, napi_enumerable, NULL },
69+
{ "newWeak", NULL, NewWeak, NULL, NULL, NULL, napi_enumerable, NULL }
70+
};
71+
72+
size_t* count = malloc(sizeof(*count));
73+
*count = 0;
74+
75+
NAPI_CALL(env, napi_define_properties(env,
76+
exports,
77+
sizeof(props) / sizeof(*props),
78+
props));
79+
NAPI_CALL(env, napi_set_instance_data(env, count, FreeCount, NULL));
80+
81+
return exports;
82+
}
Collapse file

‎benchmark/napi/ref/binding.gyp‎

Copy file name to clipboard
+10Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
'targets': [
3+
{
4+
'target_name': 'addon',
5+
'sources': [
6+
'addon.c'
7+
]
8+
}
9+
]
10+
}
Collapse file

‎benchmark/napi/ref/index.js‎

Copy file name to clipboard
+17Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict';
2+
const common = require('../../common');
3+
const addon = require(`./build/${common.buildType}/addon`);
4+
const bench = common.createBenchmark(main, { n: [1e7] });
5+
6+
function callNewWeak() {
7+
addon.newWeak();
8+
}
9+
10+
function main({ n }) {
11+
addon.count = 0;
12+
bench.start();
13+
while (addon.count < n) {
14+
callNewWeak();
15+
}
16+
bench.end(n);
17+
}
Collapse file

‎src/js_native_api_v8.cc‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.cc
+26-20Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ inline static napi_status ConcludeDeferred(napi_env env,
186186
}
187187

188188
// Wrapper around v8impl::Persistent that implements reference counting.
189-
class Reference : private Finalizer {
189+
class Reference : private Finalizer, RefTracker {
190190
private:
191191
Reference(napi_env env,
192192
v8::Local<v8::Value> value,
@@ -203,6 +203,9 @@ class Reference : private Finalizer {
203203
_persistent.SetWeak(
204204
this, FinalizeCallback, v8::WeakCallbackType::kParameter);
205205
}
206+
Link(finalize_callback == nullptr
207+
? &env->reflist
208+
: &env->finalizing_reflist);
206209
}
207210

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

288292
private:
293+
void Finalize(bool is_env_teardown = false) override {
294+
if (_finalize_callback != nullptr) {
295+
_env->CallIntoModuleThrow([&](napi_env env) {
296+
_finalize_callback(
297+
env,
298+
_finalize_data,
299+
_finalize_hint);
300+
});
301+
}
302+
303+
// this is safe because if a request to delete the reference
304+
// is made in the finalize_callback it will defer deletion
305+
// to this block and set _delete_self to true
306+
if (_delete_self || is_env_teardown) {
307+
Delete(this);
308+
} else {
309+
_finalize_ran = true;
310+
}
311+
}
312+
289313
// The N-API finalizer callback may make calls into the engine. V8's heap is
290314
// not in a consistent state during the weak callback, and therefore it does
291315
// not support calls back into it. However, it provides a mechanism for adding
@@ -303,25 +327,7 @@ class Reference : private Finalizer {
303327
}
304328

305329
static void SecondPassCallback(const v8::WeakCallbackInfo<Reference>& data) {
306-
Reference* reference = data.GetParameter();
307-
308-
if (reference->_finalize_callback != nullptr) {
309-
reference->_env->CallIntoModuleThrow([&](napi_env env) {
310-
reference->_finalize_callback(
311-
env,
312-
reference->_finalize_data,
313-
reference->_finalize_hint);
314-
});
315-
}
316-
317-
// this is safe because if a request to delete the reference
318-
// is made in the finalize_callback it will defer deletion
319-
// to this block and set _delete_self to true
320-
if (reference->_delete_self) {
321-
Delete(reference);
322-
} else {
323-
reference->_finalize_ran = true;
324-
}
330+
data.GetParameter()->Finalize();
325331
}
326332

327333
v8impl::Persistent<v8::Value> _persistent;
Collapse file

‎src/js_native_api_v8.h‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.h
+13Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,13 @@ struct napi_env__ {
1515
CHECK_EQ(isolate, context->GetIsolate());
1616
}
1717
virtual ~napi_env__() {
18+
// First we must finalize those references that have `napi_finalizer`
19+
// callbacks. The reason is that addons might store other references which
20+
// they delete during their `napi_finalizer` callbacks. If we deleted such
21+
// references here first, they would be doubly deleted when the
22+
// `napi_finalizer` deleted them subsequently.
23+
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
24+
v8impl::RefTracker::FinalizeAll(&reflist);
1825
if (instance_data.finalize_cb != nullptr) {
1926
CallIntoModuleThrow([&](napi_env env) {
2027
instance_data.finalize_cb(env, instance_data.data, instance_data.hint);
@@ -55,6 +62,12 @@ struct napi_env__ {
5562
}
5663

5764
v8impl::Persistent<v8::Value> last_exception;
65+
66+
// We store references in two different lists, depending on whether they have
67+
// `napi_finalizer` callbacks, because we must first finalize the ones that
68+
// have such a callback. See `~napi_env__()` above for details.
69+
v8impl::RefTracker::RefList reflist;
70+
v8impl::RefTracker::RefList finalizing_reflist;
5871
napi_extended_error_info last_error;
5972
int open_handle_scopes = 0;
6073
int open_callback_scopes = 0;
Collapse file

‎src/js_native_api_v8_internals.h‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8_internals.h
+39Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,45 @@
2828

2929
namespace v8impl {
3030

31+
class RefTracker {
32+
public:
33+
RefTracker() {}
34+
virtual ~RefTracker() {}
35+
virtual void Finalize(bool isEnvTeardown) {}
36+
37+
typedef RefTracker RefList;
38+
39+
inline void Link(RefList* list) {
40+
prev_ = list;
41+
next_ = list->next_;
42+
if (next_ != nullptr) {
43+
next_->prev_ = this;
44+
}
45+
list->next_ = this;
46+
}
47+
48+
inline void Unlink() {
49+
if (prev_ != nullptr) {
50+
prev_->next_ = next_;
51+
}
52+
if (next_ != nullptr) {
53+
next_->prev_ = prev_;
54+
}
55+
prev_ = nullptr;
56+
next_ = nullptr;
57+
}
58+
59+
static void FinalizeAll(RefList* list) {
60+
while (list->next_ != nullptr) {
61+
list->next_->Finalize(true);
62+
}
63+
}
64+
65+
private:
66+
RefList* next_ = nullptr;
67+
RefList* prev_ = nullptr;
68+
};
69+
3170
template <typename T>
3271
using Persistent = v8::Global<T>;
3372

Collapse file

‎test/js-native-api/test_general/testEnvCleanup.js‎

Copy file name to clipboard
+57Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
'use strict';
2+
3+
if (process.argv[2] === 'child') {
4+
const common = require('../../common');
5+
const test_general = require(`./build/${common.buildType}/test_general`);
6+
7+
// The second argument to `envCleanupWrap()` is an index into the global
8+
// static string array named `env_cleanup_finalizer_messages` on the native
9+
// side. A reverse mapping is reproduced here for clarity.
10+
const finalizerMessages = {
11+
'simple wrap': 0,
12+
'wrap, removeWrap': 1,
13+
'first wrap': 2,
14+
'second wrap': 3
15+
};
16+
17+
// We attach the three objects we will test to `module.exports` to ensure they
18+
// will not be garbage-collected before the process exits.
19+
20+
// Make sure the finalizer for a simple wrap will be called at env cleanup.
21+
module.exports['simple wrap'] =
22+
test_general.envCleanupWrap({}, finalizerMessages['simple wrap']);
23+
24+
// Make sure that a removed wrap does not result in a call to its finalizer at
25+
// env cleanup.
26+
module.exports['wrap, removeWrap'] =
27+
test_general.envCleanupWrap({}, finalizerMessages['wrap, removeWrap']);
28+
test_general.removeWrap(module.exports['wrap, removeWrap']);
29+
30+
// Make sure that only the latest attached version of a re-wrapped item's
31+
// finalizer gets called at env cleanup.
32+
module.exports['first wrap'] =
33+
test_general.envCleanupWrap({}, finalizerMessages['first wrap']),
34+
test_general.removeWrap(module.exports['first wrap']);
35+
test_general.envCleanupWrap(module.exports['first wrap'],
36+
finalizerMessages['second wrap']);
37+
} else {
38+
const assert = require('assert');
39+
const { spawnSync } = require('child_process');
40+
41+
const child = spawnSync(process.execPath, [__filename, 'child'], {
42+
stdio: [ process.stdin, 'pipe', process.stderr ]
43+
});
44+
45+
// Grab the child's output and construct an object whose keys are the rows of
46+
// the output and whose values are `true`, so we can compare the output while
47+
// ignoring the order in which the lines of it were produced.
48+
assert.deepStrictEqual(
49+
child.stdout.toString().split(/\r\n|\r|\n/g).reduce((obj, item) =>
50+
Object.assign(obj, item ? { [item]: true } : {}), {}), {
51+
'finalize at env cleanup for simple wrap': true,
52+
'finalize at env cleanup for second wrap': true
53+
});
54+
55+
// Ensure that the child exited successfully.
56+
assert.strictEqual(child.status, 0);
57+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.