src: add support for nogc types via `NogcEnv` #1514

KevinEady · 2024-06-06T12:42:17Z

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

TODO:

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

codecov-commenter · 2024-06-06T14:17:48Z

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 (bf49519) to head (6591a36).
Report is 3 commits behind head on main.

Files	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 · 2024-06-08T13:41:33Z

@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 · 2024-06-10T20:02:55Z

.github/workflows/ci.yml

      matrix:
-        node-version: [ 18.x, 20.x, 21.x, 22.x ]
+        api_version:


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

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 · 2024-06-14T15:16:00Z

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

- Use post finalizer to delete reference in `~Reference` - Remove const ref from comparison operator in `NogcEnv` - Remove leftover debugging statements

KevinEady · 2024-06-15T21:17:53Z

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 · 2024-06-22T04:44:34Z

test/finalizer_order.js

+      if (isExperimental) {
+        assert.strictEqual(binding.finalizer_order.Test.isNogcFinalizerCalled, true, 'Expected nogc finalizer to be called [before ticking]');
+        assert.strictEqual(binding.finalizer_order.Test.isGcFinalizerCalled, false, 'Expected gc finalizer to not be called [before ticking]');
+        assert.strictEqual(isCallbackCalled, false, 'Expected callback not be called [before ticking]');


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 · 2024-06-24T15:05:45Z

napi.h

@@ -2415,6 +2448,7 @@ class ObjectWrap : public InstanceWrap<T>, public Reference<Object> {
      napi_property_attributes attributes = napi_default);
  static Napi::Value OnCalledAsFunction(const Napi::CallbackInfo& callbackInfo);
  virtual void Finalize(Napi::Env env);
+  virtual void Finalize(NogcEnv env);


Suggested change

virtual void Finalize(NogcEnv env);

virtual void Finalize(Napi::NogcEnv env);

gabrielschulhof · 2024-06-24T15:56:53Z

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 · 2024-06-24T23:40:46Z

We should rename AddPostFinalizer to just PostFinalizer.

KevinEady · 2024-06-26T12:06:44Z

@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 · 2024-07-04T10:25:06Z

napi.h

- private:
-  napi_env _env;
+class NogcEnv {
+ protected:


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.

rename NoGcEnv to BasicEnv, AddPostFinalizer to PostFinalizer, NoGc/GcFinalizer to Sync/AsyncFinalizer

KevinEady · 2024-07-05T14:32:59Z

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

vmoroz reviewed Jun 10, 2024

View reviewed changes

KevinEady mentioned this pull request Jun 10, 2024

src: restore ability to run under NAPI_EXPERIMENTAL #1409

Merged

mhdawson added the SemVer-major label Jun 14, 2024

KevinEady force-pushed the add-nogc-types branch 2 times, most recently from 7926d7a to bcdde7b Compare June 15, 2024 18:57

KevinEady added 6 commits June 15, 2024 21:01

src: introduce NogcEnv; support for nogc finalizers

c64d8cf

Address comments in 8 Jun 2024 Node-API meeting:

4a112dd

- Use post finalizer to delete reference in `~Reference` - Remove const ref from comparison operator in `NogcEnv` - Remove leftover debugging statements

sfinae FinalizeData::Wrapper

c69554f

fix test/addon_build for experimental

5102d3f

sfinae ObjectWrap::Finalize

e7e8a81

revert tests

61cd1ec

KevinEady force-pushed the add-nogc-types branch from bcdde7b to 61cd1ec Compare June 15, 2024 19:02

add finalizer_order test

83af80c

KevinEady force-pushed the add-nogc-types branch from 674fa28 to 83af80c Compare June 15, 2024 20:57

KevinEady added 2 commits June 16, 2024 14:17

remove unnecessary defines

7c949dd

add AddPostFinalizer test

321fe1f

KevinEady force-pushed the add-nogc-types branch from fc5dc94 to 321fe1f Compare June 21, 2024 14:08

gabrielschulhof approved these changes Jun 22, 2024

View reviewed changes

gabrielschulhof reviewed Jun 24, 2024

View reviewed changes

gabrielschulhof mentioned this pull request Jun 24, 2024

idea: detect JS calls at compile time #1367

Closed

gabrielschulhof mentioned this pull request Jun 29, 2024

Discussion: Naming of envs #1527

Open

KevinEady commented Jul 4, 2024

View reviewed changes

renaming ideas

fc73ba9

rename NoGcEnv to BasicEnv, AddPostFinalizer to PostFinalizer, NoGc/GcFinalizer to Sync/AsyncFinalizer

wip on docs for general finalization

6591a36

KevinEady force-pushed the add-nogc-types branch from 502aee5 to 6591a36 Compare July 5, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add support for nogc types via `NogcEnv` #1514

src: add support for nogc types via `NogcEnv` #1514

KevinEady commented Jun 6, 2024 •

edited

Loading

codecov-commenter commented Jun 6, 2024 •

edited

Loading

KevinEady commented Jun 8, 2024

vmoroz Jun 10, 2024

KevinEady Jun 10, 2024

mhdawson commented Jun 14, 2024

KevinEady commented Jun 15, 2024

gabrielschulhof left a comment •

edited

Loading

gabrielschulhof Jun 22, 2024

gabrielschulhof Jun 24, 2024

gabrielschulhof commented Jun 24, 2024

gabrielschulhof commented Jun 24, 2024

KevinEady commented Jun 26, 2024

KevinEady Jul 4, 2024 •

edited

Loading

KevinEady commented Jul 5, 2024

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

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

src: add support for nogc types via NogcEnv #1514

Are you sure you want to change the base?

src: add support for nogc types via NogcEnv #1514

Conversation

KevinEady commented Jun 6, 2024 • edited Loading

codecov-commenter commented Jun 6, 2024 • edited Loading

Codecov Report

KevinEady commented Jun 8, 2024

vmoroz Jun 10, 2024

Choose a reason for hiding this comment

KevinEady Jun 10, 2024

Choose a reason for hiding this comment

mhdawson commented Jun 14, 2024

KevinEady commented Jun 15, 2024

gabrielschulhof left a comment • edited Loading

Choose a reason for hiding this comment

gabrielschulhof Jun 22, 2024

Choose a reason for hiding this comment

gabrielschulhof Jun 24, 2024

Choose a reason for hiding this comment

gabrielschulhof commented Jun 24, 2024

gabrielschulhof commented Jun 24, 2024

KevinEady commented Jun 26, 2024

KevinEady Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

KevinEady commented Jul 5, 2024

src: add support for nogc types via `NogcEnv` #1514

src: add support for nogc types via `NogcEnv` #1514

KevinEady commented Jun 6, 2024 •

edited

Loading

codecov-commenter commented Jun 6, 2024 •

edited

Loading

gabrielschulhof left a comment •

edited

Loading

KevinEady Jul 4, 2024 •

edited

Loading