node-api: generalize finalizer second pass callback #44141

legendecas · 2022-08-05T07:09:07Z

Generalize the finalizer's second pass callback to make it cancellable and simplify the code around the second pass callback.

With this change, it is determined that Reference::Finalize/RefBase::Finalize are called once, either from the env's shutdown, or from the env's second pass callback.

All existing node-api js tests should pass without a touch. The js_native_api cctest is no longer applicable with this change, just removing it.

Refs: #44071

nodejs-github-bot · 2022-08-05T07:09:11Z

Review requested:

@nodejs/gyp
@nodejs/node-api

nodejs-github-bot · 2022-08-07T06:23:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45890/

test.parallel/test-diagnostics-channel-net

nodejs-github-bot · 2022-08-09T03:09:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45940/

Jenkins failure: https://ci.nodejs.org/job/node-test-commit-freebsd/45349/nodes=freebsd12-x64/

nodejs-github-bot · 2022-08-09T16:57:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/45952/

vmoroz · 2022-08-18T22:08:49Z

src/node_api.cc

+  if (!finalization_scheduled) {
+    finalization_scheduled = true;
+    Ref();
+    node_env()->SetImmediate([this](node::Environment* node_env) {


Capturing this is not safe: if the lambda is not executed, then the env instance is never released. It would be better to use the EnvRefHolder here which is being deleted in this PR from js_native_api_v8.h. This way the Unref() is always called when the lambda is released.
Also, when we call the Unref() below we must be prepared that it was the last Unref() that causes the env instance destruction. In such case the DrainFinalizerQueue() may be called against deleted object and cause issues.

The callback of Environment::SetImmediate is deemed to be called. So we don't need additional setups for the condition that the callback is not invoked.

vmoroz · 2022-08-18T22:29:23Z

src/node_api.cc

+  while (!pending_finalizers.empty()) {
+    v8impl::RefTracker* ref_tracker = *pending_finalizers.begin();
+    pending_finalizers.erase(ref_tracker);
+    ref_tracker->Finalize();


How do we handle uncaught JS exceptions? Previously the SetImmediate code was taking care about the exceptions. It may be worth adding a test where we have a few finalizers running and they all throw JS exceptions. The JS code should handle and ignore those exceptions - we must see all the finalizers executed.
Also, we need to test that if JS does not handle the uncaught exceptions, then we crash as usual.

node_napi_env__::CallFinalizer still handles JS exception. Native SetImmediate doesn't handle JS exceptions.

src/node_api.cc

Functions declared in anonymous namespaces are not necessarily to be marked as static. PR-URL: #44301 Refs: #44141 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com>

legendecas · 2022-10-14T07:14:15Z

@mhdawson thank you for verifying the patch with node-addon-api test sets.

I believe the problem here is what napi_remove_wrap should do when the out-param napi_ref result is set on napi_wrap. The document said that:

napi_remove_wrap:
Retrieves a native instance that was previously wrapped in the JavaScript object js_object using napi_wrap() and removes the wrapping. If a finalizer callback was associated with the wrapping, it will no longer be called when the JavaScript object becomes garbage-collected.
https://nodejs.org/api/n-api.html#napi_remove_wrap

However, I found that on the main branch, when the napi_remove_wrap is called but the napi_delete_reference is not called with the out-param napi_ref result, the finalizer can still be invoked. I've added a test case for it: https://github.com/nodejs/node/pull/44141/files#diff-8037a0113a35b91ac2d997501123d0b8be5b341907e9e350fe4f70082afa361f.

So basically, when the napi_wrap is called with non-null out-paramnapi_ref result, the expected behavior would be:

invoke napi_remove_wrap with the wrapped object to invalidate the wrap, after this point no finalizer should be invoked, and napi_get_reference_value can still get the ref-ed value from the out-param napi_ref result.
invoke napi_delete_reference with the out-param napi_ref result, to delete the user-owned napi_ref.

Generalize the finalizer's second pass callback to make it cancellable and simplify the code around the second pass callback. With this change, it is determined that Reference::Finalize or RefBase::Finalize are called once, either from the env's shutdown, or from the env's second pass callback. All existing node-api js tests should pass without a touch. The js_native_api cctest is no longer applicable with this change, just removing it.

nodejs-github-bot · 2022-11-18T16:46:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48011/

legendecas · 2022-11-21T09:21:13Z

@mhdawson updated, PTAL again, thank you :)

src/node_api.cc

mhdawson

LGTM, I ran the node-addon-api tests 10000 times with these changes and they all passed.

Generalize the finalizer's second pass callback to make it cancellable and simplify the code around the second pass callback. With this change, it is determined that Reference::Finalize or RefBase::Finalize are called once, either from the env's shutdown, or from the env's second pass callback. All existing node-api js tests should pass without a touch. The js_native_api cctest is no longer applicable with this change, just removing it. PR-URL: #44141 Refs: #44071 Reviewed-By: Michael Dawson <midawson@redhat.com>

legendecas · 2022-12-19T17:45:13Z

Landed in f14fa1b

Generalize the finalizer's second pass callback to make it cancellable and simplify the code around the second pass callback. With this change, it is determined that Reference::Finalize or RefBase::Finalize are called once, either from the env's shutdown, or from the env's second pass callback. All existing node-api js tests should pass without a touch. The js_native_api cctest is no longer applicable with this change, just removing it. PR-URL: #44141 Refs: #44071 Reviewed-By: Michael Dawson <midawson@redhat.com>

Functions declared in anonymous namespaces are not necessarily to be marked as static. PR-URL: nodejs/node#44301 Refs: nodejs/node#44141 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <midawson@redhat.com>

Generalize the finalizer's second pass callback to make it cancellable and simplify the code around the second pass callback. With this change, it is determined that Reference::Finalize or RefBase::Finalize are called once, either from the env's shutdown, or from the env's second pass callback. All existing node-api js tests should pass without a touch. The js_native_api cctest is no longer applicable with this change, just removing it. PR-URL: #44141 Refs: #44071 Reviewed-By: Michael Dawson <midawson@redhat.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 5, 2022

legendecas force-pushed the node-api/second-pass branch from 486add4 to 40562fc Compare August 5, 2022 07:17

legendecas added the node-api Issues and PRs related to the Node-API. label Aug 5, 2022

legendecas force-pushed the node-api/second-pass branch 2 times, most recently from e9ac1e2 to d71cdd6 Compare August 5, 2022 07:27

legendecas marked this pull request as ready for review August 5, 2022 07:41

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 5, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 5, 2022

This comment was marked as outdated.

Sign in to view

legendecas force-pushed the node-api/second-pass branch from d71cdd6 to 2a14bf3 Compare August 7, 2022 06:13

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2022

This was referenced Aug 8, 2022

CI Reliability 2022-08-08 nodejs/reliability#340

Open

CI Reliability 2022-08-09 nodejs/reliability#341

Open

vmoroz reviewed Aug 18, 2022

View reviewed changes

src/node_api.cc Outdated Show resolved Hide resolved

legendecas force-pushed the node-api/second-pass branch 3 times, most recently from 9416d37 to c6b1b47 Compare October 14, 2022 07:13

legendecas force-pushed the node-api/second-pass branch from c6b1b47 to 65fce2b Compare October 14, 2022 09:07

legendecas mentioned this pull request Nov 10, 2022

node-api: fix immediate napi_remove_wrap test #45406

Merged

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

legendecas and others added 2 commits November 18, 2022 09:33

fixup! verify dangling napi_ref in napi_wrap

344493a

legendecas force-pushed the node-api/second-pass branch from 65fce2b to 344493a Compare November 18, 2022 09:33

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2022

mhdawson reviewed Nov 22, 2022

View reviewed changes

src/node_api.cc Show resolved Hide resolved

mhdawson approved these changes Nov 22, 2022

View reviewed changes

legendecas closed this Dec 19, 2022

legendecas deleted the node-api/second-pass branch December 19, 2022 17:45

toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Dec 26, 2022

refactor: generalize finalizer second pass callback (nodejs/node#44141)

0b985bf

RafaelGSS mentioned this pull request Jan 2, 2023

v19.4.0 proposal #46061

Merged

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-api: generalize finalizer second pass callback #44141

node-api: generalize finalizer second pass callback #44141

legendecas commented Aug 5, 2022 •

edited

Loading

nodejs-github-bot commented Aug 5, 2022

This comment was marked as outdated.

nodejs-github-bot commented Aug 7, 2022 •

edited by legendecas

Loading

nodejs-github-bot commented Aug 9, 2022 •

edited by legendecas

Loading

nodejs-github-bot commented Aug 9, 2022

vmoroz Aug 18, 2022

legendecas Aug 19, 2022

vmoroz Aug 18, 2022

legendecas Aug 19, 2022

legendecas commented Oct 14, 2022 •

edited

Loading

nodejs-github-bot commented Nov 18, 2022

legendecas commented Nov 21, 2022

mhdawson left a comment

legendecas commented Dec 19, 2022

node-api: generalize finalizer second pass callback #44141

node-api: generalize finalizer second pass callback #44141

Conversation

legendecas commented Aug 5, 2022 • edited Loading

nodejs-github-bot commented Aug 5, 2022

This comment was marked as outdated.

nodejs-github-bot commented Aug 7, 2022 • edited by legendecas Loading

nodejs-github-bot commented Aug 9, 2022 • edited by legendecas Loading

nodejs-github-bot commented Aug 9, 2022

vmoroz Aug 18, 2022

Choose a reason for hiding this comment

legendecas Aug 19, 2022

Choose a reason for hiding this comment

vmoroz Aug 18, 2022

Choose a reason for hiding this comment

legendecas Aug 19, 2022

Choose a reason for hiding this comment

legendecas commented Oct 14, 2022 • edited Loading

nodejs-github-bot commented Nov 18, 2022

legendecas commented Nov 21, 2022

mhdawson left a comment

Choose a reason for hiding this comment

legendecas commented Dec 19, 2022

legendecas commented Aug 5, 2022 •

edited

Loading

nodejs-github-bot commented Aug 7, 2022 •

edited by legendecas

Loading

nodejs-github-bot commented Aug 9, 2022 •

edited by legendecas

Loading

legendecas commented Oct 14, 2022 •

edited

Loading