-
Notifications
You must be signed in to change notification settings - Fork 7
Enable multiple Node-API hosting JS engines / runtimes to share the global Node-API functions #329
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
base: main
Are you sure you want to change the base?
Conversation
6f9356d to
f79a30b
Compare
|
Wow, I really need this capability. And I like the "wrap" mechanisms; they're very elegant for this scenario.
From my perspective, using
I agree that releasing the corresponding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds multi-host support to weak-node-api, enabling multiple Node-API implementations to coexist in a single process. The implementation wraps opaque Node-API pointers (napi_env, napi_threadsafe_function, napi_async_cleanup_hook_handle) with metadata tracking which host owns them, allowing proper delegation of API calls to the correct implementation.
Key changes:
- Introduces
WeakNodeApiMultiHostclass with wrapper mechanism for opaque Node-API types - Generates multi-host header and source files alongside existing weak-node-api files
- Adds comprehensive test coverage for multi-host scenarios including host lifecycle and wrapped pointer handling
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/weak-node-api/tests/test_multi_host.cpp |
Comprehensive test suite validating multi-host injection, call routing, host lifecycle, and wrapped opaque pointer handling |
packages/weak-node-api/tests/CMakeLists.txt |
Adds new multi-host test file to the build configuration |
packages/weak-node-api/scripts/generators/multi-host.ts |
Code generator for multi-host header and implementation, including wrapper creation and call delegation logic |
packages/weak-node-api/scripts/generate-weak-node-api.ts |
Updates generator to produce multi-host files with documentation headers |
packages/weak-node-api/CMakeLists.txt |
Includes generated multi-host source and header files in the build |
packages/weak-node-api/.gitignore |
Simplifies ignore pattern to cover all generated files |
f79a30b to
4d78b71
Compare
5ff0a5e to
daf6686
Compare
cf5e056 to
bd952a6
Compare
4ab6985 to
0e545a5
Compare
|
|
||
| struct WrappedEnv; | ||
|
|
||
| struct WrappedThreadsafeFunction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we're calling the constructors of struct WrappedThreadsafeFunction and struct WrappedAsyncCleanupHookHandle with three arguments:
auto ptr = std::make_unique<WrappedAsyncCleanupHookHandle>(original, env, weak_host);
auto ptr = std::make_unique<WrappedThreadsafeFunction>(original, env, weak_host);When compiling locally, the compiler throws an error. I'm wondering if these structs should explicitly define a constructor that accepts these three parameters?😊
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/__memory/unique_ptr.h:686:30: error: no matching constructor for initialization of 'NodeApiMultiHost::WrappedAsyncCleanupHookHandle'
return unique_ptr<_Tp>(new _Tp(_VSTD::forward<_Args>(__args)...));
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
weak-node-api/generated/NodeApiMultiHost.cpp:240:12: note: in instantiation of function template specialization 'std::make_unique<NodeApiMultiHost::WrappedAsyncCleanupHookHandle, napi_async_cleanup_hook_handle__ *&, NodeApiMultiHost::WrappedEnv *&, std::weak_ptr<NodeApiHost> &>' requested here
std::make_unique<WrappedAsyncCleanupHookHandle>(original, env, weak_host);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supposed to use the default constructor of those structs 🤔 Do you have this in your generated/NodeApiMultiHost.hpp?
struct WrappedThreadsafeFunction {
napi_threadsafe_function value;
WrappedEnv *env;
std::weak_ptr<NodeApiHost> host;
};
struct WrappedAsyncCleanupHookHandle {
napi_async_cleanup_hook_handle value;
WrappedEnv *env;
std::weak_ptr<NodeApiHost> host;
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need to add explicit constructors? I don't know why it builds for me and on CI though 🤔
I've just rebased on latest main after #330 merged - pulling the latest might change something for you after a clean build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need to add explicit constructors? I don't know why it builds for me and on CI though 🤔 I've just rebased on latest main after #330 merged - pulling the latest might change something for you after a clean build?
I see now. My local compiler is configured with a lower C++ standard that doesn't support this parenthesis initialization syntax. That makes sense now. Thanks!😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great 👍 Perhaps I could add something to the CMake config to either configure the language version or fail faster?
daf6686 to
4cbf27b
Compare
|
Hi, @kraenhansen , auto host_foo = std::shared_ptr<WeakNodeApiHost>(new WeakNodeApiHost{
.napi_create_function =
[](napi_env env, const char* utf8name, size_t length, napi_callback cb,
void* callback_data, napi_value* result) -> napi_status {
// Omitted: actual implementation of napi_create_function across different JS engines
return napi_ok;
}});
// Create and inject a multi-host, then wrap two envs
WeakNodeApiMultiHost multi_host{nullptr, nullptr};
inject_weak_node_api_host(multi_host);
// Original napi_env
napi_env raw_env{};
// foo_env is a WrappedEnv
auto foo_env = multi_host.wrap(napi_env{}, host_foo);
napi_callback foo_cb = [](napi_env env, napi_callback_info info) -> napi_value {
// This `env` is not the WrappedEnv foo_env, but the original napi_env raw_env.
// Cannot directly call weak-node-api functions here.
return nullptr;
};
napi_value foo_fn = nullptr;
napi_create_function(foo_env, "foo", 3, foo_cb, nullptr, &foo_fn); |
23802a8 to
e82efc2
Compare
Ideally, you'd be able to capture the react-native-node-api/packages/weak-node-api/tests/test_inject.cpp Lines 38 to 63 in e82efc2
Perhaps you could adjust these or add a new test as an example of the issues you're seeing? |
I've added a test case try to illustrate my issues. 🙏 By the way, I've experimented with an alternative approach locally: injecting a This approach solves the issues I'm facing because it avoids having two This is the commit for And here's the related test case: RobinWuu@8c68ff4 |
|
Okay, reading the comments in depth, I believe I understand your need and problem 👍 Thanks for bearing with me 🙂 This line in my test wont work in a real scenario, since the global functions expect a wrapped env and I'm calling it with a raw unwrapped env:
And passing the host through callback info won't work because the weak-node-api concept is supposed to be opaque from the addon's POW
It's actually a pretty fundamental flaw in the design 🤔 Hmmm ... Possible solution?A solution which come to mind would be to add wrapping of the
This might however be tricky with these callback arguments being raw function pointers 🤔 The main issue here is that we'd need to store the wrapped env behind the data pointer, which is read via the second argument to the callback (of type Here's how far I got in my experiments with an attempt to create a (Notice the TODO , which I don't know how to get around - yet). std::tuple<napi_callback, void *>
NodeApiMultiHost::WrappedEnv::wrap(napi_callback original_calback,
void *original_data, WrappedEnv *env,
std::weak_ptr<NodeApiHost> weak_host) {
auto ptr = std::make_unique<WrappedCallback>(original_calback, original_data,
env, weak_host);
auto raw_ptr = ptr.get();
env->callbacks.push_back(std::move(ptr));
napi_callback cb = [](napi_env env, napi_callback_info info) -> napi_value {
// This function is called by the "real" host with an unwrapped env, which
// we'd want to wrap before calling the actual callback
void *data;
napi_get_cb_info(env, info, nullptr, nullptr, nullptr, &data);
auto *callback = static_cast<decltype(raw_ptr)>(data);
assert(callback != nullptr);
if (auto host = callback->host.lock()) {
// Call the original callback with the wrapped env
// TODO: We cannot construct a new napi_callback_info which points to th original data 😬
callback->value(reinterpret_cast<napi_env>(callback->env), nullptr);
} else {
fprintf(
stderr,
"Wrapped Node-API callback was called after host was destroyed\n");
abort();
}
};
return {cb, raw_ptr};
}I had a look at your getter approach - to be clear, does that solve this issue in your view? |
This is my suggestion for adding "multi-host" support to
weak-node-api, enabling multiple engines implementing Node-API to co-exist and share the Node-API function namespace. While not needed specifically for bringing Node-API to React Native adding this could makeweak-node-apimore applicable in other scenarios where multiple engines implementing Node-API share a single process.I'm proposing adding mechanisms to "wrap" the opaque pointers of specific Node-API implementors with references to the
NodeApiHostobject to enable deferring implementation of Node-API functions based on thenapi_env,node_api_basic_env,napi_threadsafe_functionornapi_async_cleanup_hook_handlepassed.classDiagram class NodeApiHost { <<interface>> } class NodeApiMultiHost { -vector~unique_ptr~WrappedEnv~~ envs +wrap(napi_env, weak_ptr~NodeApiHost~) napi_env +static napi_* methods... } class WrappedEnv { +napi_env value +weak_ptr~NodeApiHost~ host -vector~unique_ptr~WrappedThreadsafeFunction~~ threadsafe_functions -vector~unique_ptr~WrappedAsyncCleanupHookHandle~~ async_cleanup_hook_handles +wrap(napi_threadsafe_function, WrappedEnv*, weak_ptr~NodeApiHost~) napi_threadsafe_function +wrap(napi_async_cleanup_hook_handle, WrappedEnv*, weak_ptr~NodeApiHost~) napi_async_cleanup_hook_handle } class WrappedThreadsafeFunction { +napi_threadsafe_function value +WrappedEnv* env +weak_ptr~NodeApiHost~ host } class WrappedAsyncCleanupHookHandle { +napi_async_cleanup_hook_handle value +WrappedEnv* env +weak_ptr~NodeApiHost~ host } NodeApiMultiHost --|> NodeApiHost : inherits NodeApiMultiHost *-- "0..*" WrappedEnv : owns WrappedEnv *-- "0..*" WrappedThreadsafeFunction : owns WrappedEnv *-- "0..*" WrappedAsyncCleanupHookHandle : owns WrappedThreadsafeFunction --> WrappedEnv : references WrappedAsyncCleanupHookHandle --> WrappedEnv : references WrappedEnv --> NodeApiHost : weak reference WrappedThreadsafeFunction --> NodeApiHost : weak reference WrappedAsyncCleanupHookHandle --> NodeApiHost : weak reference WrappedEnv ..|> enable_shared_from_this : implements note for NodeApiMultiHost "Manages multiple Node-API host implementations" note for WrappedEnv "Wraps napi_env with ownership tracking for threadsafe functions and async cleanup hook handles"WrappedEnv,WrappedThreadsafeFunctionandWrappedAsyncCleanupHookHandleobjects can then be passed around like their respective opaque pointers and are "unwrapped" in the internal implementation of the "multi host" implementation of Node-API functions.Wrappedobjects are created calling one of thewrapinstance methods onNodeApiMultiHostorWrappedEnv, which are called internally innapi_create_threadsafe_functionandnapi_add_async_cleanup_hooktoo.Usage
NodeApiMultiHost(providing functions to register modules and handling a fatal error)NodeApiMultiHost(callingmulti_host.wrap(original_env, host);)react-native-node-api/packages/weak-node-api/tests/test_multi_host.cpp
Lines 18 to 52 in f79a30b
TODO
WrappedEnv(besides deleting the entireNodeApiMultiHost)WrappedThreadsafeFunction(in somenapi_*_threadsafe_functionfunction?) andWrappedAsyncCleanupHookHandle(innapi_remove_async_cleanup_hook).Open questions
std::functioninstead of raw function pointers for all (or some) of theWeakNodeApiHostmembers? This would allow capturing lambdas, making it much easier to provide a meaningful implementation of for examplenapi_module_register.Generated code
Below are samples from the generated code:
napi_create_objectimplementationnapi_create_threadsafe_functionandnapi_add_async_cleanup_hookimplementationsNotice the calls to
wrap, wrapping their opaque "out" pointers.