Skip to content
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.

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

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

Closed
wants to merge 3 commits into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Jan 10, 2018

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

@nodejs-github-bot 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
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@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.

src/node_api.cc Outdated

private:
node::CallbackScope scope;
};
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll take a look at that.

@addaleax 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
Copy link
Member Author

@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 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
@mhdawson
Copy link
Member Author

mhdawson commented Jan 15, 2018

@mhdawson
Copy link
Member Author

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

@mhdawson
Copy link
Member Author

mhdawson commented Jan 16, 2018

1000 runs on my machine without failure :(

@mhdawson
Copy link
Member Author

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
Copy link
Member Author

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.

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Feb 1, 2018
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
Copy link
Member Author

mhdawson commented Feb 5, 2018

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
Copy link
Member Author

mhdawson commented Feb 5, 2018

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/

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

static
napi_callback_scope JsCallbackScopeFromV8CallbackScope(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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");
Copy link
Member

Choose a reason for hiding this comment

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

Space after ==.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix

uv_loop_t* loop = nullptr;
NAPI_CALL(env, napi_get_uv_event_loop(env, &loop));

uv_work_t* req = new uv_work_t;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix

return exports;
}

} // namespace
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

no complaints from the linter but I can update.

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

10000 runs on an ubuntu 14 machine, no failures.

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

10000 runs on an ubuntu 16 machine as well.

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

Crashed in CI run on ubuntu17

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

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

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

Fixed warning reported by valgrind.

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

@mhdawson
Copy link
Member Author

mhdawson commented Feb 5, 2018

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/

MylesBorins added a commit that referenced this pull request Feb 21, 2018
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
MylesBorins added a commit that referenced this pull request Feb 22, 2018
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
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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
@mhdawson mhdawson deleted the callback_scope branch September 30, 2019 13:15
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++. lib / src Issues and PRs related to general changes in the lib or src directory. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

N-API feature request: expose API to resolve promises asynchronously without napi_create_async_work
5 participants