GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — node-api: stop ref gc during environment teardown by gabrielschulhof · Pull Request #37616 · nodejs/node

Conversation

@gabrielschulhof

@gabrielschulhof gabrielschulhof commented Mar 6, 2021

Copy link
Copy Markdown
Contributor

A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabrielschulhof
add test
26fed3f
@gabrielschulhof
gabrielschulhof requested review from legendecas and mhdawson March 6, 2021 05:41
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 6, 2021
gabrielschulhof
gabrielschulhof commented Mar 6, 2021
View reviewed changes
Comment thread src/js_native_api_v8.cc Outdated

@gabrielschulhof gabrielschulhof Mar 6, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor 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.

We discussed this solution with @legendecas during today's Node-API meeting and we realized that using ClearWeak() to ensure that the engine does not cause Finalize() to re-enter by triggering a gc pass during the user's finalizer assumes that ClearWeak() causes queued finalizers to be removed from the queue. If this is not the case, it would be better to avoid this re-entrancy by using a flag that would cause Finalize() to short-circuit if it was already started as part of env teardown:

inline void Finalize(bool is_env_teardown = false) override {
  if (is_env_teardown) _env_teardown_finalize_started = true;
  if (!is_env_teardown && _env_teardown_finalize_started) return;
...

where _env_teardown_finalize_started is a member variable that is initially set to false.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabrielschulhof
node-api: stop ref gc during environment teardown
8eb03c4 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: nodejs#37236
PR-URL: nodejs#37616
@gabrielschulhof
gabrielschulhof force-pushed the env-teardown-weak-refs-main branch from 76b696f to 8eb03c4 Compare March 6, 2021 05:49
@gabrielschulhof

gabrielschulhof commented Mar 6, 2021

Copy link
Copy Markdown
Contributor Author

This fix is meant mostly for v14.x and earlier because on master we cannot call into JS during env teardown, so this is not an issue.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to Node-API. label Mar 6, 2021
@gabrielschulhof

gabrielschulhof commented Mar 7, 2021

Copy link
Copy Markdown
Contributor Author

The reason this PR is against master is that making these changes is IMO the prudent thing to do, even if they do not affect master.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof changed the title Env teardown weak refs main node-api: stop ref gc during environment teardown Mar 7, 2021
@gabrielschulhof
add requested comment
4c2131c
@gabrielschulhof

gabrielschulhof commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

@mhdawson I added the comment you requested during today's meeting.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson approved these changes Mar 12, 2021
View reviewed changes

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36667/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas approved these changes Mar 14, 2021
View reviewed changes
@legendecas legendecas linked an issue Mar 15, 2021 that may be closed by this pull request
Crash on node api add-on finalization #37236
Closed
@nodejs-github-bot

nodejs-github-bot commented Mar 18, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36779/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gabrielschulhof added a commit that referenced this pull request Mar 19, 2021
@gabrielschulhof
node-api: stop ref gc during environment teardown
52f9aaf 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: #37236
PR-URL: #37616
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof

gabrielschulhof commented Mar 19, 2021

Copy link
Copy Markdown
Contributor Author

Landed in 52f9aaf.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof closed this Mar 19, 2021
@mhdawson mhdawson mentioned this pull request Mar 22, 2021
Intermittent failure in ThreadSafe function nodejs/node-addon-api#906
Closed
@mhdawson mhdawson mentioned this pull request Mar 23, 2021
node-api: fix crash in finalization #37876
Merged
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
@gabrielschulhof @ruyadorno
node-api: stop ref gc during environment teardown
d1a3e0e 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: #37236
PR-URL: #37616
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Mar 24, 2021
@mhdawson @Trott
node-api: fix crash in finalization
eb9e0fc 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>
Trott pushed a commit to mhdawson/io.js that referenced this pull request Mar 24, 2021
@mhdawson @Trott
node-api: fix crash in finalization
42dc4d1 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Mar 29, 2021
@mhdawson @ruyadorno
node-api: fix crash in finalization
906eb53 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Mar 30, 2021
@mhdawson @ruyadorno
node-api: fix crash in finalization
3ab223d 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
v15.13.0 proposal #37977
Merged
@gabrielschulhof gabrielschulhof mentioned this pull request Apr 2, 2021
local fatal error for test/js-native-api/test_object/test.js #38040
Closed
targos pushed a commit to gabrielschulhof/node that referenced this pull request Apr 24, 2021
@gabrielschulhof @targos
node-api: stop ref gc during environment teardown
466d768 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: nodejs#37236
PR-URL: nodejs#37616
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit to gabrielschulhof/node that referenced this pull request Apr 24, 2021
@mhdawson @targos
node-api: fix crash in finalization
180c33d 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos added backport-open-v14.x and removed backport-open-v14.x needs-ci PRs that need a full CI run. labels Apr 25, 2021
targos pushed a commit that referenced this pull request Apr 26, 2021
@gabrielschulhof @targos
node-api: stop ref gc during environment teardown
6e01855 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: #37236
PR-URL: #37616
Backport-PR-URL: #37802
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Apr 26, 2021
@mhdawson @targos
node-api: fix crash in finalization
8413759 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Backport-PR-URL: #37802
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@richardlau richardlau added the lts-watch-v12.x label Mar 17, 2022
@XhmikosR XhmikosR mentioned this pull request Mar 20, 2022
Node.js 12.x need to cherry-pick https://github.com/nodejs/node/pull/37616 #42376
Closed
legendecas pushed a commit to legendecas/node that referenced this pull request Mar 29, 2022
@gabrielschulhof @legendecas
node-api: stop ref gc during environment teardown
7b6b7d3 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: nodejs#37236
PR-URL: nodejs#37616
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
legendecas pushed a commit to legendecas/node that referenced this pull request Mar 29, 2022
@mhdawson @legendecas
node-api: fix crash in finalization
affb8b2 
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: nodejs#37876
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau added backport-open-v12.x and removed lts-watch-v12.x labels Mar 30, 2022
richardlau pushed a commit that referenced this pull request Mar 30, 2022
@gabrielschulhof @richardlau
node-api: stop ref gc during environment teardown
a2f4206 
A gc may happen during environment teardown. Thus, during finalization
initiated by environment teardown we must remove the V8 finalizer
before calling the Node-API finalizer.

Fixes: #37236
PR-URL: #37616
Backport-PR-URL: #42512
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 30, 2022
@mhdawson @richardlau
node-api: fix crash in finalization
2aa9ca1 
Refs: nodejs/node-addon-api#906
Refs: #37616

Fix crash introduced by #37616

Signed-off-by: Michael Dawson <[email protected]>

PR-URL: #37876
Backport-PR-URL: #42512
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau added backported-to-v12.x and removed backport-open-v12.x labels Mar 30, 2022
@richardlau richardlau mentioned this pull request Mar 30, 2022
v12.22.12 proposal #42531
Merged
@gabrielschulhof
gabrielschulhof deleted the env-teardown-weak-refs-main branch March 17, 2023 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecas legendecas legendecas approved these changes
+1 more reviewer
@mhdawson 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++. 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.

Crash on node api add-on finalization

Uh oh!

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

6 participants

@gabrielschulhof @nodejs-github-bot @legendecas @mhdawson @targos @richardlau
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.