Skip to content

Clarify when nextTickQueue is processed - #1804

Merged
1 commit merged into
nodejs:masterfrom
OmarDarwish:patch-1
Feb 2, 2019
Merged

Clarify when nextTickQueue is processed#1804
1 commit merged into
nodejs:masterfrom
OmarDarwish:patch-1

Conversation

@OmarDarwish

Copy link
Copy Markdown
Contributor

It was a little unclear for me when the nextTickQueue is processed (see this StackOverflow question). After some experimentation and more googling, I now understand the mechanics of this queue.

I'm hoping to make this a little more clear for the next reader!

@ghost

Copy link
Copy Markdown

/cc:@nodejs/documentation. Please have a careful check about this detailled describtions :)

@a0viedo

Copy link
Copy Markdown
Member

I think this is a better description but I'm no event loop expert. pinging people who can shed some light @addaleax@mcollina@Fishrock123

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sam-github

Copy link
Copy Markdown
Contributor

I think you may have found a problem, the current text may not be correct for all types of operations, but I am not sure the replacement is correct either. Perhaps not all operations are the same?

The original text used "operations" because not all phases contain events. Operations is a more generic term for what phases do:

While each phase is special in its own way, generally, when the event loop enters a given phase, it will perform any operations specific to that phase,

"events" in the sense of the event loop are read from the kernel only in the Poll phase, so the replacement shouldn't replace operations with events, they aren't the same.

I noticed what you noticed with your example code, too, when I wrote some exploratory code, but I think its particular to "immediates". In that case, it appears the entire immediate queue is executed sequentially, and then any ticks that were queued by the immediates are executed sequentially.

In contrast, when an I/O event causes a callback to be executed, and that callback calls nextTick(), my understanding is that the tick queue is guaranteed to be executed before any other I/O callback is executed. Normally on a lightly loaded system this happens because there is never more than one event processed per Poll phase, but even on a loaded system where libuv reads more than one event from the kernel with epoll() in a single shot, I believe this guarantee holds.

@mcollina

Copy link
Copy Markdown
Member

An I/O event triggers some JavaScript to be executed. At the end of that execution, the functions in the nextTick queue are executed.

@sam-github

Copy link
Copy Markdown
Contributor

@mcollina That is my understanding as well, so the change proposed in this PR is incorrect. It says that the nextTick queue won't be executed until the end of the entire phase.

@OmarDarwish I think that executing the entire queue of Immediates is a single "operation", from the point of view of clearing the nextTick queue, which isn't documented, and is what you noticed with your test code.

@OmarDarwish

Copy link
Copy Markdown
ContributorAuthor

I think its particular to "immediates". In that case, it appears the entire immediate queue is executed sequentially, and then any ticks that were queued by the immediates are executed sequentially.

executing the entire queue of Immediates is a single "operation", from the point of view of clearing the nextTick queue

I'm up for any change in the documentation that makes this particular (edge?) case well known. @sam-github what do you think is a better way to make this easier to understand for the reader?

@mcollina

Copy link
Copy Markdown
Member

@mcollina That is my understanding as well, so the change proposed in this PR is incorrect. It says that the nextTick queue won't be executed until the end of the entire phase.

I think we should clarify the phases, and reflect the fact that there is a single transition between C to JS for multiple immediates and timers. In fact, we are deduping them as a form of optimization and this is a side effect of that.

@OmarDarwish

Copy link
Copy Markdown
ContributorAuthor

Took a stab at defining an operation, and giving an example of deduplication.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you remove this by accident?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep, lemme fix real quick 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. This line exceeds 80 characters limit now.
  2. It seems you can just fix the bottom reference if you prefer this URL.

@vsemozhetbytvsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some nits.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the there -> there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

optimzation -> optimization

@vsemozhetbytvsemozhetbytSep 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unneeded space before num.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these -> this
is considers -> considers
to have completed -> to have been completed?

