src: return early if nextTickQueue is empty #10274

Merged
merged 1 commit into from Dec 21, 2016

Projects

None yet

7 participants

@trevnorris
Contributor
trevnorris commented Dec 15, 2016 edited
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Description of change

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.

R=@bnoordhuis
R=@addaleax

CI: https://ci.nodejs.org/job/node-test-commit/6652/

@addaleax

LGTM with a question

+ * - setImmediate() uses node::MakeCallback() instead of
+ * node::AsyncWrap::MakeCallback(). Otherwise the test will always pass.
+ * Have not found a way to verify that node::MakeCallback() is used.
+ */
@addaleax
addaleax Dec 15, 2016 Member

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?

@Fishrock123
Fishrock123 Dec 19, 2016 Member

This seems like a good idea. @trevnorris?

@addaleax
addaleax Dec 19, 2016 Member

@Fishrock123 In response to my suggestion @trevnorris added the second (nested) setImmediate call below (which seems sufficient to me)

@trevnorris
Contributor

@addaleax Think the update to the test addresses what you wanted to see.

CI: https://ci.nodejs.org/job/node-test-commit/6672/

@addaleax

Yeah, that’s pretty much what I had in mind. Thanks, this still LGTM!

+
+process.on('exit', () => {
+ assert.ok(cntr > 0, '_tickDomainCallback was never called');
+});
@addaleax
addaleax Dec 16, 2016 Member

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).

@trevnorris
trevnorris Dec 16, 2016 Contributor

@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.

@trevnorris
trevnorris Dec 16, 2016 Contributor

Likewise, adding mustCall() to process.on('exit' is pointless since mustCall callbacks are checked during 'exit'.

@addaleax
addaleax Dec 17, 2016 Member

Though on my machine it would be common.mustCall(., 3)

I think that settles it, thanks for checking!

+ require('domain');
+ setImmediate(() => setImmediate(() => {
+ allsGood = true;
+ process.nextTick(() => {});
@Fishrock123
Fishrock123 Dec 16, 2016 Member

These should probably also all be wrapped in common.mustCall()

@trevnorris
trevnorris Dec 16, 2016 Contributor

I can set mustCall() around the setImmediate()'s, but not here. Since by overriding _tickDomainCallback() we loose the ability to process the nextTickQueue.

@trevnorris
trevnorris Dec 16, 2016 Contributor

Also wrapping _tickDomainCallback() with mustCall() likewise isn't necessary since cntr is being checked on 'exit'.

@trevnorris
Contributor

@addaleax @Fishrock123 Issues have either been addressed by commit or comment.

CI: https://ci.nodejs.org/job/node-test-commit/6691/

@trevnorris trevnorris src: return early if nextTickQueue is empty
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>
8f8db14
@trevnorris trevnorris closed this Dec 21, 2016
@trevnorris trevnorris deleted the trevnorris:makecallback-ret-early branch Dec 21, 2016
@trevnorris trevnorris merged commit 8f8db14 into nodejs:master Dec 21, 2016
@trevnorris trevnorris added a commit that referenced this pull request Dec 21, 2016
@trevnorris trevnorris src: return early if nextTickQueue is empty
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>
967a80a
@trevnorris trevnorris added a commit that referenced this pull request Dec 21, 2016
@trevnorris trevnorris src: return early if nextTickQueue is empty
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>
acbf806
@trevnorris
Contributor

Landed on v7.x-staging. Not critical so feel free to backport to LTS whenever.

@joyeecheung joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 2, 2017
@trevnorris @joyeecheung trevnorris + joyeecheung src: return early if nextTickQueue is empty
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>
8508206
@evanlucas evanlucas referenced this pull request Jan 3, 2017
Merged

v7.4.0 release proposal #10589

@evanlucas evanlucas added a commit that referenced this pull request Jan 3, 2017
@trevnorris @evanlucas trevnorris + evanlucas src: return early if nextTickQueue is empty
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>
5e5b1f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment