node-api: run finalizers directly from GC #42651

vmoroz · 2022-04-08T05:51:05Z

The issue

Currently Reference finalizers are run inside of SetImmediate.
In case if user code creates a lot of native objects in the main script, it could cause a significant memory pressure, even if the objects are properly released. This is because they are "collected" only inside of SetImmediate that follows the script run.
See the issue: nodejs/node-addon-api#1140

In the a74a6e3 commit the processing of finalizers was moved from the GC second pass to the SetImmediate because:

finalizers may run some arbitrary JS code while some other JS code being executed.
finalizers may throw JavaScript exceptions which can affect behavior of other functions.

The solution

In this PR we are introducing new experimental behavior where the finalizers are run directly from the GC but they are not allowed to run JS code and must only call native code. Since the finalizers cannot affect the running JS code in any way, they are safe to run at any point.

When the finalizers run from GC, we disable running JS code and any Node-API that may run JS code.
Any unhandled Node-API error or JS exception will cause the process abort.

If a finalizer must run JS code, then it can do it by calling the new node_api_post_finalizer method which schedules the finalizer run in SetImmediate as it was before. The main difference is that previously we always scheduled finalizer runs in SetImmediate implicitly, and now code must do it explicitly.

nodejs-github-bot · 2022-04-08T05:51:09Z

Review requested:

@nodejs/node-api

KevinEady · 2022-04-22T15:46:50Z

Hi @nodejs/node-api ,

Can team members take a look at this PR and provide feedback to @vmoroz so he can understand if this approach works well before he continues going through unit tests and other implementation details? Thanks.

vmoroz · 2022-04-22T23:27:33Z

As we discussed today in Node-API meeting, one of the changes in this PR is the grouping of parameters inside of a new node_api_native_data struct. This struct should allow us to evolve the set of parameters that can be associate with native data. I had also mentioned that we must be able to create a C++ template function that converts C++ lambda to that struct. Such conversion would be difficult to do if the native data and finalizers are passed as function parameters. I am going to add a unit test to demo it.

KevinEady · 2022-05-20T15:27:33Z

We discussed this in the 20 May Node-API meeting. This PR should be rebased and revalidated after #36510 has been merged since both PRs touch finalizer code.

legendecas · 2022-08-09T16:39:24Z

As discussed in the last node-api meeting, it would be worth exploring the possibility of introducing behavior flags to the initialization of an addon to invoke the finalizers in a more eager manner and disallow JavaScript evaluations in the finalizers. In this way, users can still have one single type of finalizer and don't need to distinguish them. We also don't need to introduce a bunch of new apis just for different finalizers.

KevinEady · 2022-10-07T15:23:32Z

We discussed in the 7 Oct Node API meeting that this PR is dependent on the feature-flags implementation inside #42557. Instead of adding the new methods (in PRs first post), we would be able to modify existing API behavior for object finalization.

gabrielschulhof

As currently implemented, AFAICT this change is not dependent on feature flags, since it implements all new APIs. LGTM.

I forgot that we were gonna force finalizers to be eager with a behaviour change and then provide an API to execute the JS portions immediately.

vmoroz · 2023-06-23T02:53:31Z

The latest commit changes the PR based on the latest discussions that we had in Node-API meetings:

In the new experimental version all finalizers are invoked directly from GC without access to JS.
New node_api_post_finalizer method is added to run finalizer code as a part of SetImmediate where it can access GC.

src/js_native_api_v8.cc

vmoroz · 2023-06-23T22:32:12Z

Changed status to DRAFT until I have the full set of tests and changes for the docs.
Otherwise, the rest of the code is in a relative good shape abd very close to the final change.

mhdawson

LGTM

nodejs-github-bot · 2023-09-29T18:32:13Z

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

nodejs-github-bot · 2023-09-30T04:18:54Z

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

PR-URL: #42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

mhdawson · 2023-10-03T14:06:56Z

Landed in b38e312

KevinEady · 2023-10-03T14:41:58Z

Congrats @vmoroz on getting this PR landed! This has been a long time in the making with several users wanting this type of functionality 😄

PR-URL: nodejs#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: #42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs/node#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> 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 Apr 8, 2022

vmoroz mentioned this pull request Apr 8, 2022

node-api: run Node-API finalizers in GC second pass #42515

Closed

mhdawson added the node-api Issues and PRs related to the Node-API. label Apr 8, 2022

vmoroz force-pushed the PR/PureFinalizers branch from babb75d to 71277e0 Compare April 26, 2022 03:26

legendecas mentioned this pull request Aug 29, 2022

node-api: Add API to check safety of calling JS #43805

Open

KevinEady mentioned this pull request Sep 30, 2022

Object wrap has a memory leak bug? nodejs/node-addon-api#1213

Open

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

gabrielschulhof previously approved these changes May 13, 2023

View reviewed changes

vmoroz force-pushed the PR/PureFinalizers branch from 71277e0 to 1391400 Compare June 23, 2023 02:40

