timers: fix eventloop block #15072

zhangzifa · 2017-08-29T09:36:43Z

When there are at least 2 timers set by setInterval whose callback
execution are longer than interval, the eventloop will be blocked.

This commit fix the above bug.

Fixes: #15068

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

jasnell · 2017-08-29T17:03:49Z

Can you provide (and include in this PR) a test that fails without this fix?

addaleax · 2017-08-30T14:30:30Z

I appreciate the review request but I don't really know what this change effects :) I agree that a test case would be helpful.

addaleax · 2017-08-30T14:43:14Z

Eeeh, okay, I think I got it, and this might make sense?

Basically, the question is whether interval timers should block the event loop if they are 100 % busy beyond their set interval or not. Given that, as mentioned in the issue, this "worked" on Node 4.x, I'm inclined to say this is the right thing to do.

addaleax

LGTM but really should come with a test case. If you need anything for that, let us know :)

BridgeAR · 2017-09-08T03:09:29Z

Ping @zhangzifa

zhangzifa · 2017-09-11T09:26:28Z

@BridgeAR test case added, sorry for late.

BridgeAR · 2017-09-13T18:52:02Z

test/sequential/test-timers-block-eventloop.js

+      clearInterval(t3);
+    }));
+  })).end();
+}));


I am not sure why the server is included in the test. And if I am correct this could actually obfuscate what we really want to test if the server responds in below than 10 milliseconds.

Please remove it therefore and find a simpler test case.

simpler test case updated.

zhangzifa · 2017-09-15T10:40:11Z

@BridgeAR How about the simplified test case? :-)

@Fishrock123 Any comments? Thanks.

BridgeAR · 2017-09-19T20:49:59Z

test/sequential/test-timers-block-eventloop.js

+    clearInterval(t1);
+    clearInterval(t2);
+    clearInterval(t3);
+  });


This is somewhat better but I still do not understand why there is any need for a extra function call. It should be sufficient without the fs.stat call. Just the plain set timeout is necessary as far as I can tell.

Just make sure a different event is blocked. Per my local test, timer with different interval is not blocked.

BridgeAR · 2017-09-24T15:37:03Z

@zhangzifa this PR is almost ready to land. Would you mind taking another look? :-)

zhangzifa · 2017-09-30T06:09:29Z

@BridgeAR I have just rebased the commits to simplify the commit log. Ready to land from my pov.

jasnell · 2017-10-01T05:13:23Z

test/sequential/test-timers-block-eventloop.js

+}, 10);
+
+const t3 = setTimeout(() => {
+  throw new Error('eventloop blocked!');


Why throw here? If the intent is that this function should not be called, then using common.mustNotCall() would be better.

Yes, it's better, updated as your suggestion.

BridgeAR · 2017-10-02T11:50:57Z

test/sequential/test-timers-block-eventloop.js

+  common.busyLoop(15);
+}, 10);
+
+const t3 = setTimeout(common.mustNotCall('eventloop blocked!'), 100).unref();


Nit - the unref should not be necessary as the timeout is cleared later on.

yes, updated again. :-)

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. Fixes: nodejs#15068

lance · 2017-10-16T20:57:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10766/

jasnell · 2017-10-18T18:43:11Z

Failures in CI are unrelated.

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. PR-URL: #15072 Fixes: #15068 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-10-18T19:00:44Z

Landed in e647c5d

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. PR-URL: #15072 Fixes: #15068 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. PR-URL: nodejs/node#15072 Fixes: nodejs/node#15068 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. PR-URL: #15072 Fixes: #15068 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. PR-URL: nodejs/node#15072 Fixes: nodejs/node#15068 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

When an interval takes as long or longer to run as its timeout setting and the roundtrip from rearm() to its deferal takes exactly 1ms, that interval can then block the event loop. This is an edge case of another recently fixed bug (which in itself was an edge case). Refs: nodejs#15072

When an interval takes as long or longer to run as its timeout setting and the roundtrip from rearm() to its deferal takes exactly 1ms, that interval can then block the event loop. This is an edge case of another recently fixed bug (which in itself was an edge case). PR-URL: #18486 Refs: #15072 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

When an interval takes as long or longer to run as its timeout setting and the roundtrip from rearm() to its deferal takes exactly 1ms, that interval can then block the event loop. This is an edge case of another recently fixed bug (which in itself was an edge case). PR-URL: nodejs#18486 Refs: nodejs#15072 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 29, 2017

zhangzifa mentioned this pull request Aug 29, 2017

timers: eventloop is blocked when there are two or more callback duration is larger than interval #15068

Closed

jasnell requested review from Fishrock123 and addaleax August 29, 2017 17:03

addaleax approved these changes Aug 30, 2017

View reviewed changes

addaleax mentioned this pull request Aug 30, 2017

timers distribution predictability #15081

Closed

BridgeAR requested changes Sep 13, 2017

View reviewed changes

BridgeAR requested changes Sep 19, 2017

View reviewed changes

zhangzifa force-pushed the timer_fix branch from a2bf72a to 4291de8 Compare September 30, 2017 06:08

jasnell reviewed Oct 1, 2017

View reviewed changes

zhangzifa force-pushed the timer_fix branch from 87532c7 to 3cf0386 Compare October 2, 2017 11:40

BridgeAR approved these changes Oct 2, 2017

View reviewed changes

timers: fix eventloop block

ba12e62

When there are at least 2 timers set by setInterval whose callback execution are longer than interval, the eventloop will be blocked. This commit fix the above bug. Fixes: nodejs#15068

zhangzifa force-pushed the timer_fix branch from 96b7d22 to ba12e62 Compare October 2, 2017 14:20

tniessen approved these changes Oct 2, 2017

View reviewed changes

jasnell approved these changes Oct 2, 2017

View reviewed changes

jasnell closed this Oct 18, 2017

targos mentioned this pull request Oct 19, 2017

Flaky test-timers-block-eventloop #16310

Closed

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

marzelin mentioned this pull request Nov 15, 2019

non-deterministic order of execution of setTimeout vs setImmediate nodejs/help#392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: fix eventloop block #15072

timers: fix eventloop block #15072

zhangzifa commented Aug 29, 2017

jasnell commented Aug 29, 2017

addaleax commented Aug 30, 2017

addaleax commented Aug 30, 2017

addaleax left a comment

BridgeAR commented Sep 8, 2017

zhangzifa commented Sep 11, 2017

BridgeAR Sep 13, 2017

zhangzifa Sep 14, 2017

zhangzifa commented Sep 15, 2017

BridgeAR Sep 19, 2017

zhangzifa Sep 30, 2017

BridgeAR commented Sep 24, 2017

zhangzifa commented Sep 30, 2017

jasnell Oct 1, 2017

zhangzifa Oct 2, 2017

BridgeAR Oct 2, 2017

zhangzifa Oct 2, 2017

lance commented Oct 16, 2017

jasnell commented Oct 18, 2017

jasnell commented Oct 18, 2017

timers: fix eventloop block #15072

timers: fix eventloop block #15072

Conversation

zhangzifa commented Aug 29, 2017

Checklist

Affected core subsystem(s)

jasnell commented Aug 29, 2017

addaleax commented Aug 30, 2017

addaleax commented Aug 30, 2017

addaleax left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 8, 2017

zhangzifa commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangzifa commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 24, 2017

zhangzifa commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance commented Oct 16, 2017

jasnell commented Oct 18, 2017

jasnell commented Oct 18, 2017