-
Notifications
You must be signed in to change notification settings - Fork 461
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
Async worker and error handling documentation #272
Conversation
Changed version of node-addon-api on the package.json example see: [issue 206](#206 (comment))
doc/async_operations.md
Outdated
Node.js native add-ons often need to execute long running tasks and to avoid of |
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.
remove of
doc/async_operations.md
Outdated
Node.js native add-ons often need to execute long running tasks and to avoid of | ||
blocking the **event loop** they have to accomplish to them in the asynchronous way. | ||
Lets do a quick overview of how asynchronous code work in Node.js. In our model |
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 would drop the sentence Lets ... Node.js
and change In our model
to be `In the Node.js model'
doc/async_operations.md
Outdated
blocking the **event loop** they have to accomplish to them in the asynchronous way. | ||
Lets do a quick overview of how asynchronous code work in Node.js. In our model | ||
of execution we have **two threads**, the first is the **event loop** thread, that |
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.
'we have' -> 'there are', the node.js guidance is to avoid you, we etc.
doc/async_operations.md
Outdated
of execution we have **two threads**, the first is the **event loop** thread, that | ||
represents the thread where or JavaScript code is executing in. We want avoid to | ||
stall the event loop thread doing heavy computation so we need to create a sencond |
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.
'stalling the event loop thread' -> blocking other work queued on the event loop by. Therefore, we need to do this work on another thread.
doc/async_operations.md
Outdated
Node.js native add-ons often need to execute long running tasks and to avoid of | ||
blocking the **event loop** they have to accomplish to them in the asynchronous way. |
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.
have to accomplish to them -> have to run them asynchronously from the event loop.
doc/async_operations.md
Outdated
Lets do a quick overview of how asynchronous code work in Node.js. In our model | ||
of execution we have **two threads**, the first is the **event loop** thread, that | ||
represents the thread where or JavaScript code is executing in. We want avoid to |
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.
Remove 'or' and 'in'
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.
We want avoid to -> In order to avoid
Hi @mhdawson When you have some time, could you review my first pass on AsyncWorker documentation? |
@NickNaso was out travelling this week. Will try to get to it Monday/Tuesday next week, or possibly on my travels for the summit in Berlin. |
doc/async_operations.md
Outdated
In the Node.js model of execution there are **two threads**, the first is the | ||
**event loop** thread, that represents the thread where JavaScript code is | ||
executing. The node.js guidance is to avoid you blocking other work queued on the |
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.
Node.js should be uppercase.
"avoid you"
event loop bythread"
doc/async_operations.md
Outdated
completed. | ||
|
||
Node Addon API provides an ABI-stable interface to support functions which covers |
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.
"which coversthat cover"
doc/async_operations.md
Outdated
|
||
Node Addon API provides an ABI-stable interface to support functions which covers | ||
the most common asynchronous use cases. You have two abstract class to implement |
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.
The general guidance is to not formulate sentences using "you". So, we should have "You haveThere are".
"abstract classes"
doc/async_operations.md
Outdated
Node Addon API provides an ABI-stable interface to support functions which covers | ||
the most common asynchronous use cases. You have two abstract class to implement | ||
asynchronous operation: |
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.
"operations"
doc/async_operations.md
Outdated
- **[Async Context](async_context.md)** | ||
|
||
These two classes help you to manage asynchronous operations through an abstraction |
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.
"help you to"
doc/async_worker.md
Outdated
`AsyncWorker` is an abstract class that you can subclass to remove much of the |
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.
"muchmany"
doc/async_worker.md
Outdated
`AsyncWorker` is an abstract class that you can subclass to remove much of the | ||
tedious tasks on moving data between the event loop and worker threads. This |
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.
"onof"
doc/async_worker.md
Outdated
`AsyncWorker` is an abstract class that you can subclass to remove much of the | ||
tedious tasks on moving data between the event loop and worker threads. This | ||
class internally handles all the details of creating and executing asynchronous |
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.
"an asynchronous"
doc/async_worker.md
Outdated
### Queue | ||
|
||
Requests that the created work or task will be placed on the queue of the execution. |
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.
Perhaps instead of "placed on the queue of the execution" we could say "queued for execution".
doc/async_worker.md
Outdated
``` | ||
|
||
Returns the persistent object reference of the receiver objet set when the async |
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.
"object"
doc/async_worker.md
Outdated
|
||
Returns the persistent object reference of the receiver objet set when the async | ||
worker is created. |
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.
"iswas"
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.
Excellent!
…s about constructor
@gabrielschulhof @mhdawson @kfarnung I just fixed documentation as requested. if someone want take another look should be good. |
doc/async_operations.md
Outdated
blocking the **event loop** they have to run them asynchronously from the | ||
**event loop**. | ||
In the Node.js model of execution there are **two threads**, the first is the |
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.
Are there actually two threads? There's the main event loop thread that's constant and worker threads as needed.
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.
Maybe I can rewrite the sentence like this:
In the Node.js model of execution the event loop thread represents the thread where JavaScript code is executing.
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 think that's better.
doc/async_worker.md
Outdated
### Constructor | ||
|
||
Creates new async worker. |
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.
nit: "Creates a new AsyncWorker
"
@@ -1,5 +1,260 @@ | |||
# Async worker | |||
# AsyncWorker |
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.
Can you say more about the lifetime of the AsyncWorker? I may have missed it, but I'm trying to understand exactly when/how the AsyncWorker is destroyed. I have an issue currently where Jest never exits and the uv loop has a handle count of 1. I don't know what that object is, it could be something I did in JavaScript, but the AsyncWorker I created is a suspicious candidate.
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.
LGTM
Believe all comments are addressed, plan to land, hopefully this afternoon. |
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.
Thank you for addressing my comments. Looks good and is certainly better than what we have now!
### Execute | ||
|
||
This method is used to execute some tasks out of the **event loop** on a libuv |
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.
This should probably warn that using Napi::Objects during the Execute operation will segfault.
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 think I covered that with As the method is not
+running on the main event loop, it must avoid calling any methods from node-addon-api
+or running any code that might invoke JavaScript.
PR-URL: #272 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Landed as 5a63f45 |
PR-URL: nodejs/node-addon-api#272 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#272 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#272 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#272 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
This is a first step on writing AsyncWorker documentation. Please review the document and suggest some improvements.