GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — node-api: generalize finalizer second pass callback by legendecas · Pull Request #44141 · nodejs/node

Conversation

legendecas commented Aug 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

nodejs-github-bot commented Aug 5, 2022

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/gyp
@nodejs/node-api

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

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 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. Only starts once the PR has an approving review. label

Aug 5, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 5, 2022

This comment was marked as outdated.

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. Only starts once the PR has an approving review. label

Aug 7, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 7, 2022

nodejs-github-bot commented Aug 7, 2022 •

edited by legendecas

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

test.parallel/test-diagnostics-channel-net

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 8, 2022

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

Open

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

Open

nodejs-github-bot commented Aug 9, 2022 •

edited by legendecas

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 9, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 10, 2022

CI Reliability 2022-08-10 nodejs/reliability#342

Open

CI Reliability 2022-08-11 nodejs/reliability#343

Open

CI Reliability 2022-08-12 nodejs/reliability#344

Open

CI Reliability 2022-08-13 nodejs/reliability#345

Open

CI Reliability 2022-08-14 nodejs/reliability#346

Open

CI Reliability 2022-08-15 nodejs/reliability#347

Open

CI Reliability 2022-08-16 nodejs/reliability#348

Open

vmoroz reviewed

Aug 18, 2022

View reviewed changes

Comment thread

src/node_api.cc Outdated

vmoroz Aug 18, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

legendecas Aug 19, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vmoroz reviewed

Aug 18, 2022

View reviewed changes

Comment thread

src/node_api.cc Outdated

vmoroz Aug 18, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

legendecas Aug 19, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vmoroz reviewed

Aug 18, 2022

View reviewed changes

Comment thread

src/node_api.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

legendecas force-pushed the node-api/second-pass branch from 9416d37 to c6b1b47 Compare

October 14, 2022 07:13

legendecas commented Oct 14, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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


          node-api: generalize finalizer second pass callback

57a58d1

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.


          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. Only starts once the PR has an approving review. label

Nov 18, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Nov 18, 2022

nodejs-github-bot commented Nov 18, 2022

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas commented Nov 21, 2022

Copy link

Copy Markdown

Member Author

@mhdawson updated, PTAL again, thank you :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson reviewed

Nov 22, 2022

View reviewed changes

Comment thread

src/node_api.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

mhdawson approved these changes

Nov 22, 2022

View reviewed changes

mhdawson left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

legendecas added a commit that referenced this pull request

Dec 19, 2022


          node-api: generalize finalizer second pass callback

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 <[email protected]>

legendecas commented Dec 19, 2022

Copy link

Copy Markdown

Member Author

Landed in f14fa1b

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

targos pushed a commit that referenced this pull request

Jan 1, 2023


          node-api: generalize finalizer second pass callback

71025cf

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 <[email protected]>

RafaelGSS mentioned this pull request

Jan 2, 2023

v19.4.0 proposal #46061

Merged

guangwong pushed a commit to noslate-project/node that referenced this pull request

Jan 3, 2023


          node-api: cleanup redundant static modifiers

4a555e6

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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

guangwong pushed a commit to noslate-project/node that referenced this pull request

Jan 3, 2023


          node-api: cleanup redundant static modifiers

9ceeb24

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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Jan 4, 2023


          node-api: generalize finalizer second pass callback

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 <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Jan 5, 2023


          node-api: generalize finalizer second pass callback

f3bb6a3

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 <[email protected]>

juanarbol pushed a commit that referenced this pull request

Jan 26, 2023


          node-api: generalize finalizer second pass callback

6e9676e

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 <[email protected]>

juanarbol mentioned this pull request

Jan 28, 2023

V18.14.0 proposal #46396

Merged

Xmader added a commit to Distributive-Network/node that referenced this pull request

May 14, 2025


          Reference::Finalize/RefBase::Finalize was generalized

134caf5

See nodejs#44141

Xmader added a commit to Distributive-Network/node that referenced this pull request

Nov 24, 2025


          Reference::Finalize/RefBase::Finalize was generalized

823bbf7

See nodejs#44141

Xmader added a commit to Distributive-Network/node that referenced this pull request

Oct 23, 2026


          Reference::Finalize/RefBase::Finalize was generalized

294fff7

See nodejs#44141

Reviewers

vmoroz

vmoroz left review comments

+1 more reviewer

mhdawson

mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

needs-ci

PRs that need a full CI run.

node-api

Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

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

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 5, 2022

Uh oh!

This comment was marked as outdated.

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 9, 2022

Uh oh!

vmoroz Aug 18, 2022

Choose a reason for hiding this comment

Uh oh!

legendecas Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

vmoroz Aug 18, 2022

Choose a reason for hiding this comment

Uh oh!

legendecas Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 18, 2022

Uh oh!

legendecas commented Nov 21, 2022

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

legendecas commented Dec 19, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

legendecas commented Aug 5, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 7, 2022 •

edited by legendecas

Loading

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 9, 2022 •

edited by legendecas

Loading

Uh oh!

There was an error while loading. Please reload this page.

legendecas commented Oct 14, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.