-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
doc: add call-once note to napi_queue_async_work #27582
Conversation
edfae0a
to
5c51956
Compare
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last nit:
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
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
5c51956
to
2838ab4
Compare
Landed in 6be5c3b. |
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>
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>
Add note to
napi_queue_async_work()
indicating that, upon successfulreturn, it must not be called again with the same work item.
Fixes: #27217
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes