src: return early if nextTickQueue is empty #10274
Conversation
trevnorris added c++ 
nodejs-github-bot added c++ trevnorris force-pushed the makecallback-ret-early branch from adad126 to 710315a Compare 
There was a problem hiding this comment.
Could these conditions be checked by adding a test for the reverse situation? I.e. making sure that if there are ticks pending, the _tickDomainCallback is actually executed?
There was a problem hiding this comment.
@Fishrock123 In response to my suggestion @trevnorris added the second (nested) setImmediate call below (which seems sufficient to me)
trevnorris force-pushed the makecallback-ret-early branch from 710315a to 77c90de Compare | @addaleax Think the update to the test addresses what you wanted to see. |
addaleax left a comment
addaleax left a comment There was a problem hiding this comment.
Yeah, that’s pretty much what I had in mind. Thanks, this still LGTM!
There was a problem hiding this comment.
If you think being stricter is okay, we could just wrap the fake _tickDomainCallback in common.mustCall(·, 2) (You can probably judge whether the number of calls is likely to change).
There was a problem hiding this comment.
@addaleax Though on my machine it would be common.mustCall(., 3). Because we can't actually process the nextTickQueue when overriding _tickDomainCallback() we can only guarantee it will be called at least once. I don't feel that being any stricter than that is possible w/o making some shaky assumptions on how the internals work.
There was a problem hiding this comment.
Likewise, adding mustCall() to process.on('exit' is pointless since mustCall callbacks are checked during 'exit'.
There was a problem hiding this comment.
Though on my machine it would be
common.mustCall(., 3)
I think that settles it, thanks for checking!
There was a problem hiding this comment.
These should probably also all be wrapped in common.mustCall()
There was a problem hiding this comment.
I can set mustCall() around the setImmediate()'s, but not here. Since by overriding _tickDomainCallback() we loose the ability to process the nextTickQueue.
There was a problem hiding this comment.
Also wrapping _tickDomainCallback() with mustCall() likewise isn't necessary since cntr is being checked on 'exit'.
trevnorris force-pushed the makecallback-ret-early branch from 77c90de to 4055548 Compare 
| @addaleax @Fishrock123 Issues have either been addressed by commit or comment. |
This brings the node::MakeCallback and node::AsyncWrap::MakeCallback implementations into alignment in that they return early if the nextTickQueue is empty after processing the MicrotaskQueue. Include test to make sure early return happens. Test has text explaining the conditions for the test to pass, since it relies on internal mechanisms that aren't guaranteed in the future. PR-URL: nodejs#10274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
trevnorris force-pushed the makecallback-ret-early branch from 4055548 to 8f8db14 Compare This brings the node::MakeCallback and node::AsyncWrap::MakeCallback implementations into alignment in that they return early if the nextTickQueue is empty after processing the MicrotaskQueue. Include test to make sure early return happens. Test has text explaining the conditions for the test to pass, since it relies on internal mechanisms that aren't guaranteed in the future. PR-URL: #10274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
| Landed on v7.x-staging. Not critical so feel free to backport to LTS whenever. |
This brings the node::MakeCallback and node::AsyncWrap::MakeCallback implementations into alignment in that they return early if the nextTickQueue is empty after processing the MicrotaskQueue. Include test to make sure early return happens. Test has text explaining the conditions for the test to pass, since it relies on internal mechanisms that aren't guaranteed in the future. PR-URL: #10274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This brings the node::MakeCallback and node::AsyncWrap::MakeCallback implementations into alignment in that they return early if the nextTickQueue is empty after processing the MicrotaskQueue. Include test to make sure early return happens. Test has text explaining the conditions for the test to pass, since it relies on internal mechanisms that aren't guaranteed in the future. PR-URL: #10274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This brings the node::MakeCallback and node::AsyncWrap::MakeCallback implementations into alignment in that they return early if the nextTickQueue is empty after processing the MicrotaskQueue. Include test to make sure early return happens. Test has text explaining the conditions for the test to pass, since it relies on internal mechanisms that aren't guaranteed in the future. PR-URL: nodejs/node#10274 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
8 participants
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesDescription of change
This brings the
node::MakeCallbackandnode::AsyncWrap::MakeCallbackimplementations into alignment in that they return early if the
nextTickQueueis empty after processing theMicrotaskQueue.Include test to make sure early return happens. Test has text explaining
the conditions for the test to pass, since it relies on internal
mechanisms that aren't guaranteed in the future.
R=@bnoordhuis
R=@addaleax
CI: https://ci.nodejs.org/job/node-test-commit/6652/