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.

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

napi: why napi can be called in async execute function? #1998

Closed
heroboy opened this issue Jun 25, 2019 · 4 comments
Closed

napi: why napi can be called in async execute function? #1998

heroboy opened this issue Jun 25, 2019 · 4 comments

Comments

@heroboy
Copy link

heroboy commented Jun 25, 2019

https://github.com/nodejs/node/blob/938e11882b96e19b443477571455088baaa054d8/test/node-api/test_async/test_async.cc#L24

See this function ,it calls napi_throw_type_error.
In the document:https://nodejs.org/docs/latest/api/n-api.html#n_api_napi_create_async_work, executor is called in another thread, so it can't call any napi function.
@gireeshpunathil
Copy link
Member

ping @nodejs/n-api

@addaleax
Copy link
Member

Yeah, I mean, this just seems like a bug in the test (we should use NAPI_ASSERT instead). @heroboy Thanks for pointing that out!

addaleax added a commit to addaleax/node that referenced this issue Jun 25, 2019
@addaleax
test: use assert() in N-API async test
a83a20f 
The `Execute()` callback is not allowed to call into JS, so
we should use `assert()` instead of potentially throwing JS errors.

Fixes: nodejs/help#1998
@addaleax addaleax mentioned this issue Jun 25, 2019
Closed
3 tasks
@mhdawson
Copy link
Member

@heroboy would you like to submit a PR for that? Otherwise, I'll look at doing that.

@mhdawson
Copy link
Member

Nevermind seems like there is already a PR that has been submitted to fix.

targos pushed a commit to nodejs/node that referenced this issue Jul 2, 2019
@addaleax @targos
test: use assert() in N-API async test
007965c 
The `Execute()` callback is not allowed to call into JS, so
we should use `assert()` instead of potentially throwing JS errors.

PR-URL: #28423
Fixes: nodejs/help#1998
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit to nodejs/node that referenced this issue Jul 2, 2019
@addaleax @targos
test: use assert() in N-API async test
79b1bf5 
The `Execute()` callback is not allowed to call into JS, so
we should use `assert()` instead of potentially throwing JS errors.

PR-URL: #28423
Fixes: nodejs/help#1998
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: use assert() in N-API async test addaleax/node
4 participants
@heroboy @addaleax @gireeshpunathil @mhdawson