Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node-api: stop ref gc during environment teardown #37616

Conversation

gabrielschulhof
Copy link
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.

@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
// During env teardown, `~napi_env()` alone is responsible for finalizing.
// Thus, we don't want any stray gc passes to trigger a second call to
// `Finalize()`, so let's reset the persistent here.
if (is_env_teardown) _persistent.ClearWeak();
Copy link
Contributor Author

@gabrielschulhof gabrielschulhof Mar 6, 2021

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.

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

@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Mar 6, 2021
@gabrielschulhof
Copy link
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.

@gabrielschulhof gabrielschulhof changed the title Env teardown weak refs main node-api: stop ref gc during environment teardown Mar 7, 2021
@gabrielschulhof
Copy link
Contributor Author

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas linked an issue Mar 15, 2021 that may be closed by this pull request
@nodejs-github-bot
Copy link
Collaborator

gabrielschulhof added a commit that referenced this pull request Mar 19, 2021
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
Copy link
Contributor Author

Landed in 52f9aaf.

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 19, 2021
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]>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 19, 2021
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]>
mhdawson added a commit to mhdawson/io.js that referenced this pull request Mar 23, 2021
Refs: nodejs/node-addon-api#906
Refs: nodejs#37616

Fix crash introduced by nodejs#37616

Signed-off-by: Michael Dawson <[email protected]>
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
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
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
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]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 26, 2021
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]>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 26, 2021
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]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 26, 2021
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
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
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
targos pushed a commit to gabrielschulhof/node that referenced this pull request Apr 24, 2021
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
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
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
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
legendecas pushed a commit to legendecas/node that referenced this pull request Mar 29, 2022
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
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 pushed a commit that referenced this pull request Mar 30, 2022
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
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 mentioned this pull request Mar 30, 2022
@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
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash on node api add-on finalization
6 participants