Skip to content

n-api: ensure in-module exceptions are propagated #19537

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

n-api: ensure in-module exceptions are propagated #19537

wants to merge 1 commit into from

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Mar 22, 2018

Fixes: #19437

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Mar 22, 2018
@gabrielschulhof gabrielschulhof changed the title n-api: ensure exceptions are propagated from init [WIP] n-api: ensure exceptions are propagated from init Mar 22, 2018
@gabrielschulhof gabrielschulhof force-pushed the n-api-test-init-exception branch from cffd0ab to 6500f8e Compare March 23, 2018 04:07
@gabrielschulhof gabrielschulhof changed the title [WIP] n-api: ensure exceptions are propagated from init n-api: ensure exceptions are propagated from init Mar 23, 2018
@gabrielschulhof gabrielschulhof changed the title n-api: ensure exceptions are propagated from init n-api: ensure exceptions are propagated from addons Mar 23, 2018
@gabrielschulhof gabrielschulhof changed the title n-api: ensure exceptions are propagated from addons n-api: ensure in-module exceptions are propagated Mar 23, 2018
@gabrielschulhof gabrielschulhof force-pushed the n-api-test-init-exception branch from 6500f8e to 0e5f648 Compare March 23, 2018 13:14
n-api: ensure in-module exceptions are propagated
5c15524 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: nodejs#19437
@gabrielschulhof gabrielschulhof force-pushed the n-api-test-init-exception branch from 0e5f648 to 5c15524 Compare March 23, 2018 13:24
mhdawson
mhdawson reviewed Mar 24, 2018
View reviewed changes
test/addons-napi/test_exception/test.js
function testFinalize(binding) {
let x = test_exception[binding]();
x = null;
assert.throws(() => { global.gc(); }, /Error during Finalize/);
Copy link
Member

@mhdawson mhdawson Mar 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be worth running this a number of times with the stress job to make sure its not flaky as there is no guarantee objects will be collected,
mhdawson
mhdawson approved these changes Mar 24, 2018
View reviewed changes
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 with a suggeston
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 26, 2018 •
edited
Loading

@mhdawson it seems node-stress-single-test is not configured to run make build-addons-napi.
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 27, 2018

@mhdawson I ran https://ci.nodejs.org/job/node-stress-single-test/1795/ on Debian 9 x86_64 and it passes 100% of the time.

I also found several other places where we rely on the result of a gc:

$ git grep -nEH3 '[.]gc[(][)]' -- test/addons* test/addons-napi/test_buffer/test.js-8-assert.strictEqual(binding.newBuffer().toString(), binding.theText); test/addons-napi/test_buffer/test.js-9-assert.strictEqual(binding.newExternalBuffer().toString(), binding.theText); test/addons-napi/test_buffer/test.js-10-console.log('gc1'); test/addons-napi/test_buffer/test.js:11:global.gc(); test/addons-napi/test_buffer/test.js-12-assert.strictEqual(binding.getDeleterCallCount(), 1); test/addons-napi/test_buffer/test.js-13-assert.strictEqual(binding.copyBuffer().toString(), binding.theText); test/addons-napi/test_buffer/test.js-14- -- test/addons-napi/test_buffer/test.js-16-assert.strictEqual(binding.bufferHasInstance(buffer), true); test/addons-napi/test_buffer/test.js-17-assert.strictEqual(binding.bufferInfo(buffer), true); test/addons-napi/test_buffer/test.js-18-buffer = null; test/addons-napi/test_buffer/test.js:19:global.gc(); test/addons-napi/test_buffer/test.js-20-console.log('gc2'); test/addons-napi/test_buffer/test.js-21-assert.strictEqual(binding.getDeleterCallCount(), 2); -- test/addons-napi/test_general/test.js-62-let w = {}; test/addons-napi/test_general/test.js-63-test_general.wrap(w); test/addons-napi/test_general/test.js-64-w = null; test/addons-napi/test_general/test.js:65:global.gc(); test/addons-napi/test_general/test.js-66-const derefItemWasCalled = test_general.derefItemWasCalled(); test/addons-napi/test_general/test.js-67-assert.strictEqual(derefItemWasCalled, true, test/addons-napi/test_general/test.js-68- 'deref_item() was called upon garbage collecting a ' + -- test/addons-napi/test_general/test.js-89-test_general.testFinalizeWrap(z); test/addons-napi/test_general/test.js-90-test_general.removeWrap(z); test/addons-napi/test_general/test.js-91-z = null; test/addons-napi/test_general/test.js:92:global.gc(); test/addons-napi/test_general/test.js-93-const finalizeWasCalled = test_general.finalizeWasCalled(); test/addons-napi/test_general/test.js-94-assert.strictEqual(finalizeWasCalled, false, test/addons-napi/test_general/test.js-95- 'finalize callback was not called upon garbage collection.' + -- test/addons-napi/test_reference/test.js-26- throw e; test/addons-napi/test_reference/test.js-27- } test/addons-napi/test_reference/test.js-28- setImmediate(() => { test/addons-napi/test_reference/test.js:29: global.gc(); test/addons-napi/test_reference/test.js-30- runTests(i + 1, title, tests); test/addons-napi/test_reference/test.js-31- }); test/addons-napi/test_reference/test.js-32- } -- test/addons/buffer-free-callback/test.js-11- buf = null; test/addons/buffer-free-callback/test.js-12- binding.check(slice); test/addons/buffer-free-callback/test.js-13- slice = null; test/addons/buffer-free-callback/test.js:14: global.gc(); test/addons/buffer-free-callback/test.js:15: global.gc(); test/addons/buffer-free-callback/test.js:16: global.gc(); test/addons/buffer-free-callback/test.js-17-} test/addons/buffer-free-callback/test.js-18- test/addons/buffer-free-callback/test.js-19-check(64, 1, 0);
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 27, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/13890/
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 27, 2018

