node-api: run finalizers directly from GC by vmoroz · Pull Request #42651 · nodejs/node · GitHub

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.

Review requested:

@nodejs/node-api

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.

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.

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.

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.

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.

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.

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.

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.

LGTM

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

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>

Landed in b38e312

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: #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>

See nodejs#42651

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

Closed

KevinEady mentioned this pull request Sep 30, 2022

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

Closed

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

Comment thread src/js_native_api_v8.cc Outdated

mhdawson mentioned this pull request Jun 23, 2023

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

Closed

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 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

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>

Brooooooklyn mentioned this pull request Jan 2, 2025

Native addon use ThreadsafeFunction in multithreads env casue EXC_BAD_ACCESS in worker_threads in Node 18 #56432

Closed

vmoroz deleted the PR/PureFinalizers branch April 11, 2025 13:29

Xmader added a commit to Distributive-Network/node that referenced this pull request May 12, 2025

fix the use of upward dependency of node_errors.h

154460e

See nodejs#42651

Xmader added a commit to Distributive-Network/node that referenced this pull request Nov 24, 2025

fix the use of upward dependency of node_errors.h

df59a8a

See nodejs#42651

iurisilvio mentioned this pull request Apr 6, 2026

Bump node-addon-api to fix memory leak #2436 Automattic/node-canvas#2562

Closed

node-api: run finalizers directly from GC by vmoroz · Pull Request #42651 · nodejs/node · GitHub

Uh oh!

Uh oh!

node-api: run finalizers directly from GC - #42651

node-api: run finalizers directly from GC#42651vmoroz wants to merge 10 commits intonodejs:mainnodejs/node:mainfrom vmoroz:PR/PureFinalizersvmoroz/nodejs-node:PR/PureFinalizersCopy head branch name to clipboard

Conversation

vmoroz commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The issue

The solution

Uh oh!

nodejs-github-bot commented Apr 8, 2022

Uh oh!

KevinEady commented Apr 22, 2022

Uh oh!

vmoroz commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinEady commented May 20, 2022

Uh oh!

legendecas commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinEady commented Oct 7, 2022

Uh oh!

gabrielschulhof left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz commented Jun 23, 2023

Uh oh!

Uh oh!

vmoroz commented Jun 23, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 29, 2023

Uh oh!

nodejs-github-bot commented Sep 30, 2023

Uh oh!

mhdawson commented Oct 3, 2023

Uh oh!

KevinEady commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

node-api: run finalizers directly from GC#42651
vmoroz wants to merge 10 commits into
nodejs:mainfrom
vmoroz:PR/PureFinalizers

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