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

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

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

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

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.

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.

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

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

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.

LGTM

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

instead of emit async init with receiver of the callback.

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

Landed in 5da2512...3a7537d

instead of emit async init with receiver of the callback. PR-URL: #32930 Fixes: #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 thread src/node_api.cc Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread src/node_api.cc Outdated

Flarna reviewed Apr 23, 2020

View reviewed changes

Comment thread src/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 2935f72 Compare 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 242acad Compare 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 thread src/node_api.cc Outdated

Flarna reviewed Oct 9, 2020

View reviewed changes

Comment thread src/node_api.cc Outdated

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

github-actions Bot 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 6ccd5a8 Compare 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-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 31, 2020

github-actions Bot 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>

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

Uh oh!

Uh oh!

n-api: napi_make_callback emit async init event with resource of async_context - #32930

n-api: napi_make_callback emit async init event with resource of async_context#32930legendecas wants to merge 1 commit intonodejs:masternodejs/node:masterfrom legendecas:napi_async_contextlegendecas/node:napi_async_contextCopy head branch name to clipboard

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!

Qard Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

mhdawson Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

Qard Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

Flarna Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

legendecas Oct 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-actions Bot commented Oct 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

n-api: napi_make_callback emit async init event with resource of async_context#32930
legendecas wants to merge 1 commit into
nodejs:masterfrom
legendecas:napi_async_context

legendecas commented Apr 19, 2020 •
edited
Loading

Qard commented Apr 21, 2020 •
edited
Loading

legendecas Oct 15, 2020 •
edited
Loading