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: make reference weak parameter an indirect link to references #38000

Closed
wants to merge 3 commits into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 31, 2021

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)

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

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 Outdated Show resolved Hide resolved
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
Copy link
Member Author

@addaleax thanks for suggestion, updated :)

@addaleax
Copy link
Member

@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
Copy link
Member

@legendecas will try to take a look tomorrow afternoon.

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member

mhdawson commented Apr 1, 2021

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

@mhdawson
Copy link
Member

mhdawson commented Apr 7, 2021

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

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

@mhdawson
Copy link
Member

mhdawson commented Apr 7, 2021

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

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Apr 7, 2021

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

@legendecas
Copy link
Member Author

@mhdawson thanks, looks great.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@legendecas
Copy link
Member Author

@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

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas closed this Apr 19, 2021
@legendecas legendecas deleted the node-api/reference branch April 19, 2021 16:12
legendecas added a commit to legendecas/node that referenced this pull request Apr 29, 2021
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>

# Conflicts:
#	src/js_native_api_v8.cc
legendecas added a commit to legendecas/node that referenced this pull request Apr 29, 2021
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2021
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
mhdawson added a commit to mhdawson/io.js that referenced this pull request Jun 2, 2021
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 <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jun 2, 2021
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 <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
targos pushed a commit that referenced this pull request Jun 8, 2021
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 <[email protected]>

PR-URL: #38899
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
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 <[email protected]>

PR-URL: #38899
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 19, 2021
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 <[email protected]>

PR-URL: #38899
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
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 <[email protected]>

PR-URL: #38899
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
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 <[email protected]>

PR-URL: nodejs#38899
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
legendecas added a commit to legendecas/node that referenced this pull request Mar 29, 2022
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
legendecas pushed a commit to legendecas/node that referenced this pull request Mar 29, 2022
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 <[email protected]>

PR-URL: nodejs#38899
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 30, 2022
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 <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 30, 2022
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 <[email protected]>

PR-URL: #38899
Backport-PR-URL: #42512
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau mentioned this pull request Mar 30, 2022
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++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants