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

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

 }

+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 = newuv_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#18089Fixes: nodejs#15604 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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: #18089Fixes: #15604 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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: #18089Fixes: #15604 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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#18089Fixes: nodejs#15604 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 scopeJan 11, 2018

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

cjihrig force-pushed the master branch from 993b716 to 082f952Compare 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 a0f4492Compare 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

Uh oh!

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

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

Uh oh!

Conversation

mhdawson commented Jan 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 11, 2018

Uh oh!

mhdawson commented Jan 15, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Jan 15, 2018

Uh oh!

mhdawson commented Jan 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Jan 16, 2018

Uh oh!

mhdawson commented Jan 17, 2018

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

mhdawson commented Feb 5, 2018

Uh oh!

mhdawson commented Feb 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Feb 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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