node-api: add nested object wrap and napi_ref test #57981

legendecas · 2025-04-22T22:34:32Z

Test that an napi_ref can be nested inside another ObjectWrap. The test
shows a critical case where a finalizer deletes an napi_ref whose
finalizer is also scheduled.

This add a test for comments at:

node/src/node_api.cc

Lines 116 to 118 in 68cc1c9

    
           // As userland code can delete additional references in one finalizer, 
        
           // the list of pending finalizers may be mutated as we execute them, so 
        
           // we keep iterating it until it is empty.

The changes in test/js-native-api/6_object_wrap/myobject.cc are auto-formatted by make format-cpp as the file is renamed.

Refs: #57861 (review)

nodejs-github-bot · 2025-04-22T22:34:37Z

Review requested:

@nodejs/gyp
@nodejs/node-api

Test that an napi_ref can be nested inside another ObjectWrap. The test shows a critical case where a finalizer deletes an `napi_ref` whose finalizer is also scheduled.

codecov · 2025-04-22T23:38:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (e9b286c) to head (21ac07e).
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57981      +/-   ##
==========================================
- Coverage   90.27%   90.27%   -0.01%     
==========================================
  Files         630      630              
  Lines      186165   186158       -7     
  Branches    36485    36475      -10     
==========================================
- Hits       168064   168049      -15     
- Misses      10974    10979       +5     
- Partials     7127     7130       +3

see 35 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

test/js-native-api/6_object_wrap/myobject.h

test/js-native-api/6_object_wrap/nested_wrap.cc

vmoroz · 2025-04-23T00:30:24Z

@legendecas , in regards to the PR #57861, do we ever have a situation when a napi_ref is removed from the finalizer queue, or added to it twice? I guess, in such cases the batched processing of the queue may not work.

mhdawson

LGTM

nodejs-github-bot · 2025-04-23T14:05:36Z

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

legendecas · 2025-04-24T23:33:57Z

@vmoroz in regards to the PR #57861, do we ever have a situation when a napi_ref is removed from the finalizer queue, or added to it twice? I guess, in such cases the batched processing of the queue may not work.

I think node_api_post_finalizer is the only node-api that allows putting a finalizer into the back of the queue, and it is how it supposed to work.

Putting a single finalizer into the queue twice is a bug. Do you have a minimum test to reproduce it?

nodejs-github-bot · 2025-04-25T12:54:17Z

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

nodejs-github-bot · 2025-04-25T15:10:01Z

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

mhdawson

LGTM

vmoroz · 2025-04-25T22:11:12Z

@vmoroz in regards to the PR #57861, do we ever have a situation when a napi_ref is removed from the finalizer queue, or added to it twice? I guess, in such cases the batched processing of the queue may not work.

I think node_api_post_finalizer is the only node-api that allows putting a finalizer into the back of the queue, and it is how it supposed to work.

Putting a single finalizer into the queue twice is a bug. Do you have a minimum test to reproduce it?

@legendecas , no, I do not have a repro. I am just trying to understand if the PR #57861 has issues or not. Currently based on your answers, I tend to believe that it does not have issues. I wonder if you already saw any issues by running that PR against these new tests.

legendecas · 2025-04-25T22:12:45Z

@vmoroz I commented at #57861 (comment). The change in that PR crashes this new test.

nodejs-github-bot · 2025-04-25T22:22:31Z

Landed in 3b90f34

Test that an napi_ref can be nested inside another ObjectWrap. The test shows a critical case where a finalizer deletes an `napi_ref` whose finalizer is also scheduled. PR-URL: #57981 Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Apr 22, 2025

github-project-automation bot added this to Node-API Team Project Apr 22, 2025

github-project-automation bot moved this to Need Triage in Node-API Team Project Apr 22, 2025

legendecas force-pushed the node-api/nested-wrap branch 2 times, most recently from 8fdaaaa to a5aa845 Compare April 22, 2025 22:47

