Skip to content
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 Runtime Semantics Versioning #45657

Closed
legendecas opened this issue Nov 28, 2022 · 12 comments
Closed

Node-API Runtime Semantics Versioning #45657

legendecas opened this issue Nov 28, 2022 · 12 comments
Labels
discuss Issues opened for discussions and feedbacks. node-api Issues and PRs related to the Node-API.

Comments

@legendecas
Copy link
Member

legendecas commented Nov 28, 2022

The Problem

Today Node-API has no runtime semantics based on the Node-API version targeted by the addon. This means that regardless of the Node-API version specified by the package, the same behavior is provided for a given Node-API function.

To maintain strict forward compatibility on future versions of Node.js for existing Node-API add-ons, the Node-API team has avoided introducing major bug fixes and features, like:

This is because bug fixes and features can break existing add-ons that rely on the current behavior (even if it is incorrect). For instance, if we enable the flags introduced in #36510 by default, add-ons that don’t handle the exception thrown in the context of thread safe function callbacks will crash the Node.js process.

This makes it hard for Node-API to evolve and address problems that cannot be solved without changes in behavior. We have addressed this in the past by adding duplicate functions with the modified behavior. This can be confusing to module owners and makes it harder to move packages towards using the improved/fixed behavior.

Proposed Solution

In order to address this challenge the Node-API team is considering introducing runtime semantics on Node-API versions.

What this means is that at runtime the version of Node-API targeted by a package when it is built will be used to determine how Node-API functions work. Since Node.js will continue to support older Node-API versions, the API will be stable for a given Node-API version. For example, a package that set Node-API version 8 as its target will continue to compile and run on newer versions of Node.js.

If this proposal is adopted this is what would be different:

  1. Packages would no longer get the latest Node-API version supported by the version against which they are compiled by default. The Node-API version will be pinned at Node-API version 8. Package authors will need to explicitly opt-in to a newer Node-API version.
  2. Opting into a newer Node-API version could result in changes in behavior which in turn may require changes in the code for the package.
  3. Opting into a version of Node-API later than 8 may require that the package use different macros/functions for registration.

This would allow us to better evolve Node-API while at the same time maintaining ABI compatibility for packages at a given Node-API version. It does, however, mean that changes in the code for a package might be required in order to use new features in later Node-API versions. We currently consider this a reasonable trade-off as the package author is likely already changing/testing the code if they want to use new features.

Implementation Discussion

Currently, ABI-safe Node-API modules have two ways to register modules:

  1. Create a napi_module struct and call napi_module_register on the module library (.so or .dll) load.
  2. Define the well-known symbol napi_register_module_v1 function which is called on-demand to initialize module exports.

Those two have no significant differences. However, well-known symbol registration is used in various language bindings (Rust, go, etc). Even the C/C++ WASM bindings use symbol-based registration. It is easier to implement, and it does not need calling code on module library load.

Thus, we are going to deprecate napi_module struct registration in #46319. napi_module struct registration is still supported and its support is not going to be removed. Add-ons defining their entries with NAPI_MODULE and NAPI_MODULE_INIT are switching to the well-known symbol registration once they are compiled with the latest node_api.h headers.

Declare target node-api version

Additionally, A new well-known symbol napi_module_get_api_version_v1 is added to declare add-ons' target node-api version.

typedef int32_t(NAPI_CDECL* napi_addon_get_api_version_func)();

If an add-on's napi_module_get_api_version_v1 returned a version that is greater than the node-api version that the current Node.js is built with, Node.js should reject to load it.

If an add-on didn't define such a symbol, its target node-api version is pinned at node-api version 8 -- the last version we introduce runtime semantics versioning.

Add-on can assert the node-api version returned by napi_get_version is greater than or equal to its target version.

Future Plans

Before we ship these changes, we need to reach out to add-on authors and major Node-API language library (e.g. Rust) authors to see their opinions on the approach.

Future plans and improvements with the runtime semantics versioning:

  • Enable uncaught exception emitting by default for thread-safe functions.
  • Allow napi_create_reference to create strong references on the primitive values.
  • Eagerly invoke finalize callback, with JavaScript execution disabled, to improve the performance of native data finalization.

/cc @nodejs/node-api

@legendecas legendecas added node-api Issues and PRs related to the Node-API. discuss Issues opened for discussions and feedbacks. labels Nov 28, 2022
@bnoordhuis
Copy link
Member

Does your approach mean that I, as an add-on author, can't opt in until the day v18.x goes out of support if I don't want to risk breaking compatibility with older v18.x releases? If so, that's a pretty big drawback.

@legendecas
Copy link
Member Author

@bnoordhuis Does your approach mean that I, as an add-on author, can't opt in until the day v18.x goes out of support if I don't want to risk breaking compatibility with older v18.x releases? If so, that's a pretty big drawback.

Yeah, this is a problem when add-on authors are using new node-api functions too -- these functions are backported to all release lines, but older releases still do not support those functions and refuse to load those node-api add-ons.

