Fixed some details related to the event loop #1603
Conversation
|
cc @nodejs/documentation and maybe @nodejs/libuv. |
|
cc/ @Fishrock123 |
| @@ -76,12 +76,14 @@ actually uses - are those above._ | |||
|
|
|||
| * **timers**: this phase executes callbacks scheduled by `setTimeout()` | |||
| and `setInterval()`. | |||
| * **I/O callbacks**: executes almost all callbacks with the exception of | |||
| close callbacks, the ones scheduled by timers, and `setImmediate()`. | |||
| * **pending callbacks**: executes I/O callbacks deferred to the next loop | |||
There was a problem hiding this comment.
IMO, I/O is more appropriate than pending here, 'cause close callbacks wouldn't be executed here, so I/O makes more sense in this phase.
There was a problem hiding this comment.
@marswong Please keep in mind that I'm not a C programmer, so most of my claims are based on libuv's event loop overview. Perhaps you could point to source code where Node.js has deviated or where the libuv doc is off.
I/O alone seems to indicate this is where all or most I/O callbacks are executed. However, this is the responsibility of the poll phase. Only some callbacks that were deliberately delayed will be executed in this phase (hence "pending").
Originally, I suggested renaming this phase to pending I/O callbacks, but that seemed a little long (especially for the ascii art), so I shortened it to pending callbacks and used the brief description to elaborate. As a reference, libuv calls this phase call pending callbacks.
| * **check**: `setImmediate()` callbacks are invoked here. | ||
| * **close callbacks**: e.g. `socket.on('close', ...)`. | ||
| * **close callbacks**: some close callbacks, e.g. `socket.on('close', ...)`. |
There was a problem hiding this comment.
with or without "some close callbacks" are both ok :)
There was a problem hiding this comment.
My worry is that someone would never read the detail and assume that all close callbacks are executed in this phase. Here's what the detail says:
If a socket or handle is closed abruptly (e.g. socket.destroy()), the 'close' event will be emitted in this phase. Otherwise it will be emitted via process.nextTick().
So it's actually via process.nextTick() (or after any phase) where most close callbacks will be executed. Adding "some close callbacks" should lead folks to read the details if they want to know more.
|
|
||
| ### poll | ||
|
|
||
| The **poll** phase has two main functions: | ||
|
|
||
| 1. Executing scripts for timers whose threshold has elapsed, then | ||
| 1. Calculating how long it should block and poll for I/O, then |
There was a problem hiding this comment.
Calculating doesn't cover the meaning of Executing here, and the main purpose of poll phase is to execute callbacks. So it'd be better to remain the same here or just modify Executing scripts to be Executing callbacks :)
There was a problem hiding this comment.
Calculating doesn't cover the meaning of Executing here
Agreed, I believe it was a mistake to put "executing" here. As far as I know, it's the responsibility of the timers and check phases to execute timer callbacks - depending on whether the timer was scheduled with setTimeout(), setInterval(), or setImmediate().
Please see #7 and #8 of the libuv overview for more detail. Perhaps this is an area where Node.js deviates from what libuv does by default?
There was a problem hiding this comment.
Yep, Node.js eventloop might differ a little from libuv's I/O loop, we set a UV_RUN_ONCE mode to make sure at least one callback must be invoked when it returns. Maybe @tniessen could give a more detailed explanation, thx :)
|
Is something still missing or are we good to go? Would like to merge this PR. |
I don't think anything is missing. It would be good to have some more reviews from people who have worked on this (ping @bnoordhuis and @Fishrock123, not sure who else), but otherwise I think this can land. |
|
Ok, as we didn't get any further reviews, I'm merging this now. If there are still any additions, we can handle them in another PR. @dmcghan Thank you very much for this update! |
No description provided.