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

gabrielschulhof · 2018-03-22T21:04:15Z

Fixes: #19437

Checklist

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

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

mhdawson · 2018-03-24T02:49:52Z

test/addons-napi/test_exception/test.js

+function testFinalize(binding) {
+  let x = test_exception[binding]();
+  x = null;
+  assert.throws(() => { global.gc(); }, /Error during Finalize/);


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

LGTM with a suggeston

gabrielschulhof · 2018-03-26T22:04:45Z

@mhdawson it seems node-stress-single-test is not configured to run make build-addons-napi.

gabrielschulhof · 2018-03-27T14:08:13Z

@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 · 2018-03-27T14:14:28Z

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

gabrielschulhof · 2018-03-27T17:27:43Z

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 · 2018-03-27T18:09:35Z

Landed in 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>

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>

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>

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 changed the title ~~n-api: ensure exceptions are propagated from init~~ [WIP] n-api: ensure exceptions are propagated from init Mar 22, 2018

gabrielschulhof force-pushed the n-api-test-init-exception branch from cffd0ab to 6500f8e Compare March 23, 2018 04:07

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 changed the title ~~n-api: ensure exceptions are propagated from init~~ n-api: ensure exceptions are propagated from addons Mar 23, 2018

gabrielschulhof changed the title ~~n-api: ensure exceptions are propagated from addons~~ n-api: ensure in-module exceptions are propagated Mar 23, 2018

gabrielschulhof force-pushed the n-api-test-init-exception branch from 6500f8e to 0e5f648 Compare March 23, 2018 13:14

gabrielschulhof force-pushed the n-api-test-init-exception branch from 0e5f648 to 5c15524 Compare March 23, 2018 13:24

mhdawson reviewed Mar 24, 2018

View reviewed changes

mhdawson approved these changes Mar 24, 2018

View reviewed changes

jasnell approved these changes Mar 24, 2018

View reviewed changes

gabrielschulhof closed this Mar 27, 2018

gabrielschulhof deleted the n-api-test-init-exception branch March 27, 2018 18:10

mhdawson mentioned this pull request Mar 29, 2018

n-api - failure in node-addon-api testing after change in node core #19673

Closed

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

MylesBorins pushed a commit that referenced this pull request May 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabrielschulhof commented Mar 22, 2018

mhdawson Mar 24, 2018

mhdawson left a comment

gabrielschulhof commented Mar 26, 2018 •

edited

Loading

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 27, 2018

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

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

Conversation

gabrielschulhof commented Mar 22, 2018

Checklist

mhdawson Mar 24, 2018

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Mar 26, 2018 • edited Loading

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 27, 2018

gabrielschulhof commented Mar 26, 2018 •

edited

Loading