@OmarDarwish

Copy link
Copy Markdown
ContributorAuthor

@vsemozhetbyt Thanks for double checking. I forgot to spell check before pushing. Fixing now!

@OmarDarwish

Copy link
Copy Markdown
ContributorAuthor

@sam-github@mcollina What do you think? If I understand the contributing policy, if/when you two approve we should be good to go to merge?

@mcollina

Copy link
Copy Markdown
Member

I think responsibility for this repo is to @nodejs/website.

@dmcghan

Copy link
Copy Markdown
Contributor

I just realized this doc doesn't discuss promises, which I believe have their own queue and processing timings which make them unique.

asyncfunctiondoWork(char){console.log('promise',char);}// dedup.jsconstfoo=[1,2];constbar=['a','b'];foo.forEach(num=>{setImmediate(()=>{console.log('setImmediate',num);bar.forEach(async(char)=>{process.nextTick(()=>{console.log('process.nextTick',char);});awaitdoWork(char);});});});
setImmediate 1
promise a
promise b
setImmediate 2
promise a
promise b
process.nextTick a
process.nextTick b
process.nextTick a
process.nextTick b

Maybe this PR isn't the right place to address this...

@davisjam

Copy link
Copy Markdown
Contributor

FWIW Here is an academic paper on this by @matthewloring and others.

@amiller-gh

Copy link
Copy Markdown
Member

@mcollina, thanks for the ping.

This does fit in well with our redesign / getting started guide work, but I think its okay for the content to live in here for now. We're hoping to keep all getting started guides alongside the source code after we've figured out the docs site content ingestion pipeline, so it'll end up back here anyway. The new content here will be helpful in the meantime.

We'll definitely use this doc as a starting point for for the event loop page on the new site (likely take it wholesale) awesome job @OmarDarwish!

@apapirovski

Copy link
Copy Markdown

@dmcghan Your example isn't quite accurate. Promises would normally only resolve after the nextTicks. In your case, you have console.log executing in a sync manner. Meanwhile if you were to put the console.log after the await, the order would be more like:

setImmediate 1
setImmediate 2
process.nextTick a
process.nextTick b
process.nextTick a
process.nextTick b
promise a
promise b
promise a
promise b

Once #22842 lands though, it will be more like the browser:

setImmediate 1
process.nextTick a
process.nextTick b
promise a
promise b
setImmediate 2
process.nextTick a
process.nextTick b
promise a
promise b

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Italics here, not bold.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: No italics here. We overuse italics and bold for emphasis. Just don't do it. 😄

@Trott

Copy link
Copy Markdown
Member

Once #22842 lands though, it will be more like the browser:

So in theory, the correct content of these guides is dependent on the version of Node.js...

@Trott

Copy link
Copy Markdown
Member

