feat: add support for nogc types via BasicEnv

[KevinEady]

• Introduce Napi::NogcEnv class, taking the place of Napi::Env in finalizer callbacks.
• Introduce void Napi::NogcEnv::AddPostFinalizer() and overloads.
• Modify tests to support above changes

Closes: #1508

TODO:

• docs
• discuss implementation, namings, etc
• wait for src: restore ability to run under NAPI_EXPERIMENTAL #1409 to land, and incorporate changes

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 69.38776% with 15 lines in your changes missing coverage. Please review.

Project coverage is 64.40%. Comparing base (12ffe91) to head (195ec28).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
napi-inl.h	70.83%	2 Missing and 12 partials ⚠️
napi.h	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1514      +/-   ##
==========================================
- Coverage   64.69%   64.40%   -0.30%     
==========================================
  Files           3        3              
  Lines        1997     2003       +6     
  Branches      687      693       +6     
==========================================
- Hits         1292     1290       -2     
- Misses        144      146       +2     
- Partials      561      567       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KevinEady]

@vmoroz your comment on removing const ref on std::nullptr_t has been addressed, and @mhdawson your comment on reference deletion has been addressed, both in 2a689b6

[vmoroz]

Should we add checks for the standard/experimental to ci-win.yml too?

[KevinEady]

Good thought... I pretty much copied Gabriel's CI file. I'll comment on his PR to verify, since mine would end up being overwritten when i rebase to master after his is merged.

[mhdawson]

Agreed we should do a release to get out the previous fixe that @gabrielschulhof made and then put this out in a SemVer major.

[KevinEady]

Hi team,

Using some SFINAE magic, I was able to determine at compile time how to associate the finalizer -- either directly or with node_api_post_finalizer. This would address #1367 as implemented.

This means no changes were needed to any existing tests, and I created a new test in finalizer_order to ensure finalizers are being called correctly.

PTAL! @gabrielschulhof @vmoroz @legendecas @mhdawson

[gabrielschulhof]

Looks good!

I think the Gc vs. NoGc nomenclature is a bit confusing. Unfortunately, we established this in core. I think maybe we should go with DuringGc and OutsideGc. For env, we'd have DuringGcEnv and Env, since Env is already established, but for the finalizer type names I think we can go with DuringGc and OutsideGc for clarity.

[gabrielschulhof]

Suggested change

	assert.strictEqual(isCallbackCalled, false, 'Expected callback not be called [before ticking]');
	assert.strictEqual(isCallbackCalled, false, 'Expected callback to not be called [before ticking]');

[gabrielschulhof]

Suggested change

	virtual void Finalize(NogcEnv env);
	virtual void Finalize(Napi::NogcEnv env);

[gabrielschulhof]

I tested this with Napi::External and it works great!

const { makeSync, makeAsync } = require('bindings')('test_sync_fini')

const doAsync = process.argv[2] === 'async'

;(function test(iter = 0) {
  for (let i = 0; i < 1000000; i++) {
    const id = `${iter}:${i}`
    if (doAsync) {
      makeAsync(id)
    } else {
      makeSync(id)
    }
  }
  setTimeout(test, 0, iter + 1)
})()

Here's a video of the output. Guess which column is async 🙂

Screen.Recording.2024-06-24.at.8.46.57.AM.mov

[gabrielschulhof]

We should rename AddPostFinalizer to just PostFinalizer.

[KevinEady]

@gabrielschulhof ,

For env, we'd have DuringGcEnv and Env, since Env is already established, ...

One could argue that being a Node-API wrapper, the naming in this library should be as consistent with Node-API as possible. The (almost) 1-to-1 mapping helps link up with other documentation (eg. referencing the Node-API docs), or translating between Node-API C code and node-addon-api C++ code.

I do agree though, the name DuringGcEnv better describes the restriction ("during GC") on how the environment can be used, but not sure if we should stray from the Node-API names.

---

... but for the finalizer type names I think we can go with DuringGc and OutsideGc for clarity.

By "finalizer type names" do you mean changing the Finalizer in places eg.:

template <typename Finalizer>
static External New(napi_env env, T* data, Finalizer finalizeCallback);

If so, I don't think we should change this name since the finalizer passed can accept either a DuringGcEnv or Env. A Finalizer is a callback ran either during the GC cycle (if using DuringGcEnv) or on a future tick after the GC cycle completes (if using Env). This means the concept of "finalization" is dependent on your callback signature, and would be documented as such.

---

We should rename AddPostFinalizer to just PostFinalizer.

Makes sense, esp. considering what I said previously.

---

While on the subject of documentation: we don't have any specific entry for "finalization", as a finalizer callback is always documented on each individual object or function, eg:

• Napi::External:
  

  node-addon-api/doc/external.md

  Line 7 in bf49519

  `Napi::External` objects can be created with an optional Finalizer function and optional Hint value. The Finalizer function, if specified, is called when your `Napi::External` object is released by Node's garbage collector. It gives your code the opportunity to free any dynamically created data. If you specify a Hint value, it is passed to your Finalizer function.

• Napi::Buffer::New:
  

  node-addon-api/doc/buffer.md

  Lines 73 to 75 in bf49519

  	- `[in] finalizeCallback`: The function to be called when the `Napi::Buffer` is
  	destroyed. It must implement `operator()`, accept an Napi::Env, a `T*` (which is the
  	external data pointer), and return `void`.

• and others

I propose we create a new top-level documentation page for finalization describing the above, and can have each use of finalizers in the documentation link to this page.

[KevinEady]

