n-api: napi_make_callback emit async init event with resource of async_context by legendecas · Pull Request #32930 · nodejs/node

legendecas · 2020-04-19T12:35:06Z

instead of emitting async init with the receiver of the callback.

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

Qard · 2020-04-21T21:44:49Z

This duplicates a lot of code from AsyncWrap. It also appears to be a major breaking change to N-API which I was hoping to avoid as it makes it difficult to backport executionAsyncResource related code. Not sure how I feel about this. 😟

legendecas · 2020-04-22T03:52:48Z

This duplicates a lot of code from AsyncWrap.

Yes, this is for AsyncWrap not suitable in the case since the resource object in the N-API may have no internal slot, which fails the BaseObject's assertion.

It also appears to be a major breaking change to N-API

Could you elaborate on how this would be breaking to existing N-API codes? It seems to me doesn't break codes and behaviors? (They are not working as expected already).

Qard · 2020-04-22T19:16:19Z

cc @nodejs/n-api @nodejs/diagnostics

The approach seems reasonable. Could use some more review from people that know n-api better though, especially around ABI impact.

mhdawson · 2020-09-30T16:39:12Z

I think we discussed this in the last N-API team meeting and @legendecas just needs to find some time to get back to it.

legendecas · 2020-10-06T18:16:26Z

@addaleax @mhdawson @Flarna Sorry for the terrible long delay. I've updated the PR, PTAL :)

Qard

Overall LGTM, but might be good to do something with that unused resource or people might get confused if they try to use a different resource and it silently ignores it. 🤔

Qard · 2020-10-07T01:36:24Z

Should we perhaps use the resource_object to warn if it's different than the object contained in the AsyncContext?

@quad I see what you mean, but that might be a breaking change and we don't have breaking changes for N-API methods. If we felt strongly about it we should add a new method without the parameter and doc deprecate the existing method (but not remove of course). I think we did something similar for another API in terms of indicate /** ignored */

I was just thinking a log message warning, no change in behaviour. Is that still considered breaking? Either way, not blocking on that. Just a thought that it'd be nice to have some indicator when the API is used improperly.

To my knowledge node has no logger. Logging to stdout/stderr tends to break command line tools.
But I think we should find a way how to deprecate something in NAPI in a way more visible to users like comments in doc.
I think a compile time warning like it is used for node::MakeCallback would be great.

I think a compile time warning like it is used for node::MakeCallback would be great.
AFAICT, it is not possible to emit compile-time warning without changing the API shapes. i.e. the signature of node::MakeCallback was migrated to the one with an additional parameter async_context. Though in the case of n_api, the deprecation is the parameter in the middle of the parameter list. So a new n-api function without the parameter will be required and the one existing can be deprecated.

mhdawson

LGTM

nodejs-github-bot · 2020-10-16T20:32:51Z

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

instead of emit async init with receiver of the callback.

nodejs-github-bot · 2020-10-30T23:06:25Z

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

github-actions · 2020-10-31T00:18:03Z

Landed in 5da2512...3a7537d

instead of emit async init with receiver of the callback. PR-URL: #32930Fixes: #32898 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Refs: #32930

Refs: #32930 PR-URL: #49180 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Refs: nodejs/node#32930 PR-URL: nodejs/node#49180 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Apr 19, 2020

legendecas marked this pull request as ready for review April 21, 2020 02:17

legendecas mentioned this pull request Apr 21, 2020
n-api: napi_make_callback with async_hooks.executionAsyncResource #32898
Closed

addaleax reviewed Apr 22, 2020
View reviewed changes

Comment threadsrc/node_api.cc Outdated

Flarna reviewed Apr 23, 2020
View reviewed changes

Comment threadsrc/node_api.cc Outdated

Flarna reviewed Apr 23, 2020
View reviewed changes

Comment threadsrc/node_api.cc Outdated

legendecas mentioned this pull request Apr 30, 2020
Resource is optional in napi_async_init#33153
Closed

Qard mentioned this pull request May 19, 2020
src: use symbol to store AsyncWrap resource #31745
Closed
2 tasks

Flarna mentioned this pull request May 28, 2020
AsyncLocalStorage potential issue ? #33580
Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72Compare May 31, 2020 12:18

Flarna mentioned this pull request Sep 23, 2020
Proposal: mark AsyncResource and AsyncLocalStorage as stable #35286
Closed

legendecas force-pushed the napi_async_context branch 3 times, most recently from eb5fdef to 242acadCompare October 6, 2020 18:15

Qard approved these changes Oct 7, 2020
View reviewed changes

mhdawson approved these changes Oct 8, 2020
View reviewed changes

Flarna reviewed Oct 9, 2020
View reviewed changes

Comment threadsrc/node_api.cc Outdated

Flarna reviewed Oct 9, 2020
View reviewed changes

Comment threadsrc/node_api.cc Outdated

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 16, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 16, 2020

This comment has been minimized.
Sign in to view

n-api: napi_make_callback emit async init with resource of async_context
6ccd5a8
instead of emit async init with receiver of the callback.

legendecas force-pushed the napi_async_context branch from 777e9b5 to 6ccd5a8Compare October 30, 2020 13:03

This comment has been minimized.
Sign in to view

Flarna approved these changes Oct 31, 2020
View reviewed changes

Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2020

github-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2020

github-actionsBot closed this Oct 31, 2020

legendecas deleted the napi_async_context branch October 31, 2020 06:29

targos mentioned this pull request Nov 3, 2020
v15.1.0 release proposal #35948
Merged

gabrielschulhof mentioned this pull request Nov 13, 2020
n-api: duplicated parameters on napi_open_callback_scope #32897
Closed

BethGriggs mentioned this pull request Dec 10, 2020
v14.15.2 proposal #36476
Merged

tniessen added a commit that referenced this pull request Aug 15, 2023
doc: fix wording in napi_async_init
3047ead
Refs: #32930

tniessen mentioned this pull request Aug 15, 2023
doc: fix wording in napi_async_init #49180
Merged

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
doc: fix wording in napi_async_init
777931f
Refs: #32930 PR-URL: #49180 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos pushed a commit that referenced this pull request Nov 27, 2023
doc: fix wording in napi_async_init
3b82e9a
Refs: #32930 PR-URL: #49180 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Uh oh!

Uh oh!

Conversation

legendecas commented Apr 19, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Qard commented Apr 21, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legendecas commented Apr 22, 2020

Uh oh!

Qard commented Apr 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhdawson commented Sep 30, 2020

Uh oh!

legendecas commented Oct 6, 2020

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

QardOct 7, 2020

Choose a reason for hiding this comment

Uh oh!

mhdawsonOct 8, 2020

Choose a reason for hiding this comment

Uh oh!

QardOct 8, 2020

Choose a reason for hiding this comment

Uh oh!

FlarnaOct 9, 2020

Choose a reason for hiding this comment

Uh oh!

legendecasOct 15, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 16, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Oct 30, 2020

Uh oh!

github-actionsBot commented Oct 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

legendecas commented Apr 19, 2020•
edited
Loading

Qard commented Apr 21, 2020•
edited
Loading

legendecasOct 15, 2020•
edited
Loading