n-api: defer Buffer finalizer with SetImmediate#28082
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Previously, the throwing callback would have been re-executed in case of an exception. This patch corrects the calculation to exclude the callback.
We have a test that verifies that JS execution from the Buffer finalizer is accepted, and that errors thrown are passed down synchronously. However, since the finalizer executes during GC, this is behaviour is fundamentally invalid and, for good reasons, disallowed by the JS engine. This leaves us with the options of either finding a way to allow JS execution from the callback, or explicitly forbidding it on the N-API side as well. This commit implements the former option, since it is the more backwards-compatible one, in the sense that the current situation sometimes appears to work as well and we should not break that behaviour if we don’t have to, but rather try to actually make it work reliably. Since GC timing is largely unobservable anyway, this commit moves the callback into a `SetImmediate()`, as we do elsewhere in the code, and a second pass callback is not an easily implemented option, as the API is supposed to wrap around Node’s `Buffer` API. In this case, exceptions are handled like other uncaught exceptions. Two tests have to be adjusted to account for the timing difference. This is unfortunate, but unavoidable if we want to conform to the JS engine API contract and keep all tests. Fixes: nodejs#26754
This comment has been minimized.
This comment has been minimized.
Collaborator
CI: https://ci.nodejs.org/job/node-test-pull-request/23684/ (:white_check_mark:) |
MemberAuthor
addaleax commented Jun 9, 2019
@nodejs/n-api PTAL |
mhdawson approved these changes Jun 11, 2019
Member
mhdawson left a comment
mhdawson left a comment There was a problem hiding this comment.
LGTM this looks like a good solution as I agree that code should not be depending on the timing of when a finalizer is run.
gabrielschulhof approved these changes Jun 12, 2019
Member
mhdawson commented Jun 13, 2019
jasnell approved these changes Jun 14, 2019
MemberAuthor
addaleax commented Jun 14, 2019
addaleax added a commit that referenced this pull request Jun 14, 2019
Previously, the throwing callback would have been re-executed in case of an exception. This patch corrects the calculation to exclude the callback. PR-URL: #28082Fixes: #26754 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jun 14, 2019
We have a test that verifies that JS execution from the Buffer finalizer is accepted, and that errors thrown are passed down synchronously. However, since the finalizer executes during GC, this is behaviour is fundamentally invalid and, for good reasons, disallowed by the JS engine. This leaves us with the options of either finding a way to allow JS execution from the callback, or explicitly forbidding it on the N-API side as well. This commit implements the former option, since it is the more backwards-compatible one, in the sense that the current situation sometimes appears to work as well and we should not break that behaviour if we don’t have to, but rather try to actually make it work reliably. Since GC timing is largely unobservable anyway, this commit moves the callback into a `SetImmediate()`, as we do elsewhere in the code, and a second pass callback is not an easily implemented option, as the API is supposed to wrap around Node’s `Buffer` API. In this case, exceptions are handled like other uncaught exceptions. Two tests have to be adjusted to account for the timing difference. This is unfortunate, but unavoidable if we want to conform to the JS engine API contract and keep all tests. Fixes: #26754 PR-URL: #28082 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Previously, the throwing callback would have been re-executed in case of an exception. This patch corrects the calculation to exclude the callback. PR-URL: #28082Fixes: #26754 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
We have a test that verifies that JS execution from the Buffer finalizer is accepted, and that errors thrown are passed down synchronously. However, since the finalizer executes during GC, this is behaviour is fundamentally invalid and, for good reasons, disallowed by the JS engine. This leaves us with the options of either finding a way to allow JS execution from the callback, or explicitly forbidding it on the N-API side as well. This commit implements the former option, since it is the more backwards-compatible one, in the sense that the current situation sometimes appears to work as well and we should not break that behaviour if we don’t have to, but rather try to actually make it work reliably. Since GC timing is largely unobservable anyway, this commit moves the callback into a `SetImmediate()`, as we do elsewhere in the code, and a second pass callback is not an easily implemented option, as the API is supposed to wrap around Node’s `Buffer` API. In this case, exceptions are handled like other uncaught exceptions. Two tests have to be adjusted to account for the timing difference. This is unfortunate, but unavoidable if we want to conform to the JS engine API contract and keep all tests. Fixes: #26754 PR-URL: #28082 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
targos pushed a commit that referenced this pull request Jun 18, 2019
Previously, the throwing callback would have been re-executed in case of an exception. This patch corrects the calculation to exclude the callback. PR-URL: #28082Fixes: #26754 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jun 18, 2019
We have a test that verifies that JS execution from the Buffer finalizer is accepted, and that errors thrown are passed down synchronously. However, since the finalizer executes during GC, this is behaviour is fundamentally invalid and, for good reasons, disallowed by the JS engine. This leaves us with the options of either finding a way to allow JS execution from the callback, or explicitly forbidding it on the N-API side as well. This commit implements the former option, since it is the more backwards-compatible one, in the sense that the current situation sometimes appears to work as well and we should not break that behaviour if we don’t have to, but rather try to actually make it work reliably. Since GC timing is largely unobservable anyway, this commit moves the callback into a `SetImmediate()`, as we do elsewhere in the code, and a second pass callback is not an easily implemented option, as the API is supposed to wrap around Node’s `Buffer` API. In this case, exceptions are handled like other uncaught exceptions. Two tests have to be adjusted to account for the timing difference. This is unfortunate, but unavoidable if we want to conform to the JS engine API contract and keep all tests. Fixes: #26754 PR-URL: #28082 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
src: fix off-by-one error in native SetImmediate
Previously, the throwing callback would have been re-executed in case
of an exception. This patch corrects the calculation to exclude the
callback.
n-api: defer Buffer finalizer with SetImmediate
We have a test that verifies that JS execution from the Buffer
finalizer is accepted, and that errors thrown are passed
down synchronously.
However, since the finalizer executes during GC, this is behaviour is
fundamentally invalid and, for good reasons, disallowed by the
JS engine. This leaves us with the options of either finding a way
to allow JS execution from the callback, or explicitly forbidding it on
the N-API side as well.
This commit implements the former option, since it is the more
backwards-compatible one, in the sense that the current situation
sometimes appears to work as well and we should not break that
behaviour if we don’t have to, but rather try to actually make it
work reliably.
Since GC timing is largely unobservable anyway, this commit moves
the callback into a
SetImmediate(), as we do elsewhere in the code,and a second pass callback is not an easily implemented option,
as the API is supposed to wrap around Node’s
BufferAPI.In this case, exceptions are handled like other uncaught exceptions.
Two tests have to be adjusted to account for the timing difference.
This is unfortunate, but unavoidable if we want to conform to the
JS engine API contract and keep all tests.
Fixes: #26754 (as far as the non-build part is concerned)
@nodejs/n-api
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes