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

Resource is optional in napi_async_init #33153

Closed
legendecas opened this issue Apr 30, 2020 · 2 comments
Closed

Resource is optional in napi_async_init #33153

legendecas opened this issue Apr 30, 2020 · 2 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. node-api Issues and PRs related to the Node-API.

Comments

@legendecas
Copy link
Member

Maybe we should also think about adapting napi_async_init as it states that the resource is optional. As Node is meanwhile built to require an async resource it seems to be strange that we create an object on the fly if user doesn't provide one.
Removing the null check in napi_async_init would be no issue from ABI point of view but breaking from behavior. Not sure if this should be included in this PR.

Originally posted by @Flarna in #32930
@legendecas legendecas mentioned this issue Apr 30, 2020
Closed
4 tasks
@legendecas legendecas added node-api Issues and PRs related to the Node-API. async_hooks Issues and PRs related to the async hooks subsystem. labels Apr 30, 2020
mhdawson added a commit to mhdawson/io.js that referenced this issue Apr 30, 2020
@mhdawson
doc: update napi_async_init documentation
a62c146 
Fixes: nodejs#33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson mhdawson mentioned this issue Apr 30, 2020
Closed
3 tasks
@mhdawson
Copy link
Member

@legendecas, @Flarna this is what I'd suggest we do:

#33181

I don't think we should change the implementation as it would be breaking which we can't do for N-API but updating the doc does make sense.

@mhdawson
Copy link
Member

I've left as draft until I get feedback from @legendecas and @Flarna that this makes sense to them as well.

codebytere pushed a commit that referenced this issue May 7, 2020
@mhdawson @codebytere
doc: update napi_async_init documentation
cf88ed8 
Fixes: #33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33181
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
codebytere pushed a commit that referenced this issue Jun 7, 2020
@mhdawson @codebytere
doc: update napi_async_init documentation
033bc96 
Fixes: #33153

Change documentation to make async_resource required
as opposed to optional in napi-async_init.

Changes over time mean this parameter is required for
proper operation of async hooks (which are still experimental).
This changes the documentation to document what
callers should do. We are doing this only in the doc
in order to avoid a breaking change in N-API. We could
create a new version of the method for which the
parametrer is enforced as mandatory but we should only
do that once async hooks is no longer experimental. In
that case we could deprecate (but not remove this version
of the method).

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #33181
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

doc: update napi_async_init documentation mhdawson/io.js
2 participants
@legendecas @mhdawson