n-api: defer Buffer finalizer with SetImmediate by addaleax · Pull Request #28082 · nodejs/node · GitHub

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 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 (as far as the non-build part is concerned)

@nodejs/n-api

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

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

CI: https://ci.nodejs.org/job/node-test-pull-request/23684/ (:white_check_mark:)

@nodejs/n-api PTAL

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.

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

Landed in fb4d528 3ca0df2

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: #28082 Fixes: #26754 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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: #28082 Fixes: #26754 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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: #28082 Fixes: #26754 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <michael_dawson@ca.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added 2 commits June 5, 2019 23:27

src: fix off-by-one error in native SetImmediate

24a1905

Previously, the throwing callback would have been re-executed in case of an exception. This patch corrects the calculation to exclude the callback.

addaleax added node-api Issues and PRs related to the Node-API. lts-watch-v8.x labels Jun 5, 2019

This comment has been minimized.

Sign in to view

addaleax added the review wanted PRs that need reviews. label Jun 10, 2019

mhdawson approved these changes Jun 11, 2019

View reviewed changes

gabrielschulhof approved these changes Jun 12, 2019

View reviewed changes

jasnell approved these changes Jun 14, 2019

View reviewed changes

addaleax removed the review wanted PRs that need reviews. label Jun 14, 2019

addaleax closed this Jun 14, 2019

addaleax deleted the napi-buffer-finalize branch June 14, 2019 11:03

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

BethGriggs added the backport-requested-v10.x label Jul 9, 2019

n-api: defer Buffer finalizer with SetImmediate by addaleax · Pull Request #28082 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: defer Buffer finalizer with SetImmediate - #28082

n-api: defer Buffer finalizer with SetImmediate#28082
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:napi-buffer-finalize

addaleax commented Jun 5, 2019

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jun 6, 2019 •
edited by addaleax
Loading

Uh oh!

Uh oh!

addaleax commented Jun 9, 2019

Uh oh!

mhdawson left a comment

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

addaleax commented Jun 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

n-api: defer Buffer finalizer with SetImmediate by addaleax · Pull Request #28082 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: defer Buffer finalizer with SetImmediate - #28082

n-api: defer Buffer finalizer with SetImmediate#28082addaleax wants to merge 2 commits intonodejs:masternodejs/node:masterfrom addaleax:napi-buffer-finalizeaddaleax/node:napi-buffer-finalizeCopy head branch name to clipboard

Conversation

addaleax commented Jun 5, 2019

src: fix off-by-one error in native SetImmediate

n-api: defer Buffer finalizer with SetImmediate

Checklist

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jun 6, 2019 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jun 9, 2019

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jun 13, 2019

Uh oh!

addaleax commented Jun 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

n-api: defer Buffer finalizer with SetImmediate#28082
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:napi-buffer-finalize

nodejs-github-bot commented Jun 6, 2019 •
edited by addaleax
Loading