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

N-API documentation incorectly states napi_create_async_work is not multithreaded #19071

Closed
ebickle opened this issue Mar 1, 2018 · 0 comments

Comments

@ebickle
Copy link
Contributor

ebickle commented Mar 1, 2018

  • Version: 9.7.0
  • Platform: Windows 7, Build 7601: Service Pack 1, 64-bit.
  • Subsystem: Documentation / N-API

The N-API documentation for the Custom Asynchronous Operations (https://nodejs.org/api/n-api.html#n_api_custom_asynchronous_operations) incorrectly states that napi_create_async_work (Simple Asynchronous Operations) executes on the main event loop.

This is incorrect; simple asynchronous operations execute from the Node.js Worker Thread Pool. People who read the documentation may make the dangerous assumption that asynchronous work queued via N-API will never execute in parallel with the main event loop.

I've implemented my own simple test on 64-bit Windows 7 using the N-API Simple Asynchronous Operations and verified that Execute is called from a separate thread-id.

ebickle added a commit to ebickle/node that referenced this issue Mar 1, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Fixes: nodejs#19071
addaleax pushed a commit to addaleax/node that referenced this issue Mar 5, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

PR-URL: nodejs#19073
Fixes: nodejs#19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 12, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

PR-URL: nodejs#19073
Fixes: nodejs#19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 16, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

PR-URL: nodejs#19073
Fixes: nodejs#19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 16, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

Backport-PR-URL: #19447
PR-URL: #19073
Fixes: #19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue May 1, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

Backport-PR-URL: #19265
PR-URL: #19073
Fixes: #19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Documentation for N-API Custom Asynchronous Operations incorrectly
stated that async execution happens on the main event loop.
Added details to napi_create_async_work about which threads are
used to invoke the execute and complete callbacks.

Changed 'async' to 'asynchronous' in the documentation for Custom
Asynchronous Operations. Changed "executes in parallel" to "can
execute in parallel" for the documentation of napi_create_async_work
execute parameter.

PR-URL: nodejs#19073
Fixes: nodejs#19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant