n-api: add methods to open/close callback scope #18089

mhdawson · 2018-01-10T19:55:33Z

Fixes: #15604

Add support for the following methods;
napi_open_callback_scope
napi_close_callback_scope

These are needed when running asynchronous methods directly
using uv.

The originator of 15604 confirmed that it addressed
his requirement.

Checklist

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

Affected core subsystem(s)

n-api

addaleax

@mhdawson I think I figured out what is causing this – the async_hook that we enable here registers the process.emitWarning() call from loading the N-API addon as asynchronous activity because it contains a process.nextTick() call.

I’d just monkey-patch it to be a no-op, e.g. adding process.emitWarning = () => {}; before loading the test addon should fix the test.

addaleax · 2018-01-10T19:58:54Z

src/node_api.cc

+
+ private:
+  node::CallbackScope scope;
+};


I think we only needed that wrapper because V8 doesn’t allow heap-allocating their HandleScope classes, right? We should not require that for Node’s own CallbackScope class

Thanks, I'll take a look at that.

mhdawson · 2018-01-11T01:31:44Z

@addaleax thanks for your help/suggestions that resolved the test issue. I updated based on your comments and added the do so this is now ready for review.

mhdawson · 2018-01-15T18:43:59Z

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

mhdawson · 2018-01-15T19:42:15Z

Seems like there are some crashes in the CI, will need to investigate.

mhdawson · 2018-01-16T20:33:27Z

1000 runs on my machine without failure :(

mhdawson · 2018-01-16T20:40:33Z

Failing test:

not ok 1885 addons-napi/test_callback_scope/test
  ---
  duration_ms: 0.513
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 11)
  ...

mhdawson · 2018-01-17T23:26:46Z

10000 runs and no recreate, at this point I will probably not get back to this until I'm back from vacation the week of the 29th.

Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv.

mhdawson · 2018-02-05T15:40:50Z

Rebased and forced pushed change as it's been a while

CI run(lite) to validate we still see problems in parallel with me trying to recreate locally: https://ci.nodejs.org/job/node-test-pull-request-lite/147/

mhdawson · 2018-02-05T15:53:47Z

Lite CI ran passed, I think it had failed on LinuxOne before, but that does not necessarily mean anything as I think it was an intermittent issue (since I cannot recreate on my boxes)

Full CI to confirm it still recreates: https://ci.nodejs.org/job/node-test-pull-request/12947/

bnoordhuis · 2018-02-05T16:24:20Z

src/node_api.cc

@@ -251,6 +252,18 @@ V8EscapableHandleScopeFromJsEscapableHandleScope(
  return reinterpret_cast<EscapableHandleScopeWrapper*>(s);
 }

+static
+napi_callback_scope JsCallbackScopeFromV8CallbackScope(


The name is a bit of a misnomer, isn't it?

It's consistent with the naming of the other similar functions. We may want to change them all but I think that should be in a different PR.

bnoordhuis · 2018-02-05T16:25:26Z

test/addons-napi/test_callback_scope/binding.cc

+  napi_value args[4];
+
+  NAPI_CALL(env, napi_get_cb_info(env, info, &argc, nullptr, nullptr, nullptr));
+  NAPI_ASSERT(env, argc ==4 , "Wrong number of arguments");


Space after ==.

bnoordhuis · 2018-02-05T16:26:15Z

test/addons-napi/test_callback_scope/binding.cc

+    uv_loop_t* loop = nullptr;
+    NAPI_CALL(env, napi_get_uv_event_loop(env, &loop));
+
+    uv_work_t* req = new uv_work_t;


Can you use new uv_work_t()? That stops coverity from complaining about uninitialized members.

bnoordhuis · 2018-02-05T16:27:35Z

test/addons-napi/test_callback_scope/binding.cc

+  return exports;
+}
+
+}  // namespace


Doesn't the linter complain that you should be using // anonymous namespace?

no complaints from the linter but I can update.

mhdawson · 2018-02-05T16:33:20Z

10000 runs on an ubuntu 14 machine, no failures.

