-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
node-api: use c-based api for libnode embedding #54660
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
src/node_api_embedding.h
Outdated
// Skip printing output for --help, --version, --v8-options. | ||
node_api_platform_no_print_help_or_version_output = 1 << 12, | ||
// Initialize the process for predictable snapshot generation. | ||
node_api_platform_generate_predictable_snapshot = 1 << 14, |
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 think we should have an option which is something like
node_api_platform_nodejs_binary_default
which gives you the same configuration that is present for the node.js binary
src/node_api_embedding.h
Outdated
typedef struct node_api_env_options__* node_api_env_options; | ||
|
||
typedef enum { | ||
node_api_platform_no_flags = 0, |
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.
since a bunch of them seem to disable specific flags should there be an all_flags, or are they all on by default and then there are no/disable flags only?
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 have changed the approach since our last Node-API meeting. These flags are 1-to-1 mapping to the flags defined in the node.h. The default is the no_flags
configuration. Then, embedders can disable some default Node.js features.
We can add an alias for the no_flags
as a default_flags
.
src/node_api_embedding.cc
Outdated
return napi_ok; | ||
} | ||
|
||
napi_status NAPI_CDECL |
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.
If an engine does not support snapshots, can it just do nothing in the snapshot functions?
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 guess so. Maybe we can change it in a way that the snapshot can be just a JS text. In JSI they use term "prepared JavaScript" for the same purpose. The only question if we want this API to be Node-specific, or we rather target it to be Runtime/engine independent. E.g. I use this API with the jsr_
prefix across the V8 and Hermes JS engines (it is also based on the Node-API): https://github.com/microsoft/v8-jsi/blob/master/src/node-api/js_runtime_api.h
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.
Since you are already using it, being cross runtime might makes sense, just need to makes sure its easy for a platform to not support it and still have the same code run.
src/node_api_embedding.cc
Outdated
return std::move(env_setup_); | ||
} | ||
|
||
napi_status OpenScope() { |
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 assume that a scope is something different than a handle_scope - https://nodejs.org/api/n-api.html#napi_handle_scope, just wondering if there might be confusion between the concepts?
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.
Yes, it is different. When we are inside of a module we already have some current v8::Isolate
and v8::Context
. We do not have them when we are outside and operating with the environment. So, we must establish them to use any V8/Node API. In the standalone v8-jsi project I used a function jsr_run_task
that opens/closes the scope internally. (edit: I see that the v8-jsi also has the open/close scope. It is convenient to use when we do not want to create a lot of lambdas.)
src/node_api_embedding.cc
Outdated
return napi_ok; | ||
} | ||
|
||
napi_status NAPI_CDECL node_api_open_env_scope(napi_env env) { |
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'm wondering if all functions which are only to be called as part of embedding versus in an add-on implementation should have some extra bit in the name. For exampe in this method node_api_embed_open_env_scope
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.
This open/close scope API gives us a lot of flexibility, but it is difficult to use and like you said it is quite confusing.
I am currently considering to replace it with a function that receives a lambda (c function + void state), and then the napi_env
will be available only for that lambda. Other APIs will change from using napi_env
to something like node_embedding_env
or node_embedded_env
.
src/node_api_embedding.cc
Outdated
return napi_ok; | ||
} | ||
|
||
napi_status NAPI_CDECL node_api_await_promise(napi_env env, |
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.
At first I thought this might be an extension to the promise support we already have - https://nodejs.org/api/n-api.html#promises
This is a good example were I think we needed the embed or something else in the name as otherwise people might get confused and think it could be called from an addon.
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.
or maybe the prefix should be node_embedding_api_XXX
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.
What if we shorten it to the node_embedding_
?
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'm good with node_embedding_
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.
All API is changed to use the napi_embedding_
prefix.
Co-authored-by: Michael Dawson <mdawson@devrus.com>
Co-authored-by: Michael Dawson <mdawson@devrus.com>
This PR was discussed today 9/13/2024 in the Node-API meeting.
|
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.
Does the new C-based embedding API has a goal to do the same as the C++ embedding API?
- The answer is "yes" and "no", or better to say "it depends".
- While we want to have the same functionality, there is no goal to wrap up all existing C++ embedding APIs.
- The new C embedding API is going to grow based on the scenarios, and we hope that the Builder pattern let us evolve the API without ABI-breaking changes.
- The C embedding API is going to be implemented on the top of the existing C++ embedded API.
I think this question could be better addressed with an approach for embedders to opt into the "bleeding-edge" C++ API, like mentioned in #43542 (comment). An embedder can highly customize the behavior of V8/Node.js, e.g. Inspectors. If such advanced needs arise in an embedder that already adopted the C embedding API, I believe it would not be trivial for them to migrate to C++ based APIs. Allowing conversion between C/C++ API types would reduce the gaps for embedders using the two variant interfaces.
src/node_embedding_api.h
Outdated
|
||
// Initializes the Node.js platform. | ||
NAPI_EXTERN node_embedding_exit_code NAPI_CDECL | ||
node_embedding_platform_initialize(node_embedding_platform platform, |
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.
Flags and args are unsafe to be changed after initialization. I think node_embedding_platform_set_flags
and node_embedding_platform_set_args
should be merged in node_embedding_platform_initialize
.
To allow builder style API for ABI compatibility, we can add an opaque initialization parameter bag and pass it here:
typedef struct node_embedding_platform_init_opt__* node_embedding_platform_init_opt;
NAPI_EXTERN node_embedding_exit_code NAPI_CDECL
node_embedding_platform_initialize(node_embedding_platform platform,
node_embedding_platform_init_opt opt,
bool* early_return);
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.
Good point! I was also thinking along the same lines. As a result the "initialization" and "is_initialized" methods are removed and the "create" methods have a new configuration callback where all the builder set methods are called.
Based on your feedback I am going to go one more step further and introduce opaque "options" types so that all these "set_" and "on_" methods are only applicable to these "optons" types.
This way it would be easy to understand when it is safe to call them.
src/node_embedding_api.h
Outdated
|
||
// Runs the Node.js runtime event loop. | ||
NAPI_EXTERN node_embedding_exit_code NAPI_CDECL | ||
node_embedding_runtime_run_event_loop( |
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.
Even though libuv doesn't provide ABI guarantees, I think we should expose uv in embedder C API instead of wrapping it, allowing more flexibility of this C API since embedders have more control over libnode
.
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.
While I agree with you that it can provide more flexibility, my concern is that non-C/C++ users have to bind to a much broader set of APIs which like you said may not be ABI safe.
Also, while we offer functions that seems to do the same as the uv_run
, in practice they may be doing some more Node.js work such as draining the v8::Isolate
work items.
Currently the node_api.h
exposes the uv_loop_t
associated with the napi_env
. Technically, users can get it today. Though, we were discussing to deprecate it.
I also would like to explore a scenario where the UV loop is not used for processing the task queue, but we rather offer something like the V8 foreground task runner where the embedder API is responsible for pumping the task queue.
Thus, my proposal is to wait for a scenario where exposing the raw uv_loop_t
is required before we add it.
Adding new API is easy, deprecating is more difficult.
We have discussed the API today 09/20/2024 with @mhdawson. The key take aways:
|
I just do not see how it can be done in practice. The only real "escape hatch" is to add the missing functionality to the C API and compile the libnode privately until the PR is accepted by Node.js. Thus, the C API is designed to be extensible from the beginning. |
Note: this is an active work in progress and there are still a lot of code churning. You are welcome to comment on the code and share your thoughts, but please be aware that the code is not final yet.
This is a temporary spin off from the PR #43542.
This separate PR is created to simplify merging and rebasing with the latest code while we discuss the new API design.
When the code is ready it should be merged back to PR #43542.
The goal of the original PR is to enable C API and the Node-API for the embedded scenarios.
The C API allows using the shared
libnode
from runtimes that do not interop with C++ such as WASM, C#, Java, etc.This PR works towards the same goal with some changes to the original code.
This is the related issue #23265.
The API design principles
node_embedding_
.The API usage
node_embedding_platform
. It initializes Node and V8 JS engine once per process and parses the CLI arguments.node_embedding_runtime
s. A runtime is responsible for running JavaScript code.args
andexec_args
from the result of the platform initialization. They can be overridden while configuring the runtime.napi_api
instance. Any Node-API code that uses thenapi_env
must be run in the runtime scope controlled bynode_embedding_runtime_open_scope
andnode_embedding_runtime_close_scope
functions.The API overview
Based on the use scenarios, the API can be split up into six groups.
Error handling API
node_embedding_on_error
sets the global error handling hook.Global platform API
node_embedding_set_api_version
node_embedding_run_main
node_embedding_create_platform
node_embedding_delete_platform
node_embedding_platform_set_flags
node_embedding_platform_get_parsed_args
Runtime API
node_embedding_run_runtime
node_embedding_create_runtime
node_embedding_delete_runtime
node_embedding_runtime_set_flags
node_embedding_runtime_set_args
node_embedding_runtime_on_preload
node_embedding_runtime_on_start_execution
node_embedding_runtime_add_module
Runtime API to run event loops
node_embedding_runtime_set_task_runner
node_embedding_run_event_loop
node_embedding_complete_event_loop
node_embedding_terminate_event_loop
beforeExit
eventexit
eventRuntime API to interop with Node-API
node_embedding_run_node_api
node_embedding_open_node_api_scope
node_embedding_close_node_api_scope
Documentation
embedding.md
file after the C++ embedding API description.index.md
is changed to indicate that theembedding.md
has docs for C++ and C APIs.Tests
embedtest
executable can be run in several modes controlled by the first CLI argument. It effectively contains severalmain
functions for different test scenarios.The PR status
The code is not 100% complete yet. There are still a few TODO items, but I would like to start a discussion with the Node-API team about the new API.
implementation. - Complete implementation for non-Windows.
observer thread?
binding with other languages.