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

gabrielschulhof · 2021-03-06T05:41:58Z

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.

gabrielschulhof · 2021-03-06T05:47:00Z

src/js_native_api_v8.cc

+    // 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();


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 · 2021-03-06T05:51:03Z

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 · 2021-03-07T04:03:03Z

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 · 2021-03-12T18:02:28Z

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

mhdawson

LGTM

nodejs-github-bot · 2021-03-12T23:56:46Z

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

nodejs-github-bot · 2021-03-18T14:51:19Z

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

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

gabrielschulhof · 2021-03-19T03:43:02Z

Landed in 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: nodejs#37236 PR-URL: nodejs#37616 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com>

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com>

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#37876 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#37876 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: nodejs/node-addon-api#906 Refs: #37616 Fix crash introduced by #37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #37876 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#37876 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: #37616 Fix crash introduced by #37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #37876 Backport-PR-URL: #37802 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#37876 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Refs: nodejs/node-addon-api#906 Refs: #37616 Fix crash introduced by #37616 Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #37876 Backport-PR-URL: #42512 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

add test

26fed3f

gabrielschulhof requested review from legendecas and mhdawson March 6, 2021 05:41

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 commented Mar 6, 2021

View reviewed changes

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 force-pushed the env-teardown-weak-refs-main branch from 76b696f to 8eb03c4 Compare March 6, 2021 05:49

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Mar 6, 2021

gabrielschulhof changed the title ~~Env teardown weak refs main~~ node-api: stop ref gc during environment teardown Mar 7, 2021

add requested comment

4c2131c

mhdawson approved these changes Mar 12, 2021

View reviewed changes

legendecas approved these changes Mar 14, 2021

View reviewed changes

legendecas linked an issue Mar 15, 2021 that may be closed by this pull request

Crash on node api add-on finalization #37236

Closed

gabrielschulhof closed this Mar 19, 2021

mhdawson mentioned this pull request Mar 22, 2021

Intermittent failure in ThreadSafe function nodejs/node-addon-api#906

Closed

mhdawson added a commit to mhdawson/io.js that referenced this pull request Mar 23, 2021

node-api: fix crash in finalization

c897654

Refs: nodejs/node-addon-api#906 Refs: nodejs#37616 Fix crash introduced by nodejs#37616 Signed-off-by: Michael Dawson <mdawson@devrus.com>

mhdawson mentioned this pull request Mar 23, 2021

node-api: fix crash in finalization #37876

Merged

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 24, 2021

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 <mdawson@devrus.com>

ruyadorno mentioned this pull request Mar 30, 2021

v15.13.0 proposal #37977

Merged

gabrielschulhof mentioned this pull request Apr 2, 2021

local fatal error for test/js-native-api/test_object/test.js #38040

Closed

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

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

richardlau added the lts-watch-v12.x label Mar 17, 2022

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

richardlau added backport-open-v12.x and removed lts-watch-v12.x labels Mar 30, 2022

richardlau added backported-to-v12.x and removed backport-open-v12.x labels Mar 30, 2022

richardlau mentioned this pull request Mar 30, 2022

v12.22.12 proposal #42531

Merged

gabrielschulhof deleted the env-teardown-weak-refs-main branch March 17, 2023 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabrielschulhof commented Mar 6, 2021

gabrielschulhof Mar 6, 2021 •

edited

gabrielschulhof commented Mar 6, 2021

gabrielschulhof commented Mar 7, 2021

gabrielschulhof commented Mar 12, 2021

mhdawson left a comment

nodejs-github-bot commented Mar 12, 2021

nodejs-github-bot commented Mar 18, 2021

gabrielschulhof commented Mar 19, 2021

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

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

Conversation

gabrielschulhof commented Mar 6, 2021

gabrielschulhof Mar 6, 2021 • edited

Choose a reason for hiding this comment

gabrielschulhof commented Mar 6, 2021

gabrielschulhof commented Mar 7, 2021

gabrielschulhof commented Mar 12, 2021

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 12, 2021

nodejs-github-bot commented Mar 18, 2021

gabrielschulhof commented Mar 19, 2021

gabrielschulhof Mar 6, 2021 •

edited