mhdawson · 2018-02-05T16:47:02Z

10000 runs on an ubuntu 16 machine as well.

mhdawson · 2018-02-05T16:58:45Z

Crashed in CI run on ubuntu17

mhdawson · 2018-02-05T16:58:58Z

https://ci.nodejs.org/job/node-test-commit-linux/16127/nodes=ubuntu1710-x64/console

mhdawson · 2018-02-05T17:48:46Z

Running test with valgrind to see if that flags any issues.

mhdawson · 2018-02-05T18:18:02Z

Fixed warning reported by valgrind.

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

mhdawson · 2018-02-05T19:46:15Z

New CI run looking good, only arm to complete and all passed. Since it was intermittent before going to start a second just in case I got "lucky" even though none of the earlier CI runs managed to pass across all the runs before the recent fixes.

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

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv. PR-URL: nodejs#18089 Fixes: nodejs#15604 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv. Backport-PR-URL: #19447 PR-URL: #18089 Fixes: #15604 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv. Backport-PR-URL: #19265 PR-URL: #18089 Fixes: #15604 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv. PR-URL: nodejs#18089 Fixes: nodejs#15604 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 10, 2018

addaleax approved these changes Jan 10, 2018

View reviewed changes

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. node-api Issues and PRs related to the Node-API. labels Jan 10, 2018

mhdawson mentioned this pull request Jan 10, 2018

N-API feature request: expose API to resolve promises asynchronously without napi_create_async_work #15604

Closed

mhdawson changed the title ~~WORK IN PROGRESS - n-api: add methods to open/close callback scope~~ n-api: add methods to open/close callback scope Jan 11, 2018

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR added the wip Issues and PRs that are still a work in progress. label Feb 1, 2018

n-api: add methods to open/close callback scope

a0f4492

Add support for the following methods; napi_open_callback_scope napi_close_callback_scope These are needed when running asynchronous methods directly using uv.

mhdawson force-pushed the callback_scope branch from f7be45a to a0f4492 Compare February 5, 2018 15:38

bnoordhuis approved these changes Feb 5, 2018

View reviewed changes

squash: address comments

e9200b4

squash: fix valgrind warnings

a1ea4c8

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

mhdawson deleted the callback_scope branch September 30, 2019 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: add methods to open/close callback scope #18089

n-api: add methods to open/close callback scope #18089

mhdawson commented Jan 10, 2018 •

edited

Loading

addaleax left a comment

addaleax Jan 10, 2018

mhdawson Jan 10, 2018

mhdawson commented Jan 11, 2018

mhdawson commented Jan 15, 2018 •

edited

Loading

mhdawson commented Jan 15, 2018

mhdawson commented Jan 16, 2018 •

edited

Loading

mhdawson commented Jan 16, 2018

mhdawson commented Jan 17, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

bnoordhuis Feb 5, 2018

mhdawson Feb 5, 2018

bnoordhuis Feb 5, 2018

mhdawson Feb 5, 2018

bnoordhuis Feb 5, 2018

mhdawson Feb 5, 2018

bnoordhuis Feb 5, 2018

mhdawson Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018 •

edited

Loading

mhdawson commented Feb 5, 2018 •

edited

Loading

n-api: add methods to open/close callback scope #18089

n-api: add methods to open/close callback scope #18089

Conversation

mhdawson commented Jan 10, 2018 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jan 11, 2018

mhdawson commented Jan 15, 2018 • edited Loading

mhdawson commented Jan 15, 2018

mhdawson commented Jan 16, 2018 • edited Loading

mhdawson commented Jan 16, 2018

mhdawson commented Jan 17, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018

mhdawson commented Feb 5, 2018 • edited Loading

mhdawson commented Feb 5, 2018 • edited Loading

mhdawson commented Jan 10, 2018 •

edited

Loading

mhdawson commented Jan 15, 2018 •

edited

Loading

mhdawson commented Jan 16, 2018 •

edited

Loading

mhdawson commented Feb 5, 2018 •

edited

Loading

mhdawson commented Feb 5, 2018 •

edited

Loading