node-api: add nested object wrap and napi_ref test

161b8e3

Test that an napi_ref can be nested inside another ObjectWrap. The test shows a critical case where a finalizer deletes an `napi_ref` whose finalizer is also scheduled.

legendecas force-pushed the node-api/nested-wrap branch from a5aa845 to 161b8e3 Compare April 22, 2025 22:48

vmoroz reviewed Apr 23, 2025

View reviewed changes

test/js-native-api/6_object_wrap/myobject.h Show resolved Hide resolved

vmoroz reviewed Apr 23, 2025

View reviewed changes

test/js-native-api/6_object_wrap/nested_wrap.cc Outdated Show resolved Hide resolved

vmoroz approved these changes Apr 23, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

legendecas mentioned this pull request Apr 23, 2025

node-api: use container swap in DrainFinalizerQueue to reduce erase overhead #57861

Closed

mhdawson approved these changes Apr 23, 2025

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2025

fixup! node-api: add nested object wrap and napi_ref test

21ac07e

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2025

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 24, 2025

This comment was marked as outdated.

Sign in to view

legendecas removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 25, 2025

mhdawson approved these changes Apr 25, 2025

View reviewed changes

mhdawson moved this from Need Triage to In Progress in Node-API Team Project Apr 25, 2025

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2025

nodejs-github-bot merged commit 3b90f34 into nodejs:main Apr 25, 2025
60 checks passed

github-project-automation bot moved this from In Progress to Done in Node-API Team Project Apr 25, 2025

legendecas deleted the node-api/nested-wrap branch April 25, 2025 22:33

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

toyobayashi mentioned this pull request May 12, 2025

test: add nested object wrap and napi_ref test toyobayashi/emnapi#145

Merged

github-actions bot mentioned this pull request May 19, 2025

2025-05-21, Version 22.16.0 'Jod' (LTS) #58388

Merged

ghost mentioned this pull request Jun 8, 2025

### Notable Changes autofix-ci/action#28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

node-api: add nested object wrap and napi_ref test #57981

node-api: add nested object wrap and napi_ref test #57981

Uh oh!

legendecas commented Apr 22, 2025 •

edited

Loading

Uh oh!

nodejs-github-bot commented Apr 22, 2025 •

edited by legendecas

Loading

Uh oh!

codecov bot commented Apr 22, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

vmoroz commented Apr 23, 2025

Uh oh!

This comment was marked as duplicate.

mhdawson left a comment

Uh oh!

nodejs-github-bot commented Apr 23, 2025

Uh oh!

legendecas commented Apr 24, 2025

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 25, 2025

Uh oh!

nodejs-github-bot commented Apr 25, 2025

Uh oh!

mhdawson left a comment

Uh oh!

vmoroz commented Apr 25, 2025

Uh oh!

legendecas commented Apr 25, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 25, 2025

Uh oh!

Uh oh!

	// As userland code can delete additional references in one finalizer,
	// the list of pending finalizers may be mutated as we execute them, so
	// we keep iterating it until it is empty.

Uh oh!

node-api: add nested object wrap and napi_ref test #57981

node-api: add nested object wrap and napi_ref test #57981

Uh oh!

Conversation

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

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 22, 2025 • edited by legendecas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

vmoroz commented Apr 23, 2025

Uh oh!

This comment was marked as duplicate.

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 23, 2025

Uh oh!

legendecas commented Apr 24, 2025

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Apr 25, 2025

Uh oh!

nodejs-github-bot commented Apr 25, 2025

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

vmoroz commented Apr 25, 2025

Uh oh!

legendecas commented Apr 25, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 25, 2025

Uh oh!

Uh oh!

legendecas commented Apr 22, 2025 •

edited

Loading

nodejs-github-bot commented Apr 22, 2025 •

edited by legendecas

Loading

codecov bot commented Apr 22, 2025 •

edited

Loading