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

idea: detect JS calls at compile time #1367

Open
gabrielschulhof opened this issue Aug 16, 2023 · 4 comments
Open

idea: detect JS calls at compile time #1367

gabrielschulhof opened this issue Aug 16, 2023 · 4 comments

Comments

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Aug 16, 2023

Recording this idea so it doesn't slip under the radar:

In support of pure finalizers, I'm wondering if it's possible to templetize napi::Environment such that one specialization attaches js-execution-capable finalizers whereas the other attaches pure finalizers. We can use type inference to specialize the right class for a given user-provided finalizer.

That is, if the user's finalizer, after inlining and all, contains no calls to functions that execute JS, then we specialize the environment that attaches pure finalizers, otherwise we specialize the environment that does not. That way, add-on maintainers whose finalizers do not really execute javascript could simply recompile and have their memory leak worries go away without any code changes, once nodejs/node#42651 lands.

The reason I chose napi::Environment as the "probe" which will tell us whether the user's finalizer ends up calling a Node-API that executes JS is that all Node-APIs require an environment, so the type inference will snap to a non-pure napi::Environment if there is even one call to a JS-executing Node-API in the user's code.

We can actually work on this before nodejs/node#42651 lands, but for now we'll attach finalizers the same way whether or not their body calls into JS. Maybe we can use #warning to output compiler messages indicating whether the finalizer would have been pure or not.

Another aspect is this: if there are two napi::Environment classes available and you pass the wrong one to the API that sets up the finalizer, it will fail to compile if you pass a "pure" environment along with a finalizer that does not accept such a "pure" environment.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Sep 15, 2023

After discussing at today's Node-API meeting, we concluded that, in core, we should guard the new pure env functionality with an additional NODE_API_EXPERIMENTAL_PURE_ENV flag. The motivation and policy are as follow:

  • An additional flag the name of which contains the feature allows us to have multiple concurrent feature-specific flags within NAPI_EXPERIMENTAL, allowing adopters running with NAPI_EXPERIMENTAL to independently adopt each experimental feature.
  • The flags will be removed along with NAPI_EXPERIMENTAL when we make a new Node-API release.
  • The motivation for these flags is that we are breaking APIs we have already released and which are already stable, even though we're breaking them without any corresponding ABI breakage. This would cause the type of breakage beyond what we warn about in conjunction with the NAPI_EXPERIMENTAL flag.

@gabrielschulhof
Copy link
Contributor Author

nodejs/node#50060 is the first step. Then we have to turn on NODE_API_EXPERIMENTAL_PURE_ENV in node-addon-api and plumb the changes through its code base.

Copy link

github-actions bot commented Jan 5, 2024

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.

@github-actions github-actions bot added the stale label Jan 5, 2024
@legendecas legendecas removed the stale label Feb 2, 2024
Copy link

github-actions bot commented May 3, 2024

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.

@github-actions github-actions bot added the stale label May 3, 2024
@NickNaso NickNaso removed the stale label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants