n-api: revert change to finalisation #35777

mhdawson · 2020-10-23T18:14:41Z

This reverts commit a6b6556 which
changed finalization behavior related to N-API. We will investigate
the original issue with the test separately.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Fixes: nodejs#35620 This reverts commit a6b6556 which changed finalization behavior related to N-API. We will investigate the original issue with the test separately.

nodejs-github-bot · 2020-10-23T18:14:45Z

Review requested:

@nodejs/n-api

mhdawson · 2020-10-23T18:19:22Z

Issue to investigate test that is being re-excluded: #35778

codecov-io · 2020-10-23T19:33:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (d233f10) to head (3769505).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #35777      +/-   ##
==========================================
- Coverage   87.92%   87.89%   -0.03%     
==========================================
  Files         477      477              
  Lines      113090   113170      +80     
  Branches    24630    25425     +795     
==========================================
+ Hits        99433    99472      +39     
- Misses       7952     7995      +43     
+ Partials     5705     5703       -2

Files with missing lines	Coverage Δ
src/js_native_api_v8.cc	`62.92% <ø> (-0.03%)`	⬇️

... and 99 files with indirect coverage changes

Trott · 2020-10-24T12:56:53Z

test/node-api/test_worker_terminate_finalization/test.js

@@ -1,6 +1,10 @@
 'use strict';
 const common = require('../../common');

+// TODO(addaleax): Run this test once it stops failing under ASAN/valgrind.
+// Refs: https://github.com/nodejs/node/issues/34731


Optional non-blocking suggestion: Should the comment mention #35778 as well? And maybe #35777 (this pull request) too?

test/node-api/test_worker_terminate_finalization/test.js

nodejs-github-bot · 2020-10-27T07:52:00Z

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

github-actions · 2020-10-27T09:16:07Z

Commit Queue failed

- Loading data for nodejs/node/pull/35777 ✔ Done loading data for nodejs/node/pull/35777 ----------------------------------- PR info ------------------------------------ Title n-api: revert change to finalisation (#35777) Author Michael Dawson (@mhdawson) Branch mhdawson:napi-finalization-revert -> nodejs:master Labels C++, author ready, n-api Commits 2 - n-api: revert change to finalization - Update test/node-api/test_worker_terminate_finalization/test.js Committers 2 - Michael Dawson - GitHub PR-URL: https://github.com/nodejs/node/pull/35777 Fixes: https://github.com/nodejs/node/issues/35620 Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott Reviewed-By: Juan José Arboleda Reviewed-By: Chengzhong Wu Reviewed-By: Gerhard Stöbich ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/35777 Fixes: https://github.com/nodejs/node/issues/35620 Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott Reviewed-By: Juan José Arboleda Reviewed-By: Chengzhong Wu Reviewed-By: Gerhard Stöbich -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-10-27T07:52:00Z: https://ci.nodejs.org/job/node-test-pull-request/33880/ - Querying data for job/node-test-pull-request/33880/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Fri, 23 Oct 2020 18:14:41 GMT ✔ Approvals: 5 ✔ - Gireesh Punathil (@gireeshpunathil) (TSC): https://github.com/nodejs/node/pull/35777#pullrequestreview-516154267 ✔ - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/35777#pullrequestreview-516275689 ✔ - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/35777#pullrequestreview-516417140 ✔ - Chengzhong Wu (@legendecas): https://github.com/nodejs/node/pull/35777#pullrequestreview-516437433 ✔ - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/35777#pullrequestreview-517477376 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 35777 From https://github.com/nodejs/node * branch refs/pull/35777/merge -> FETCH_HEAD ✔ Fetched commits as 04d16646a089..3769505233a5 -------------------------------------------------------------------------------- [master 9a4400f4e2] n-api: revert change to finalization Author: Michael Dawson Date: Fri Oct 23 13:36:54 2020 -0400 2 files changed, 6 insertions(+), 4 deletions(-) [master 1b0290bf89] Update test/node-api/test_worker_terminate_finalization/test.js Author: Michael Dawson Date: Mon Oct 26 15:04:32 2020 -0400 1 file changed, 2 insertions(+) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #35620, skipping..
--------------------------------- New Message ----------------------------------
n-api: revert change to finalization

Fixes: #35620

This reverts commit a6b655614f03e073b9c60f3d71ed884c5af32ffc which
changed finalization behavior related to N-API. We will investigate
the original issue with the test separately.

PR-URL: #35777
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Juan José Arboleda [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Gerhard Stöbich [email protected]

[detached HEAD 861d9f6a01] n-api: revert change to finalization
Author: Michael Dawson [email protected]
Date: Fri Oct 23 13:36:54 2020 -0400
2 files changed, 6 insertions(+), 4 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test/node-api/test_worker_terminate_finalization/test.js

PR-URL: #35777
Fixes: #35620
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Juan José Arboleda [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Gerhard Stöbich [email protected]

[detached HEAD b18eba7eb5] Update test/node-api/test_worker_terminate_finalization/test.js
Author: Michael Dawson [email protected]
Date: Mon Oct 26 15:04:32 2020 -0400
1 file changed, 2 insertions(+)

Successfully rebased and updated refs/heads/master.
✔ 861d9f6a016cf8eec3aea93c6ac896f6f5c297f4
✔ 1:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 7:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ b18eba7eb56e272d82c02148dc87edb81639efde
✔ 2:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

ℹ Please fix the commit message and try again.

Commit Queue action: https://github.com/nodejs/node/actions/runs/330967079

Fixes: #35620 This reverts commit a6b6556 which changed finalization behavior related to N-API. We will investigate the original issue with the test separately. PR-URL: #35777 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

Flarna · 2020-10-27T10:41:31Z

Landed in 0b45382

Currently, a reference is being unlinked from the list of references tracked by the environment when `v8impl::Reference::Delete` is called. This causes a leak when deletion must be deferred because the finalizer hasn't yet run, but the finalizer does not run because environment teardown is in progress, and so no more gc runs will happen, and the `FinalizeAll` run that happens during environment teardown does not catch the reference because it's no longer in the list. The test below will fail when running with ASAN: ``` ./node ./test/node-api/test_worker_terminate_finalization/test.js ``` OTOH if, to address the above leak, we make a special case to not unlink a reference during environment teardown, we run into a situation where the reference gets deleted by `v8impl::Reference::Delete` but does not get unlinked because it's environment teardown time. This leaves a stale pointer in the linked list which will result in a use-after-free in `FinalizeAll` during environment teardown. The test below will fail if we make the above change: ``` ./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');" ``` Thus, we unlink a reference precisely when we destroy it – in its destructor. Refs: nodejs#34731 Refs: nodejs#34839 Refs: nodejs#35620 Refs: nodejs#35777 Fixes: nodejs#35778 Signed-off-by: Gabriel Schulhof <[email protected]>

Fixes: #35620 This reverts commit a6b6556 which changed finalization behavior related to N-API. We will investigate the original issue with the test separately. PR-URL: #35777 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

MylesBorins · 2020-11-03T18:42:24Z

I've landed on 12 and 14. Since this is a revert I don't think we need to let it bake

Currently, a reference is being unlinked from the list of references tracked by the environment when `v8impl::Reference::Delete` is called. This causes a leak when deletion must be deferred because the finalizer hasn't yet run, but the finalizer does not run because environment teardown is in progress, and so no more gc runs will happen, and the `FinalizeAll` run that happens during environment teardown does not catch the reference because it's no longer in the list. The test below will fail when running with ASAN: ``` ./node ./test/node-api/test_worker_terminate_finalization/test.js ``` OTOH if, to address the above leak, we make a special case to not unlink a reference during environment teardown, we run into a situation where the reference gets deleted by `v8impl::Reference::Delete` but does not get unlinked because it's environment teardown time. This leaves a stale pointer in the linked list which will result in a use-after-free in `FinalizeAll` during environment teardown. The test below will fail if we make the above change: ``` ./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');" ``` Thus, we unlink a reference precisely when we destroy it – in its destructor. Refs: #34731 Refs: #34839 Refs: #35620 Refs: #35777 Fixes: #35778 Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #35933 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

Fixes: #35620 This reverts commit a6b6556 which changed finalization behavior related to N-API. We will investigate the original issue with the test separately. PR-URL: #35777 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>

springmeyer · 2020-11-23T16:51:21Z

Thanks @mhdawson for this fix. In the case of my team/org we are waiting on this fix landing in v14 to upgrade from node v10 -> v14. It looks like this ddid not land in v14.15.1 (I see no sign of the commit being applied to https://github.com/nodejs/node/blob/v14.15.1/src/js_native_api_v8.cc#L230). Does anyone know if it can land in another v14.x release soon?

BethGriggs · 2020-11-23T18:03:38Z

@springmeyer, this should make it into the next v14.x release - it's on our v14.x-staging branch ready to go. v14.15.1 was a security release that we follow a slightly different process for. In terms of when, we have a loose/draft schedule over at nodejs/Release#567. The schedule is subject to releaser availability, but you should hopefully expect the commit to be in a release within the next couple of weeks.

springmeyer · 2020-11-23T18:17:40Z

Amazing, thanks for the details @BethGriggs!

Currently, a reference is being unlinked from the list of references tracked by the environment when `v8impl::Reference::Delete` is called. This causes a leak when deletion must be deferred because the finalizer hasn't yet run, but the finalizer does not run because environment teardown is in progress, and so no more gc runs will happen, and the `FinalizeAll` run that happens during environment teardown does not catch the reference because it's no longer in the list. The test below will fail when running with ASAN: ``` ./node ./test/node-api/test_worker_terminate_finalization/test.js ``` OTOH if, to address the above leak, we make a special case to not unlink a reference during environment teardown, we run into a situation where the reference gets deleted by `v8impl::Reference::Delete` but does not get unlinked because it's environment teardown time. This leaves a stale pointer in the linked list which will result in a use-after-free in `FinalizeAll` during environment teardown. The test below will fail if we make the above change: ``` ./node -e "require('./test/node-api/test_instance_data/build/Release/test_ref_then_set.node');" ``` Thus, we unlink a reference precisely when we destroy it – in its destructor. Refs: #34731 Refs: #34839 Refs: #35620 Refs: #35777 Fixes: #35778 Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #35933 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

n-api: revert change to finalization

0f46eaa

Fixes: nodejs#35620 This reverts commit a6b6556 which changed finalization behavior related to N-API. We will investigate the original issue with the test separately.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 23, 2020

This was referenced Oct 23, 2020

Segfault on v12.19.0 #35620

Closed

Investigate test/node-api/test_worker_terminate_finalization/test.js #35778

Closed

gireeshpunathil approved these changes Oct 24, 2020

View reviewed changes

Trott approved these changes Oct 24, 2020

View reviewed changes

Trott reviewed Oct 24, 2020

View reviewed changes

juanarbol approved these changes Oct 26, 2020

View reviewed changes

legendecas approved these changes Oct 26, 2020

View reviewed changes

mhdawson commented Oct 26, 2020

View reviewed changes

test/node-api/test_worker_terminate_finalization/test.js Show resolved Hide resolved

Update test/node-api/test_worker_terminate_finalization/test.js

3769505

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2020

This comment has been minimized.

Sign in to view

Flarna added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. node-api Issues and PRs related to the Node-API. commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 27, 2020

Flarna approved these changes Oct 27, 2020

View reviewed changes

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 27, 2020

Flarna mentioned this pull request Oct 27, 2020

Tracking Issue: commit queue issues and feedback #34770

Open

14 tasks

Flarna closed this Oct 27, 2020

hertzg mentioned this pull request Oct 30, 2020

Investigate new v12.19.0 segfaults in tests. hertzg/node-net-keepalive#93

Closed

gabrielschulhof mentioned this pull request Nov 2, 2020

n-api: unlink reference during its destructor #35933

Closed

4 tasks

Flarna mentioned this pull request Nov 3, 2020

Release plan - v12.x Maintenance nodejs/Release#494

Closed

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

MylesBorins mentioned this pull request Nov 3, 2020

v12.20.0 proposal #35950

Merged

mhdawson mentioned this pull request Nov 6, 2020

Memory leaks when using node-v10 and node-v12 nodejs/node-addon-api#829

Closed

MylesBorins removed the lts-watch-v12.x label Nov 16, 2020

BethGriggs removed the lts-watch-v14.x label Dec 10, 2020

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

addaleax mentioned this pull request Dec 23, 2020

Segfault on process shutdown with vm.createContext node-ffi-napi/weak-napi#31

Open

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: revert change to finalisation #35777

n-api: revert change to finalisation #35777

mhdawson commented Oct 23, 2020

nodejs-github-bot commented Oct 23, 2020

mhdawson commented Oct 23, 2020

codecov-io commented Oct 23, 2020 •

edited by codecov bot

Loading

Trott Oct 24, 2020

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

PR-URL: #35777
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Juan José Arboleda [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Gerhard Stöbich [email protected]

PR-URL: #35777
Fixes: #35620
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Juan José Arboleda [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Gerhard Stöbich [email protected]

Flarna commented Oct 27, 2020

MylesBorins commented Nov 3, 2020

springmeyer commented Nov 23, 2020 •

edited

Loading

BethGriggs commented Nov 23, 2020

springmeyer commented Nov 23, 2020

n-api: revert change to finalisation #35777

n-api: revert change to finalisation #35777

Conversation

mhdawson commented Oct 23, 2020

Checklist

nodejs-github-bot commented Oct 23, 2020

mhdawson commented Oct 23, 2020

codecov-io commented Oct 23, 2020 • edited by codecov bot Loading

Codecov Report

Trott Oct 24, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

PR-URL: #35777 Reviewed-By: Gireesh Punathil [email protected] Reviewed-By: Rich Trott [email protected] Reviewed-By: Juan José Arboleda [email protected] Reviewed-By: Chengzhong Wu [email protected] Reviewed-By: Gerhard Stöbich [email protected]

PR-URL: #35777 Fixes: #35620 Reviewed-By: Gireesh Punathil [email protected] Reviewed-By: Rich Trott [email protected] Reviewed-By: Juan José Arboleda [email protected] Reviewed-By: Chengzhong Wu [email protected] Reviewed-By: Gerhard Stöbich [email protected]

Flarna commented Oct 27, 2020

MylesBorins commented Nov 3, 2020

springmeyer commented Nov 23, 2020 • edited Loading

BethGriggs commented Nov 23, 2020

springmeyer commented Nov 23, 2020

codecov-io commented Oct 23, 2020 •

edited by codecov bot

Loading

PR-URL: #35777
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Juan José Arboleda [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Gerhard Stöbich [email protected]

PR-URL: #35777
Fixes: #35620
Reviewed-By: Gireesh Punathil [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: Juan José Arboleda [email protected]
Reviewed-By: Chengzhong Wu [email protected]
Reviewed-By: Gerhard Stöbich [email protected]

springmeyer commented Nov 23, 2020 •

edited

Loading