-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
n-api: add APIs for per-instance state management #28682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
gabrielschulhof
wants to merge
3
commits into
nodejs:master
from
gabrielschulhof:n-api-instance-data
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,9 +46,9 @@ class BufferFinalizer : private Finalizer { | |
v8::HandleScope handle_scope(finalizer->_env->isolate); | ||
v8::Context::Scope context_scope(finalizer->_env->context()); | ||
|
||
NapiCallIntoModuleThrow(finalizer->_env, [&]() { | ||
finalizer->_env->CallIntoModuleThrow([&](napi_env env) { | ||
finalizer->_finalize_callback( | ||
finalizer->_env, | ||
env, | ||
finalizer->_finalize_data, | ||
finalizer->_finalize_hint); | ||
}); | ||
|
@@ -59,44 +59,22 @@ class BufferFinalizer : private Finalizer { | |
} | ||
}; | ||
|
||
static inline napi_env GetEnv(v8::Local<v8::Context> context) { | ||
static inline napi_env NewEnv(v8::Local<v8::Context> context) { | ||
node_napi_env result; | ||
|
||
auto isolate = context->GetIsolate(); | ||
auto global = context->Global(); | ||
|
||
// In the case of the string for which we grab the private and the value of | ||
// the private on the global object we can call .ToLocalChecked() directly | ||
// because we need to stop hard if either of them is empty. | ||
// | ||
// Re https://github.com/nodejs/node/pull/14217#discussion_r128775149 | ||
auto value = global->GetPrivate(context, NAPI_PRIVATE_KEY(context, env)) | ||
.ToLocalChecked(); | ||
|
||
if (value->IsExternal()) { | ||
result = static_cast<node_napi_env>(value.As<v8::External>()->Value()); | ||
} else { | ||
result = new node_napi_env__(context); | ||
auto external = v8::External::New(isolate, result); | ||
|
||
// We must also stop hard if the result of assigning the env to the global | ||
// is either nothing or false. | ||
CHECK(global->SetPrivate(context, NAPI_PRIVATE_KEY(context, env), external) | ||
.FromJust()); | ||
|
||
// TODO(addaleax): There was previously code that tried to delete the | ||
// napi_env when its v8::Context was garbage collected; | ||
// However, as long as N-API addons using this napi_env are in place, | ||
// the Context needs to be accessible and alive. | ||
// Ideally, we'd want an on-addon-unload hook that takes care of this | ||
// once all N-API addons using this napi_env are unloaded. | ||
// For now, a per-Environment cleanup hook is the best we can do. | ||
result->node_env()->AddCleanupHook( | ||
[](void* arg) { | ||
static_cast<napi_env>(arg)->Unref(); | ||
}, | ||
static_cast<void*>(result)); | ||
} | ||
result = new node_napi_env__(context); | ||
// TODO(addaleax): There was previously code that tried to delete the | ||
// napi_env when its v8::Context was garbage collected; | ||
// However, as long as N-API addons using this napi_env are in place, | ||
// the Context needs to be accessible and alive. | ||
// Ideally, we'd want an on-addon-unload hook that takes care of this | ||
// once all N-API addons using this napi_env are unloaded. | ||
// For now, a per-Environment cleanup hook is the best we can do. | ||
result->node_env()->AddCleanupHook( | ||
[](void* arg) { | ||
static_cast<napi_env>(arg)->Unref(); | ||
}, | ||
static_cast<void*>(result)); | ||
|
||
return result; | ||
} | ||
|
@@ -325,7 +303,7 @@ class ThreadSafeFunction : public node::AsyncResource { | |
v8::Local<v8::Function>::New(env->isolate, ref); | ||
js_callback = v8impl::JsValueFromV8LocalValue(js_cb); | ||
} | ||
NapiCallIntoModuleThrow(env, [&]() { | ||
env->CallIntoModuleThrow([&](napi_env env) { | ||
call_js_cb(env, js_callback, context, data); | ||
}); | ||
} | ||
|
@@ -346,7 +324,7 @@ class ThreadSafeFunction : public node::AsyncResource { | |
v8::HandleScope scope(env->isolate); | ||
if (finalize_cb) { | ||
CallbackScope cb_scope(this); | ||
NapiCallIntoModuleThrow(env, [&]() { | ||
env->CallIntoModuleThrow([&](napi_env env) { | ||
finalize_cb(env, finalize_data, context); | ||
}); | ||
} | ||
|
@@ -481,10 +459,10 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports, | |
|
||
// Create a new napi_env for this module or reference one if a pre-existing | ||
// one is found. | ||
napi_env env = v8impl::GetEnv(context); | ||
napi_env env = v8impl::NewEnv(context); | ||
|
||
napi_value _exports; | ||
NapiCallIntoModuleThrow(env, [&]() { | ||
env->CallIntoModuleThrow([&](napi_env env) { | ||
_exports = init(env, v8impl::JsValueFromV8LocalValue(exports)); | ||
}); | ||
|
||
|
@@ -889,15 +867,9 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork { | |
|
||
CallbackScope callback_scope(this); | ||
|
||
// We have to back up the env here because the `NAPI_CALL_INTO_MODULE` macro | ||
// makes use of it after the call into the module completes, but the module | ||
// may have deallocated **this**, and along with it the place where _env is | ||
// stored. | ||
napi_env env = _env; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By changing the expected signature of the lambda(s) being passed into |
||
|
||
NapiCallIntoModule(env, [&]() { | ||
_complete(_env, ConvertUVErrorCode(status), _data); | ||
}, [env](v8::Local<v8::Value> local_err) { | ||
_env->CallIntoModule([&](napi_env env) { | ||
_complete(env, ConvertUVErrorCode(status), _data); | ||
}, [](napi_env env, v8::Local<v8::Value> local_err) { | ||
// If there was an unhandled exception in the complete callback, | ||
// report it as a fatal exception. (There is no JavaScript on the | ||
// callstack that can possibly handle it.) | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.