Should we make this private and have Env be a friend class? With it being protected, the default finalizers (as well as _env) would be exposed in any user-defined classes that extend NogcEnv.

EDIT: I went with this private-friend approach, making this comment out-dated. Can discuss in meeting.

[KevinEady]

I've added some WIP documentation on the top-level finalization page: https://github.com/nodejs/node-addon-api/blob/6591a3609c2b94f22956b72736e06b86586556cf/doc/finalization.md

[KevinEady]

@mhdawson @gabrielschulhof as discussed in the 5 July Node-API meeting, I updated the docs to use the concept of a "basic finalizer" as a special categorization of finalizers, and removing the concept of "[a]synchronous". PTAL: https://github.com/nodejs/node-addon-api/blob/7d7b8211552216657494144d0417bf57b690871e/doc/finalization.md

Once we finalize (hah) this verbiage, I will update any associated type names in the headers + tests, and update the other docs that have any finalization features.

[gabrielschulhof]

@KevinEady the doc LGTM 👍

[legendecas]

(this is still a draft PR, is it ready for review?)

[mhdawson]

doc LGTM with a few suggestions:

1. I would remove

NOTE: Optimizations via basic finalizers will only occur if using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, the engine will not differentiate between basic and (extended) finalizers.

there is never any promise that optimization will take place so I think we can just remove that.

2. In terms of

In addition to passing finalizers to Napi::Externals and other Node-API constructs, use Napi::BasicEnv::PostFinalize(Napi::BasicEnv, Finalizer) to schedule a callback to run outside of the garbage collector finalization.

can we explain without promising it will run outside of gc collection? Maybe something like "to maximize the likelyhood that the finalization not being tied to garbage collection finalization" ...

[KevinEady]

@legendecas the overall C++ work/implementation is finished, but the PR still needs some more work (markdown docs, test docs, ...)

@mhdawson

1. I would remove

NOTE: Optimizations via basic finalizers will only occur if using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, the engine will not differentiate between basic and (extended) finalizers.

there is never any promise that optimization will take place so I think we can just remove that.

With this, I was trying to convey that using basic finalizers only changes functionality in experimental mode. I could see someone using basic finalizers without experimental mode, running into #1213 and being a bit confused. Maybe we can use some other verbiage?

NOTE: Node-API will only differentiate between basic and (extended) finalizers when using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, basic finalizers execute in the same manner as (extended) finalizers.

What do we think of that?

---

2. In terms of

In addition to passing finalizers to Napi::Externals and other Node-API constructs, use Napi::BasicEnv::PostFinalize(Napi::BasicEnv, Finalizer) to schedule a callback to run outside of the garbage collector finalization.

can we explain without promising it will run outside of gc collection?

Well, I took inspiration from the Node-API docs for node_api_post_finalizer:

node_api_post_finalizer helps to work around this limitation by allowing the add-on to defer calls to such Node-APIs to a point in time outside of the GC finalization.

Also, when you say:

"to maximize the likelyhood that the finalization not being tied to garbage collection finalization"

The API is intended to guarantee the callback does not run inside GC, so saying "maximize the likelihood" is not completely accurate, as it 100% will not run inside GC finalization.

Since this API strictly deals with callbacks + GC finalization order, I think it's a good thing to keep.

[KevinEady]

In Node API meeting 19 July, we discussed:

1. I would remove

NOTE: Optimizations via basic finalizers will only occur if using NAPI_EXPERIMENTAL and the NODE_API_EXPERIMENTAL_NOGC_ENV_OPT_OUT define flag has not been set. Otherwise, the engine will not differentiate between basic and (extended) finalizers.

there is never any promise that optimization will take place so I think we can just remove that.

Yes, remove this.

---

2. In terms of

In addition to passing finalizers to Napi::Externals and other Node-API constructs, use Napi::BasicEnv::PostFinalize(Napi::BasicEnv, Finalizer) to schedule a callback to run outside of the garbage collector finalization.

can we explain without promising it will run outside of gc collection?

The name node_api_post_finalizer can be made in the context of "running (non-basic) code after (post) a finalizer" , so maybe we can change the documentation in core (and also here) if needed, but the current wording is fine.

---

Additionally, we discussed adding an opt-in flag that requires -- at compilation time -- all finalizers are of the basic type, under the realization that:

• if using the C-based Node-API, opting into basic finalizers requires all finalizers to be basic, providing devs a way to guarantee all of their finalizers would be run in the "maybe optimized" way and fail compilation if callbacks do not match the current basic finalizer signature.
• if using this wrapper, (currently) there is no guarantee that all finalizers will be of the basic signature, meaning developers do not have an easy way to forcefully get compilation errors for finalizers that are not basic.

I suggest using a define: NODE_API_REQUIRE_BASIC_FINALIZERS NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS

Changes since last approval (22 June)

[KevinEady]

The Windows experimental CI passed, eg: test (experimental, 20.x, x64, windows-2019)

Testing with Node-API Version '2147483647'.

@legendecas @mhdawson @gabrielschulhof @vmoroz PTAL! 👍

[mhdawson]

LGTM

[legendecas]

Good work! Thanks

[KevinEady]

Hi @gabrielschulhof , IIRC you mentioned you had an in-progress review. Any movement on that?

[mhdawson]

Looks like all comments have been addressed and CI is green. Landing.

[KevinEady]

Hi @legendecas ,

This PR was merged into main, but the release-please action failed: https://github.com/nodejs/node-addon-api/actions/runs/10687066491/job/29623803353

I think it has to do with permissions?