-
Notifications
You must be signed in to change notification settings - Fork 459
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
Context-aware add-ons? #567
Comments
Fwiw, all N-API addons are considered context-aware.
I think (The documentation for it is kind of misleading imo – Node.js currently does not support loading addons from multiple vm Contexts, so right now it’s not wrong to say that the data is associated with the current agent, but conceptually, the association is with the current module instantiation, which would be per-Context and not per-Agent.) |
It does seem like Is there any way to polyfill this for Node 10? Would be a shame to have to wait for Node 10 to be out of LTS for it to be generally usable for add-ons. |
@gabrielschulhof I think you implemented napi_set_instance_date/napi_get_instance_data. I also remember that we refactored the internal structure at some point. Do you remember if that was before/after 10.x as that might affect how easily those two functions can be supported in the 10.x line. |
@rolftimmermans there are several solutions to attaching data to an addon instance, in increasing order of correctness and convenience, but decreasing availability in terms of older Node.js versions:
I agree that we could use a nice abstraction on top of these mechanisms to render addons as instance factories. |
whew ... I did several edits to my comment after I wrote it. Please do not read the emailed version 😀 |
Thanks for clearing this up! Very helpful. I see that |
So it turns out that |
I have gone down the I guess It's just not possible right now to detect when the module is loaded/released from the main thread, correct? I have attempted to use static destructors for this cleanup but it does not seem to work correctly on windows. |
@addaleax how safe is it to assume that the relationship between the environment and the thread is 1:1? @rolftimmermans could you use thread-safe refcounting to decide when to tear down the per-process state? I'm fairly certain that the main thread is the last one to exit. |
You can have multiple napi environments in a single thread, but you can't have multiple threads interacting with a napi environment. |
Alright, the following is a summary of I have now, based on your comments. It might be useful for others. This seems to work well for Node 10+, except that it appears nodejs/node#28428 won't be included in a stable version until Node 14, which means that so far any use of threads with native addons is giant memory leak (I haven't found a reliable workaround for this unfortunately – suggestions welcome!). class GlobalState {
// ... Globally shared, thread-safe state.
// e.g. uint64_t FooBar;
public:
static std::shared_ptr<GlobalState> Instance() {
static std::mutex lock;
static std::weak_ptr<GlobalState> shared;
std::lock_guard<std::mutex> guard(lock);
// Get an existing instance from the weak reference, if possible.
if (auto instance = shared.lock()) {
return instance;
}
// Create a new instance and keep a weak reference.
// Global state will be cleaned up when last thread exits.
auto instance = std::make_shared<ContextReaper>();
shared = instance;
return instance;
}
};
class Module {
// Pointer to shared global state
std::shared_ptr<GlobalState> global_state = GlobalState::Instance();
// ... Other per-module state
// e.g.: Napi::FunctionReference foo_constructor;
public:
GlobalState& GlobalState() {
return *global_state;
}
Module(Napi::Object exports) {
// Initialization per module, assigns constructors, etc.
auto proto = {
// ... InstanceMethod("foo", &Foo::foo),
};
// Pass module pointer as last argument.
DefineClass(exports.Env(), "Foo", proto, this);
}
};
class Foo : public Napi::ObjectWrap<Foo> {
Module& module;
public:
// Construct native object. `info.Data()` contains pointer to module.
Foo(const Napi::CallbackInfo& info)
: Napi::ObjectWrap<Foo>(info),
module(*reinterpret_cast<Module*>(info.Data())) {
// Access global state with e.g. `module.GlobalState().FooBar`
}
}
NAPI_MODULE_INIT(/* env, exports */) {
auto module = new Module(Napi::Object(env, exports));
auto terminate = [](void* data) { delete reinterpret_cast<Module*>(data); };
auto status = napi_add_env_cleanup_hook(env, terminate, module);
assert(status == napi_ok);
return exports;
} In the future the module-wide state could be accessed via |
@gabrielschulhof This is currently always true for the Node.js release binary/when used with Workers, but native addons or embedders don’t need to follow any rules regarding relationships between threads and
@rolftimmermans If you are looking for “global” state that should be torn down when your addon is unloaded, then
Why do you think it won’t be included into Node 13 and backported to earlier release lines? While I certainly think we should be cautious about that, I think this is what we should be doing.
Yeah, that’s just a really, really unfortunate fact in terms of N-API’s design. You should be able to use |
@addaleax It would be nice if it could be back-ported, definitely!
Having it in |
@rolftimmermans I have a POC that does not use node-addon-api, but which can no doubt be adapted: The boilerplate: template <class AddonClass>
class AddonBase {
public:
virtual ~AddonBase() {
while(methods != nullptr) {
MethodData* next = methods->next;
delete methods;
methods = next;
}
}
virtual napi_value Init(napi_env env, napi_value exports) {
return exports;
}
static napi_value New(napi_env env, napi_value exports) {
AddonClass* instance = new AddonClass();
napi_set_instance_data(env,
instance,
Delete,
nullptr);
return instance->Init(env, exports);
}
static void Delete(napi_env env, void* data, void* hint) {
delete static_cast<AddonClass*>(data);
}
typedef napi_value(AddonClass::*MethodPointer)(napi_env env,
napi_callback_info info);
struct MethodData {
public:
MethodData(MethodPointer cb, AddonClass* instance, MethodData* next):
cb(cb), instance(instance), next(next) {}
MethodPointer cb;
AddonClass* instance;
MethodData* next;
};
void AddMethod(napi_env env,
napi_value exports,
const char* name,
MethodPointer method) {
napi_value result;
methods = new MethodData(method, static_cast<AddonClass*>(this), methods);
napi_create_function(env,
name,
NAPI_AUTO_LENGTH,
Dispatcher,
methods,
&result);
napi_set_named_property(env, exports, name, result);
}
private:
static napi_value Dispatcher(napi_env env, napi_callback_info info) {
size_t argc = 0;
void* raw_data;
napi_get_cb_info(env, info, &argc, nullptr, nullptr, &raw_data);
MethodData* mdata = static_cast<MethodData*>(raw_data);
return ((mdata->instance)->*(mdata->cb))(env, info);
}
MethodData* methods = nullptr;
}; and its usage: class MyAddon: public AddonBase<MyAddon> {
public:
napi_value Init(napi_env env, napi_value exports) override {
// List methods to expose here.
AddMethod(env, exports, "somethingUseful", &MyAddon::SomethingUseful);
return exports;
}
// The binding is an instance method and so has access to the instance.
napi_value SomethingUseful(napi_env env, napi_callback_info info) {
napi_value result;
napi_create_uint32(env, ++call_count, &result);
return result;
}
private:
size_t call_count = 0;
};
NAPI_MODULE_INIT() {
return MyAddon::New(env, exports);
} I'll check to see how best to integrate this with node-addon-api and its multiple ways of exposing bindings. |
I ran into some trouble when using static globals that are initialized in the before-main time on windows machines, at least while using cmakejs instead of gyp: cmake-js/cmake-js#205 , however static local variables are working fine, so I recommend to use those. However the whole context aware addon problematic came to my attention just of today, so my bug might be related to my probale misunderstanding of this. Correct me if I am wrong: Since the whole shared library is loaded only once, at the electron app seems to be just one node-process (but multiple envs/worker threads /agents), I could use per process globals like static local variables for whole process granularity, as long as I do in a thread safe matter, right? Only draw back, would be the missing clean up. But at least I could have a global USB Device context, and queue the commands issued by my JS code. |
@Superlokkus Yes, everything you said makes sense 👍 |
@Superlokkus additionally, if you need per-addon-instance data, you can use Lines 192 to 204 in 9c9accf
Whoops ... no docs yet 😳 |
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: nodejs#567 (comment)
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: nodejs#567 (comment)
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: #567 (comment) PR-URL: #708 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made. |
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: nodejs/node-addon-api#567 (comment) PR-URL: nodejs/node-addon-api#708 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: nodejs/node-addon-api#567 (comment) PR-URL: nodejs/node-addon-api#708 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: nodejs/node-addon-api#567 (comment) PR-URL: nodejs/node-addon-api#708 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Re: nodejs/node-addon-api#567 (comment) PR-URL: nodejs/node-addon-api#708 Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Dear All, Any help would be highly appreciated.. |
Hi @vipin387 , This thread is a little different than what features you need. You need to use thread-safe functions. If you need further assistance, please open a new issue. |
I am reading this https://nodejs.org/api/addons.html#addons_context_aware_addons and am left wondering if
node-addon-api
can help making context aware native modules.There does not seem to be a way to initialise a context-aware module with
node-addon-api
currently.Additionally it seems to me that it would be nice to provide a default wrapper for per-context global state (what would normally be easily accomplished with static variables).
Any thoughts?
The text was updated successfully, but these errors were encountered: