node-api: make reference weak parameter an indirect link to references #38000

legendecas · 2021-03-31T11:20:04Z

As the cancellation of second pass callbacks are not reachable from the current
v8 API, and the second pass callbacks are scheduled with NodePlatform's task
runner, we have to ensure that the weak parameter holds indirect access to the
v8impl::Reference object so that the object can be destroyed on addon env
teardown before the whole node env is able to shutdown.

There aren't any means to request garbage collection with asynchronous second
pass callbacks at node.js teardown (force gc would trigger second pass callbacks
synchronously). And on the main branch, it is not allowed to calling into JavaScript
during environment teardown. So this PR can not be stably tested.

Refs: #37802 (comment)

legendecas · 2021-03-31T11:29:59Z

Also on testing around the teardown process, I found that there are possibilities that v8impl::Reference can call into module's finalizer by napi_env__::CallFinalizer, which would schedule the invocation with an Environment::SetImmediate. However, the napi_env__ would self-destroy after all final v8impl::Reference were destroyed, and then the SetImmediate callback may cause crashes since they are still referencing the old napi_env__.

I believe the issue can be partially (not BufferFinalizer. Although it extends v8impl::Reference, it schedules its own finalizing immediate) addressed by #36510, as it normalized finalizers and removed the unnecessary SetImmediate

src/js_native_api_v8.cc

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown.

legendecas · 2021-03-31T18:06:48Z

@addaleax thanks for suggestion, updated :)

addaleax · 2021-03-31T18:23:34Z

@legendecas Okay, thanks for clarifying :) I’m not sure I understand enough of what’s going on here to review this, but then again, I’m fairly sure that nobody understands this code.

mhdawson · 2021-03-31T21:44:54Z

@legendecas will try to take a look tomorrow afternoon.

nodejs-github-bot · 2021-04-01T16:38:39Z

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

mhdawson · 2021-04-01T20:58:36Z

Sorry did not get to it today, and out until next Tuesday, will try to review then

mhdawson · 2021-04-07T21:30:34Z

@legendecas the change looks good. I pushed a commit that changes the naming of the variable and tweaked the comments but should not affect it functionally. It took me a bit of time to understand what the PR changed/did not change and at least for me the new variable name/comments make it more obvious on that front.

Let me know what you think, we can always remove the commit if it is not an improvement.

mhdawson

LGTM

mhdawson · 2021-04-07T21:33:11Z

See there is a linter failure, not sure why my local run did not report that.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

mhdawson · 2021-04-07T21:56:38Z

Pushed a commit to fixup linter failure and the warning as failure, neither of which were reported in lcoal build.

legendecas · 2021-04-08T05:58:54Z

@mhdawson thanks, looks great.

legendecas · 2021-04-14T08:06:26Z

@gabrielschulhof As it seems the problem doesn't only present on older LTS lines but also on the latest main branch tests, as seen in #38040, it will be great if you can take a look at this one :).

nodejs-github-bot · 2021-04-15T02:37:44Z

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

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: nodejs#38000 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> # Conflicts: # src/js_native_api_v8.cc

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: nodejs#38000 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: #38000 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR nodejs#38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: nodejs#38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com>

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: #38000 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR #38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: #38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: #38000 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR #38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: #38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR nodejs#38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: nodejs#38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: nodejs#38000 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR nodejs#38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: nodejs#38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: nodejs#38899 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

As the cancellation of second pass callbacks are not reachable from the current v8 API, and the second pass callbacks are scheduled with NodePlatform's task runner, we have to ensure that the weak parameter holds indirect access to the v8impl::Reference object so that the object can be destroyed on addon env teardown before the whole node env is able to shutdown. PR-URL: #38000 Backport-PR-URL: #42512 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

PR #38000 added indirection so that we could stop finalization in cases where it had been scheduled in a second pass callback but we were doing it in advance in environment teardown. Unforunately we missed that the code which tries to clear the second pass parameter checked if the pointer to the parameter (_secondPassParameter) was nullptr and that when the second pass callback was scheduled we set _secondPassParameter to nullptr in order to avoid it being deleted outside of the second pass callback. The net result was that we would not clear the _secondPassParameter contents and failed to avoid the Finalization in the second pass callback. This PR adds an additional boolean for deciding if the secondPassParameter should be deleted outside of the second pass callback instead of setting secondPassParameter to nullptr thus avoiding the conflict between the 2 ways it was being used. See the discussion starting at: #38273 (comment) for how this was discovered on OSX while trying to upgrade to a new V8 version. Signed-off-by: Michael Dawson <mdawson@devrus.com> PR-URL: #38899 Backport-PR-URL: #42512 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 31, 2021

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

legendecas force-pushed the node-api/reference branch from 41c280c to 8b4e754 Compare March 31, 2021 11:22

legendecas force-pushed the node-api/reference branch from 8b4e754 to 7b2de34 Compare March 31, 2021 17:01

addaleax reviewed Mar 31, 2021

View reviewed changes

src/js_native_api_v8.cc Outdated Show resolved Hide resolved

legendecas force-pushed the node-api/reference branch from 7b2de34 to 48c95d0 Compare March 31, 2021 18:05

squash: update wording, tweak comments

a85e1f7

mhdawson approved these changes Apr 7, 2021

View reviewed changes

squash: fixup

98eecdf

Signed-off-by: Michael Dawson <mdawson@devrus.com>

This comment has been minimized.

Sign in to view

mhdawson mentioned this pull request Apr 9, 2021

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

Closed

This comment has been minimized.

Sign in to view

legendecas requested a review from gabrielschulhof April 14, 2021 08:04

This comment has been minimized.

Sign in to view

BethGriggs mentioned this pull request Apr 14, 2021

Revert "lib: add WeakRef and FinalizationRegistry to primordials" #38238

Closed

legendecas closed this Apr 19, 2021

legendecas deleted the node-api/reference branch April 19, 2021 16:12

This was referenced Apr 19, 2021

Revert "timers: refactor to use optional chaining" #38245

Closed

2021-04-20, Version 16.0.0 (Current) #37678

Merged

legendecas mentioned this pull request Apr 21, 2021

src: remove unused guards around node-api reference #38334

Closed

mhdawson mentioned this pull request Apr 28, 2021

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

Closed

legendecas mentioned this pull request Apr 29, 2021

[v14.x] node-api: make reference weak parameter an indirect link to references #38469

Closed

legendecas added the backport-open-v14.x label Apr 29, 2021

legendecas mentioned this pull request May 6, 2021

node-api: fix shutdown crashes #38492

Closed

targos removed the backport-open-v14.x label May 30, 2021

mhdawson mentioned this pull request Jun 2, 2021

node-api: avoid SecondPassCallback crash #38899

Closed

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

legendecas mentioned this pull request Mar 27, 2022

Node.js 12.x need to cherry-pick https://github.com/nodejs/node/pull/37616 #42376

Closed

richardlau mentioned this pull request Mar 30, 2022

v12.22.12 proposal #42531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-api: make reference weak parameter an indirect link to references #38000

node-api: make reference weak parameter an indirect link to references #38000

legendecas commented Mar 31, 2021 •

edited

legendecas commented Mar 31, 2021

legendecas commented Mar 31, 2021

addaleax commented Mar 31, 2021

mhdawson commented Mar 31, 2021

nodejs-github-bot commented Apr 1, 2021

mhdawson commented Apr 1, 2021

mhdawson commented Apr 7, 2021

mhdawson left a comment

mhdawson commented Apr 7, 2021

mhdawson commented Apr 7, 2021

legendecas commented Apr 8, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

legendecas commented Apr 14, 2021

This comment has been minimized.

nodejs-github-bot commented Apr 15, 2021

node-api: make reference weak parameter an indirect link to references #38000

node-api: make reference weak parameter an indirect link to references #38000

Conversation

legendecas commented Mar 31, 2021 • edited

legendecas commented Mar 31, 2021

legendecas commented Mar 31, 2021

addaleax commented Mar 31, 2021

mhdawson commented Mar 31, 2021

nodejs-github-bot commented Apr 1, 2021

mhdawson commented Apr 1, 2021

mhdawson commented Apr 7, 2021

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Apr 7, 2021

mhdawson commented Apr 7, 2021

legendecas commented Apr 8, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

legendecas commented Apr 14, 2021

This comment has been minimized.

nodejs-github-bot commented Apr 15, 2021

legendecas commented Mar 31, 2021 •

edited