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

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.

src/node_api.cc

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

src/node_api.cc

@@ -627,26 +692,19 @@ NAPI_NO_RETURN void napi_fatal_error(const char* location,
 }

 napi_status napi_open_callback_scope(napi_env env,
-                                     napi_value resource_object,
+                                     napi_value /** ignored */,


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

src/node_api.cc

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: #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

src/node_api.cc Show resolved Hide resolved

Flarna reviewed Apr 23, 2020

View reviewed changes

src/node_api.cc Outdated Show resolved Hide resolved

Flarna reviewed Apr 23, 2020

View reviewed changes

src/node_api.cc Outdated Show resolved Hide resolved

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

src/node_api.cc Outdated Show resolved Hide resolved

Flarna reviewed Oct 9, 2020

View reviewed changes

src/node_api.cc Outdated Show resolved Hide resolved

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

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>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #32930

legendecas commented Apr 19, 2020 •

edited

Qard commented Apr 21, 2020 •

edited

legendecas commented Apr 22, 2020

Qard commented Apr 22, 2020

mhdawson commented Sep 30, 2020

legendecas commented Oct 6, 2020

Qard left a comment

Qard Oct 7, 2020

mhdawson Oct 8, 2020

Qard Oct 8, 2020

Flarna Oct 9, 2020

legendecas Oct 15, 2020 •

edited

mhdawson left a comment

nodejs-github-bot commented Oct 16, 2020

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

github-actions bot commented Oct 31, 2020

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 #32930

Conversation

legendecas commented Apr 19, 2020 • edited

Checklist

Qard commented Apr 21, 2020 • edited

legendecas commented Apr 22, 2020

Qard commented Apr 22, 2020

mhdawson commented Sep 30, 2020

legendecas commented Oct 6, 2020

Qard left a comment

Choose a reason for hiding this comment

Qard Oct 7, 2020

Choose a reason for hiding this comment

mhdawson Oct 8, 2020

Choose a reason for hiding this comment

Qard Oct 8, 2020

Choose a reason for hiding this comment

Flarna Oct 9, 2020

Choose a reason for hiding this comment

legendecas Oct 15, 2020 • edited

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 16, 2020

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

github-actions bot commented Oct 31, 2020

legendecas commented Apr 19, 2020 •

edited

Qard commented Apr 21, 2020 •

edited

legendecas Oct 15, 2020 •

edited