(If this is an improvement over existing text, I'm totally OK with it landing with or without my nits addressed.)

@dmcghan

Copy link
Copy Markdown
Contributor

@apapirovski Interesting. Looking back, I'm not sure I meant to add the await operator. I was trying to add something to the promise queue but I wasn't concerned with it resolving before continuing execution of the current stack.

Maybe this is a bit simpler:

constfoo=[1,2];constbar=['a','b'];foo.forEach(num=>{setImmediate(()=>{console.log('setImmediate',num);bar.forEach(async(char)=>{process.nextTick(()=>{console.log('process.nextTick',char);});newPromise((resolve,reject)=>{console.log('promise',char);});});});});

You can reverse the order of the nextTick and Promise calls and the result is the same (expected):

setImmediate 1
promise a
promise b
setImmediate 2
promise a
promise b
process.nextTick a
process.nextTick b
process.nextTick a
process.nextTick b

This output surprises me. I'd have expected the output you showed:

setImmediate 1
setImmediate 2
process.nextTick a
process.nextTick b
process.nextTick a
process.nextTick b
promise a
promise b
promise a
promise b

I'm missing something. Does no

While I'd like to get a better understanding of this behavior, I was really just trying to point out that this level of detail isn't in the doc.

Do you have a link to 22842?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if this is the correct place. The next section answers why the behaviour mentioned in the line above is allowed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hmm, you're right. Let me fix this.

Comment threadlocale/en/docs/guides/event-loop-timers-and-nexttick.md Outdated
Comment threadlocale/en/docs/guides/event-loop-timers-and-nexttick.md Outdated
@ghost

Copy link
Copy Markdown

@OmarDarwish:There're too many changes here, so can you merge them by using "rebase..." in your local PC and make a push submit?

Not sure whether this post can be merged? I think it's been a long time....

Define an operation. Give example of dedup.
Remove empty section
Fix trigger happy deletions
Remove modififed REPL link
Spellcheck Deduplication section
Address Nits
* Remove italics.
* Change italics to bold
Move Deduplication sectino down
Update locale/en/docs/guides/event-loop-timers-and-nexttick.md
Co-Authored-By: OmarDarwish <om.drwsh@gmail.com>
Update locale/en/docs/guides/event-loop-timers-and-nexttick.md
Co-Authored-By: OmarDarwish <om.drwsh@gmail.com>
@OmarDarwish

Copy link
Copy Markdown
ContributorAuthor

@Maledong I've rebased and squashed

@ghost

Copy link
Copy Markdown

@OmarDarwish:Thanks!

@watson, @sam-github , @thefourtheye , @Trott :Any other ideas or suggestions? Can this be merged?

process.nextTick b
process.nextTick a
process.nextTick b
```

@ZYSzysZYSzysDec 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAIK, did the output incorrect ?
The output is always as below while my Node.js version is v11.4.0.

setImmediate 1
process.nextTick a
process.nextTick b
setImmediate 2
process.nextTick a
process.nextTick b

So it's indeed as @Trott said:

So in theory, the correct content of these guides is dependent on the version of Node.js...

Maybe we also need to explain this there.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You're right. Looks like something changed for node 11:

bash-3.2$ pbpaste
const foo = [1, 2];
const bar = ['a', 'b'];
foo.forEach(num => {
setImmediate(() => {
console.log('setImmediate', num);
bar.forEach(char => {
process.nextTick(() => {
console.log('process.nextTick', char);
});
});
});
});
bash-3.2$ nvm use 10.15.0
Now using node v10.15.0 (npm v6.4.1)
bash-3.2$ pbpaste | node
setImmediate 1
setImmediate 2
process.nextTick a
process.nextTick b
process.nextTick a
process.nextTick b
bash-3.2$ nvm use 11.0.0
Now using node v11.0.0 (npm v6.4.1)
bash-3.2$ pbpaste | node
setImmediate 1
process.nextTick a
process.nextTick b
setImmediate 2
process.nextTick a
process.nextTick b
bash-3.2$ nvm use 11.6.0
Now using node v11.6.0 (npm v6.5.0-next.0)
bash-3.2$ pbpaste | node
setImmediate 1
process.nextTick a
process.nextTick b
setImmediate 2
process.nextTick a
process.nextTick b

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It looks like guides is not segregated by version. What would be the best way to indicate that this behavior is only applicable for Node 10 and below?

Also, what changed in node 11?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is what changed nodejs/node#22842

@ghostghost deleted a comment from OmarDarwishFeb 1, 2019
@ghost

ghost commented Feb 1, 2019

Copy link
Copy Markdown

Can this be merged? It's been for ages :)

@a0viedo

Copy link
Copy Markdown
Member

I think all changes have been addressed I'm +1 on merging this.

This pull request was closed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@OmarDarwish@a0viedo@sam-github@mcollina@dmcghan@davisjam@amiller-gh@apapirovski@Trott@watson@thefourtheye@BridgeAR@vsemozhetbyt@ryzokuken@ZYSzys