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 b69f2dd

Browse filesBrowse files
addaleaxtargos
authored andcommitted
n-api: keep napi_env alive while it has finalizers
Manage the napi_env refcount from Finalizer instances, as the finalizer may refer to the napi_env until it is deleted. Fixes: #31134 Fixes: node-ffi-napi/node-ffi-napi#48 PR-URL: #31140 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 3fe37e6 commit b69f2dd
Copy full SHA for b69f2dd

File tree

Expand file treeCollapse file tree

3 files changed

+39
-6
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+39
-6
lines changed
Open diff view settings
Collapse file

‎src/js_native_api_v8.h‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.h
+23-5Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,26 +182,43 @@ inline v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
182182

183183
// Adapter for napi_finalize callbacks.
184184
class Finalizer {
185+
public:
186+
// Some Finalizers are run during shutdown when the napi_env is destroyed,
187+
// and some need to keep an explicit reference to the napi_env because they
188+
// are run independently.
189+
enum EnvReferenceMode {
190+
kNoEnvReference,
191+
kKeepEnvReference
192+
};
193+
185194
protected:
186195
Finalizer(napi_env env,
187196
napi_finalize finalize_callback,
188197
void* finalize_data,
189-
void* finalize_hint)
198+
void* finalize_hint,
199+
EnvReferenceMode refmode = kNoEnvReference)
190200
: _env(env),
191201
_finalize_callback(finalize_callback),
192202
_finalize_data(finalize_data),
193-
_finalize_hint(finalize_hint) {
203+
_finalize_hint(finalize_hint),
204+
_has_env_reference(refmode == kKeepEnvReference) {
205+
if (_has_env_reference)
206+
_env->Ref();
194207
}
195208

196-
~Finalizer() = default;
209+
~Finalizer() {
210+
if (_has_env_reference)
211+
_env->Unref();
212+
}
197213

198214
public:
199215
static Finalizer* New(napi_env env,
200216
napi_finalize finalize_callback = nullptr,
201217
void* finalize_data = nullptr,
202-
void* finalize_hint = nullptr) {
218+
void* finalize_hint = nullptr,
219+
EnvReferenceMode refmode = kNoEnvReference) {
203220
return new Finalizer(
204-
env, finalize_callback, finalize_data, finalize_hint);
221+
env, finalize_callback, finalize_data, finalize_hint, refmode);
205222
}
206223

207224
static void Delete(Finalizer* finalizer) {
@@ -214,6 +231,7 @@ class Finalizer {
214231
void* _finalize_data;
215232
void* _finalize_hint;
216233
bool _finalize_ran = false;
234+
bool _has_env_reference = false;
217235
};
218236

219237
class TryCatch : public v8::TryCatch {
Collapse file

‎src/node_api.cc‎

Copy file name to clipboardExpand all lines: src/node_api.cc
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -724,7 +724,8 @@ napi_status napi_create_external_buffer(napi_env env,
724724

725725
// The finalizer object will delete itself after invoking the callback.
726726
v8impl::Finalizer* finalizer = v8impl::Finalizer::New(
727-
env, finalize_cb, nullptr, finalize_hint);
727+
env, finalize_cb, nullptr, finalize_hint,
728+
v8impl::Finalizer::kKeepEnvReference);
728729

729730
auto maybe = node::Buffer::New(isolate,
730731
static_cast<char*>(data),
Collapse file
+14Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
'use strict';
2+
3+
const common = require('../../common');
4+
const binding = require(`./build/${common.buildType}/test_buffer`);
5+
const assert = require('assert');
6+
7+
// Regression test for https://github.com/nodejs/node/issues/31134
8+
// Buffers with finalizers are allowed to remain alive until
9+
// Environment cleanup without crashing the process.
10+
// The test stores the buffer on `process` to make sure it lives as long as
11+
// the Context does.
12+
13+
process.externalBuffer = binding.newExternalBuffer();
14+
assert.strictEqual(process.externalBuffer.toString(), binding.theText);

0 commit comments

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