vmoroz changed the title ~~node-api: split finalizers into two different types~~ node-api: run finalizers directly from GC Jun 23, 2023

toyobayashi mentioned this pull request Jun 23, 2023

run finalizers directly from GC toyobayashi/emnapi#72

Merged

toyobayashi reviewed Jun 23, 2023

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

mhdawson mentioned this pull request Jun 23, 2023

Stale issues in node-addon-api to discuss nodejs/abi-stable-node#415

Open

vmoroz marked this pull request as draft June 23, 2023 15:07

vmoroz changed the title ~~node-api: run finalizers directly from GC~~ node-api: (DRAFT) run finalizers directly from GC Jun 23, 2023

vmoroz force-pushed the PR/PureFinalizers branch from fee7037 to ff681e7 Compare July 7, 2023 14:51

vmoroz force-pushed the PR/PureFinalizers branch from ff681e7 to 82dd3e6 Compare July 21, 2023 14:23

vmoroz changed the title ~~node-api: (DRAFT) run finalizers directly from GC~~ node-api: run finalizers directly from GC Jul 21, 2023

vmoroz marked this pull request as ready for review July 21, 2023 15:16

vmoroz added 5 commits September 27, 2023 19:07

fix code and test for Linux

2edf5af

fix JS linter error to use assert.strictEqual

971b894

Address PR feedback

5ef58d3

remove Call_while_GC.txt that accidentally added

03d16c7

address PR feedback

25bfcaf

vmoroz force-pushed the PR/PureFinalizers branch from b7f91b9 to 25bfcaf Compare September 28, 2023 02:08

mhdawson approved these changes Sep 29, 2023

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2023

github-actions bot mentioned this pull request Sep 30, 2023

CI Reliability 2023-09-30 nodejs/reliability#679

Open

31 tasks

This was referenced Oct 1, 2023

CI Reliability 2023-10-01 nodejs/reliability#680

Open

CI Reliability 2023-10-02 nodejs/reliability#681

Open

CI Reliability 2023-10-03 nodejs/reliability#682

Open

mhdawson pushed a commit that referenced this pull request Oct 3, 2023

node-api: run finalizers directly from GC

b38e312

PR-URL: #42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

mhdawson closed this Oct 3, 2023

gabrielschulhof mentioned this pull request Oct 6, 2023

node-api: segregate nogc APIs from rest via type system #50060

Merged

RafaelGSS mentioned this pull request Oct 9, 2023

2023-10-17, Version 21.0.0 (Current) #49870

Merged

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023

node-api: run finalizers directly from GC

1bffb4d

PR-URL: nodejs#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

targos pushed a commit that referenced this pull request Nov 11, 2023

node-api: run finalizers directly from GC

db2a07f

PR-URL: #42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

targos pushed a commit that referenced this pull request Nov 27, 2023

node-api: run finalizers directly from GC

5148f03

PR-URL: #42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024

node-api: run finalizers directly from GC

385cfa2

PR-URL: nodejs#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

node-api: run finalizers directly from GC

1bd52c3

PR-URL: nodejs/node#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024

node-api: run finalizers directly from GC

df46654

PR-URL: nodejs/node#42651 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-api: run finalizers directly from GC #42651

node-api: run finalizers directly from GC #42651

vmoroz commented Apr 8, 2022 •

edited

Loading

nodejs-github-bot commented Apr 8, 2022

KevinEady commented Apr 22, 2022

vmoroz commented Apr 22, 2022 •

edited

Loading

KevinEady commented May 20, 2022

legendecas commented Aug 9, 2022 •

edited

Loading

KevinEady commented Oct 7, 2022

gabrielschulhof left a comment •

edited

Loading

vmoroz commented Jun 23, 2023

vmoroz commented Jun 23, 2023

mhdawson left a comment

nodejs-github-bot commented Sep 29, 2023

nodejs-github-bot commented Sep 30, 2023

mhdawson commented Oct 3, 2023

KevinEady commented Oct 3, 2023

node-api: run finalizers directly from GC #42651

node-api: run finalizers directly from GC #42651

Conversation

vmoroz commented Apr 8, 2022 • edited Loading

The issue

The solution

nodejs-github-bot commented Apr 8, 2022

KevinEady commented Apr 22, 2022

vmoroz commented Apr 22, 2022 • edited Loading

KevinEady commented May 20, 2022

legendecas commented Aug 9, 2022 • edited Loading

KevinEady commented Oct 7, 2022

gabrielschulhof left a comment • edited Loading

Choose a reason for hiding this comment

vmoroz commented Jun 23, 2023

vmoroz commented Jun 23, 2023

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 29, 2023

nodejs-github-bot commented Sep 30, 2023

mhdawson commented Oct 3, 2023

KevinEady commented Oct 3, 2023

vmoroz commented Apr 8, 2022 •

edited

Loading

vmoroz commented Apr 22, 2022 •

edited

Loading

legendecas commented Aug 9, 2022 •

edited

Loading

gabrielschulhof left a comment •

edited

Loading