Similarly, this mechanism needs to go through the node-api experimental phase, like all new node-api functions. In this phase, it is backported to all supported release lines. When the mechanism goes out of experimental, we've already shipped it in many releases.

@mhdawson
Copy link
Member

I agree with @legendecas I don't think this proposal changes things in terms of the issue @bnoordhuis mentioned. If you want to leverage a newer Node-API level then the addon will only run on versions that support that level. If the Node-API level is backported that likely means that at least some earlier vesions in the line won't be able to support the addon.

@bnoordhuis
Copy link
Member

It's needlessly limiting though, isn't it?

If I understand #36510 right, if I could fix my add-on and programmatically (i.e., at runtime) opt into the new behavior, it'd run okay with newer node releases and not any worse than before with older releases.

With the compile-time option, I have to give up compatibility with older releases. Seems strictly worse to me.

Another problem: it doesn't work for add-ons that register themselves through the napi_register_module_v1 magic symbol. Those don't have a struct napi_module.

@legendecas
Copy link
Member Author

legendecas commented Nov 30, 2022

If I understand #36510 right, if I could fix my add-on and programmatically (i.e., at runtime) opt into the new behavior, it'd run okay with newer node releases and not any worse than before with older releases.

With the compile-time option, I have to give up compatibility with older releases. Seems strictly worse to me.

Add-ons can already catch the exception themselves and emit those exceptions as uncaught with napi_fatal_exception. The crux here is that we can not change the default behavior for new add-ons even if the status quo can be problematic. And every existing add-ons and new add-ons must programmatically opt-in to the expected behavior. The idea of the runtime version semantics is that we can encourage new add-ons to start with the fixed behavior and existing add-ons to gradually migrate to the fixed behavior.

Another problem: it doesn't work for add-ons that register themselves through the napi_register_module_v1 magic symbol. Those don't have a struct napi_module.

napi_module_register requires an napi_module record.

@Brooooooklyn
Copy link

This seems to have no effect on NAPI-RS, which controls the compilation of the target Node-API version through the feature flags, and will report an error at the compilation stage if a Node-API other than the target is used.

@bnoordhuis
Copy link
Member

napi_module_register requires an napi_module record.

napi_register_module_v1 init functions don't call that function.

@legendecas
Copy link
Member Author

legendecas commented Dec 1, 2022

napi_module_register requires an napi_module record.

napi_register_module_v1 init functions don't call that function.

@bnoordhuis I may not understand your points on napi_register_module_v1 correctly. This is not part of the node-api interface. It is a convention name of the module initializer as a result of NAPI_MODULE_INIT. Add-ons can define their own initializer with their own choice of name with NAPI_MODULE. However, both macro registers the napi_module record with napi_module_register. Would you mind expanding on this?

Update: sorry, I may get myself confused. I'll revisit this part.

@mhdawson
Copy link
Member

mhdawson commented Dec 1, 2022

I think part of what @bnoordhuis is pointing out is that the feature flag type approach may allow people to opt int to some subeset of changes more easily than having to target a new Node-API level .

@vmoroz
Copy link
Member

vmoroz commented Dec 2, 2022

@bnoordhuis, it seems that napi-rs project uses term "feature" to indicate the same meaning as the NAPI_VERSION macro in the Node-API C header files. Is it correct? If yes, then the use of NAPI_VERSION to modify internal behavior discussed in this proposal is essentially the same as how napi-rs uses features.

In #42557 the proposal to use features has a different meaning. The features there are rather Boolean values that can tweak behavior. Each new version of Node-API will have its own default set of features, but module can modify it if needed. In our Node-API meetings we discussed that if we have too many of them, then it will be increasingly difficult to maintain them. So, when Node-API team says "features" we use the meaning as in the PR #42557, and not as in the napi-rs feature flags.

I looked today into the issue with the "magical" napi_register_module_v1 function. I see it being used in the napi-cs and napi-dotnet projects. I guess it was taken from the napi-rs use (not sure about it). I think we can address it by adding another optional "magical" function napi_get_module_api_version_v1 to return the NAPI_VERSION used by the module. Since the NAPI_MODULE_VERSION is used to define the names of these "magical" functions, we should probably avoid changing it.

@legendecas, I do not feel comfortable with the idea of passing NAPI_VERSION as napi_module.nm_version field. I believe this field indicates the version of the struct rather than version of the API used by module. We can use one of the reserved fields instead of changing semantic of the existing field.

@legendecas
Copy link
Member Author

@vmoroz I do not feel comfortable with the idea of passing NAPI_VERSION as napi_module.nm_version field. I believe this field indicates the version of the struct rather than version of the API used by module. We can use one of the reserved fields instead of changing semantic of the existing field.

I should have updated the document to save the NAPI_VERSION in the reserved field instead. If you find anything incorrect, please feel free to point it out. Thank you.

@legendecas
Copy link
Member Author

Done in #45715.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

5 participants