The following tests failed:

node-test-commit-plinux#16354 -> ppcle-ubuntu1404 -> parallel/test-process-redirect-warnings node-test-commit-linux#17390 -> alpine-last-latest-x64 -> parallel/test-http-client-timeout-agent node-test-commit-smartos#16341 -> smartos15-64 -> parallel/test-file-read-noexist node-test-commit-linux-containered#3200 -> ubuntu1604_sharedlibs_zlib_x64 -> parallel/test-http-client-timeout-agent

I don't believe they are related.
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 27, 2018

Landed in 9e22647.
gabrielschulhof pushed a commit that referenced this pull request Mar 27, 2018
n-api: ensure in-module exceptions are propagated
9e22647 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: #19437 PR-URL: #19537 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof gabrielschulhof deleted the n-api-test-init-exception branch March 27, 2018 18:10
@mhdawson mhdawson mentioned this pull request Mar 29, 2018
Closed
targos pushed a commit that referenced this pull request Apr 2, 2018
@targos
n-api: ensure in-module exceptions are propagated
b0a3a44 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: #19437 PR-URL: #19537 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@targos targos mentioned this pull request Apr 4, 2018
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
n-api: ensure in-module exceptions are propagated
65b06de 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: nodejs#19437 PR-URL: nodejs#19537 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
n-api: ensure in-module exceptions are propagated
f4b0f7a 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: nodejs#19437 PR-URL: nodejs#19537 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: ensure in-module exceptions are propagated
e6ccdfb 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: #19437 Backport-PR-URL: #19447 PR-URL: #19537 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 1, 2018
@MylesBorins
n-api: ensure in-module exceptions are propagated
92f699e 
Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: #19437 Backport-PR-URL: #19265 PR-URL: #19537 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
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++. node-api Issues and PRs related to the Node-API.

4 participants

@gabrielschulhof @jasnell @mhdawson @nodejs-github-bot