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.

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

doc: add call-once note to napi_queue_async_work #27582

Conversation

gabrielschulhof
Copy link
Contributor

Add note to napi_queue_async_work() indicating that, upon successful
return, it must not be called again with the same work item.

Fixes: #27217

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels May 6, 2019
@gabrielschulhof gabrielschulhof force-pushed the n-api-doc-queue-async-work-once branch from edfae0a to 5c51956 Compare May 6, 2019 15:34
doc/api/n-api.md Outdated
@@ -4060,7 +4060,8 @@ napi_status napi_queue_async_work(napi_env env,
Returns `napi_ok` if the API succeeded.

This API requests that the previously allocated work be scheduled
for execution.
for execution. Once it returns successfully, this API **must not be called again
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Micro-nit/question: Is "successfully" necessary here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always a fan of leaving off the bold/italics/etc. Not blocking, but suggesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"must not be called again" might stand some clarification? Like, will it throw? Or fail silently? Or result in a behavior that is undefined?

Copy link
Member

@Trott Trott May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last nit:

Suggested change
for execution. Once it returns successfully, this API **must not be called again
for execution. Once it returns, do not call the API with the same
`_napi_async_work` item again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott My understanding was that our style was to avoid addressing the reader. "do not call" is another way of saying "you", isn't it?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bunch of tiny comments. LGTM with or without the suggestions in those comments.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2019
Add note to `napi_queue_async_work()` indicating that, upon successful
return, it must not be called again with the same work item.

Fixes: nodejs#27217
PR-URL: nodejs#27582
@gabrielschulhof gabrielschulhof force-pushed the n-api-doc-queue-async-work-once branch from 5c51956 to 2838ab4 Compare May 13, 2019 05:22
@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

Landed in 6be5c3b.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request May 13, 2019
Add note to `napi_queue_async_work()` indicating that, upon successful
return, it must not be called again with the same work item.

Fixes: nodejs#27217
PR-URL: nodejs#27582
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@gabrielschulhof gabrielschulhof deleted the n-api-doc-queue-async-work-once branch May 13, 2019 16:58
targos pushed a commit that referenced this pull request May 14, 2019
Add note to `napi_queue_async_work()` indicating that, upon successful
return, it must not be called again with the same work item.

Fixes: #27217
PR-URL: #27582
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsafe to call napi_queue_async_work with already-queued napi_async_work
7 participants